Sql statement api error messaging fixes. by cryptoe · Pull Request #14629 · apache/druid

cryptoe · 2023-07-20T12:29:42Z

This patch contains the following fixes:

Error message changes when durable storage is not enabled.
Changing the undocumented context param selectDestination values from TASK_REPORT and DURABLE_STORAGE to taskReport and durableStorage.
Follow up PR from Adding null fix for rows and col stats information. #14617
Documentation of context param selectDestination.

This PR has:

LakshSingla · 2023-07-20T13:06:25Z


 For queries where you want to use fault tolerance for workers,  set `faultTolerance` to `true`, which automatically sets `durableShuffleStorage` to `true`.

+For select queryies which want to write the final result to `durableStoage`, set `selectDestination`:`durableStorage`. Which shuffle mesh the job uses can still be controller by `durableShuffleStorage` flag ie. a combination where `selectDestination`:`durableStorage` and `durableShuffleStorage`:`false` is perfectly valid.


spelling of queries and durableStorage

Something seems off in the second sentence. I think it should be "controlled" instead of "controller".

durableStoage (first use) should be written as "durable storage" (without double quotes) since we are using it as a noun instead of code word.

We should use something else instead of "shuffle mesh" in the docs since it's not a well-documented term in the docs and could be simplified further. Wdyt about simplifying it to something with following structure:

Set `selectDestination`:`durableStorage` for select queries that want to write the final results to durable storage instead of the task reports. Saving the results in the durable storage allows users to .. (benefits for the user) The location where the workers can write the intermediate results is independent of the location where the final results get stored. Therefore "durableShuffleStorage":false and "selectDestination":"durableStorage" is a valid configuration to use in the query context, that instructs the controller to persist only the final result in the durable storage, and not the intermediate results.

I like you structure better. Going ahead with that.

LakshSingla · 2023-07-20T13:09:43Z

   * Writes all the results directly to the report.
   */
-  TASK_REPORT(false),
+  TASKREPORT("taskReport", false),


We should use a snake case for the static variable names. Please revert it to the original values.
Since this is changed for equality, we can create a static method that checks if the String provided equals the enum destination.

Suggested change

TASKREPORT("taskReport", false),

TASK_REPORT("taskReport", false),

This was done so that we are able to still use the query context enum methods. Also I think ResultFormat class also uses a similar trick. Checkout objectlines

LakshSingla · 2023-07-20T13:09:53Z

   * Writes the results as frame files to durable storage. Task report can be truncated to a preview.
   */
-  DURABLE_STORAGE(true);
+  DURABLESTORAGE("durableStorage", true);


Suggested change

DURABLESTORAGE("durableStorage", true);

DURABLE_STORAGE("durableStorage", true);

LakshSingla · 2023-07-20T13:16:43Z

  );
-  public static final String JSON_STRING = "{\"numTotalRows\":1,\"totalSizeInBytes\":1,\"resultFormat\":\"object\",\"dataSource\":\"ds\",\"pages\":[{\"numRows\":1,\"sizeInBytes\":1,\"id\":0}]}";
-  public static final String JSON_STRING_1 = "{\"numTotalRows\":1,\"totalSizeInBytes\":1,\"resultFormat\":\"object\",\"dataSource\":\"ds\",\"sampleRecords\":[[\"1\"],[\"2\"],[\"3\"]],\"pages\":[{\"numRows\":1,\"sizeInBytes\":1,\"id\":0}]}";
+  public static final String JSON_STRING = "{\"numTotalRows\":1,\"totalSizeInBytes\":1,\"resultFormat\":\"object\",\"dataSource\":\"ds\",\"pages\":[{\"id\":0,\"numRows\":1,\"sizeInBytes\":1}]}";


Can you add a few tests that ensure that nulls in the size or rows are not serialized?

LakshSingla · 2023-07-20T13:17:37Z

          key,
-          StringUtils.format("a value of enum [%s]", clazz.getSimpleName()),
+          StringUtils.format(
+              "referring to one of the values[%s] of enum [%s]",


There's inconsistent spacing between values[%s] and enum [%s]

LakshSingla · 2023-07-20T13:18:53Z

TY for picking up the older comments from the reviews!

LakshSingla · 2023-07-20T16:15:59Z


-For select queryies which want to write the final result to `durableStoage`, set `selectDestination`:`durableStorage`. Which shuffle mesh the job uses can still be controller by `durableShuffleStorage` flag ie. a combination where `selectDestination`:`durableStorage` and `durableShuffleStorage`:`false` is perfectly valid.
+Set `selectDestination`:`durableStorage` for select queries that want to write the final results to durable storage instead of the task reports. Saving the results in the durable
+storage allows users to fetch large result sets. The location where the workers write the intermediate results is different than the location where final results get stored. Therefore, `durableShuffleStorage`:`false` and


Suggested change

storage allows users to fetch large result sets. The location where the workers write the intermediate results is different than the location where final results get stored. Therefore, `durableShuffleStorage`:`false` and

storage allows users to fetch large result sets. The location where the workers write the intermediate results can be different from the location where the final results get stored. Therefore, `durableShuffleStorage`:`false` and

The location where the workers write the intermediate results is always different hence I would like to keep the wording as is unless you feel strongly about it.

LakshSingla · 2023-07-20T16:16:54Z

   * Writes all the results directly to the report.
   */
-  TASK_REPORT(false),
+  TASKREPORT("taskReport", false),


LakshSingla

A nitty comment, overall PR LGTM

* Error messaging fixes. * Static check fix * Review comments (cherry picked from commit 77e0c16)

* Error messaging fixes. * Static check fix * Review comments

Error messaging fixes.

0a2896c

github-actions Bot added the Area - Documentation label Jul 20, 2023

cryptoe added Area - MSQ For multi stage queries - https://github.com/apache/druid/issues/12262 and removed Area - Documentation labels Jul 20, 2023

cryptoe added this to the 27.0 milestone Jul 20, 2023

LakshSingla reviewed Jul 20, 2023

View reviewed changes

Static check fix

76769db

github-actions Bot added the Area - Documentation label Jul 20, 2023

Review comments

b89e2ce

LakshSingla reviewed Jul 20, 2023

View reviewed changes

LakshSingla approved these changes Jul 20, 2023

View reviewed changes

cryptoe merged commit 77e0c16 into apache:master Jul 20, 2023

cryptoe added a commit that referenced this pull request Jul 20, 2023

Sql statement api error messaging fixes. (#14629)

11a542f

* Error messaging fixes. * Static check fix * Review comments (cherry picked from commit 77e0c16)

sergioferragut pushed a commit to sergioferragut/druid that referenced this pull request Jul 21, 2023

Sql statement api error messaging fixes. (apache#14629)

44cdc63

* Error messaging fixes. * Static check fix * Review comments

vogievetsky mentioned this pull request Jul 26, 2023

Web console: add durable storage selector #14669

Merged


		For queries where you want to use fault tolerance for workers, set `faultTolerance` to `true`, which automatically sets `durableShuffleStorage` to `true`.

		For select queryies which want to write the final result to `durableStoage`, set `selectDestination`:`durableStorage`. Which shuffle mesh the job uses can still be controller by `durableShuffleStorage` flag ie. a combination where `selectDestination`:`durableStorage` and `durableShuffleStorage`:`false` is perfectly valid.

	TASKREPORT("taskReport", false),
	TASK_REPORT("taskReport", false),

	DURABLESTORAGE("durableStorage", true);
	DURABLE_STORAGE("durableStorage", true);

	storage allows users to fetch large result sets. The location where the workers write the intermediate results is different than the location where final results get stored. Therefore, `durableShuffleStorage`:`false` and
	storage allows users to fetch large result sets. The location where the workers write the intermediate results can be different from the location where the final results get stored. Therefore, `durableShuffleStorage`:`false` and

Conversation

cryptoe commented Jul 20, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

LakshSingla commented Jul 20, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

LakshSingla left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

LakshSingla commented Jul 20, 2023 •

edited

Loading