Using DruidExceptions in MSQ (changes related to the Broker) by LakshSingla · Pull Request #14534 · apache/druid

LakshSingla · 2023-07-06T11:39:29Z

Description

Most of the places in MSQ currently rely on the older error code, dating pre #14004. This is an attempt to fix the bits of the error code in the places where bits of MSQ reside in the broker. This extends upon the PR #14531.

The MSQTaskQueryMaker and the MSQTaskEngine might return a different error code for some invalid cases (which might not be inline with what the behavior originally was, but is correct now). For example, setting maxNumTasks to 1 would return a 500 error code, which is not right since its an invalid user input. This was the original behavior, and this PR changes that.
The EXTERN function returns a 400 error code if the parameters to it are incorrect (which is in line with what its behavior originally was and is correct)

Note: This PR doesn't unify the DruidExceptions with the MSQException

Release note

MSQ engine returns correct error codes for invalid user inputs in the query context

This PR has:

somu-imply

Looks good. Minor nits

imply-cheddar · 2023-07-07T04:54:37Z

+                          .build(
+                              e,
+                              "Unable to deserialize the insert granularity. Please retry the query with a "
+                              + "valid segment graularity"
+                          );


This message reads to me like it's trying to get to the end user, but a defensive exception is something that we expect to never happen (because the code doesn't allow it) and therefore it faces the developer. Given this message, are you sure that this isn't an InvalidSqlInput?

This would only be thrown if we can't serialize one of the objects predefined in the code which we are sure would get serialized. (Handles the jsonMapper.writeValueAsString throw clause). We never intend to encounter this exception, and if we do, there's something wrong with the code. Updated the error message to indicate this.

imply-cheddar · 2023-07-07T04:56:56Z

+      throw InvalidInput.exception(
+          "%s cannot be less than 2 since at least 1 controller and 1 worker is necessary.",
+          MultiStageQueryContext.CTX_MAX_NUM_TASKS
+      );


This message doesn't follow our conventions for how to write error messages or interpolate things. Please read style-conventions.md and adjust.

In this case, I would probably suggest

"MSQ context maxNumTasks [%,d] cannot be less than 2, since at least 1 controller and 1 worker is necessary"

imply-cheddar · 2023-07-07T04:57:28Z

+        throw DruidException.forPersona(DruidException.Persona.DEVELOPER)
+                            .ofCategory(DruidException.Category.DEFENSIVE)
+                            .build("Cannot INSERT with destination [%s]", ctxDestination);


This message again doesn't seem defensive, it seems like it's intended for the end-user and should be InvalidSqlInput no?

imply-cheddar · 2023-07-07T04:58:16Z

+        throw DruidException.forPersona(DruidException.Persona.DEVELOPER)
+                            .ofCategory(DruidException.Category.DEFENSIVE)
+                            .build("Unable to convert %s to a segment granularity", segmentGranularity);


This is throwing away the Exception e so we will never be able to figure out why we were unable to convert the value. It also doesn't follow our conventions for how to write and interpolate messages and I'm not sure that it's truly defensive?

Good point on keeping the exception. I updated the exception message to state why this is a defensive exception.

This parameter is added by Druid while processing PARTITIONED BY _____. User inputs whatever is in front of the partitioned by we populate the query context based on that. Therefore whatever hits this exception has been sanitized and populated by us (after already going through a transformation-serialization process). We don't ever wish to encounter this in a normal misinput situation, any exception or incorrect input from the user would have been flagged by the SQL parser or while serializing.

imply-cheddar · 2023-07-07T05:08:08Z

-                              )
-                          )
+            InvalidInput.exception(
+                "The statement sql api only supports sync mode[%s]. Please set context parameter [%s=%s] in the context payload",


I'm not sure I know what the statement sql api is? If it's only supposed to be one thing and this code knows what that one thing is and the user got their request to this line of the code, why does the user need to change their query to only do the one thing that this code can do? I.e. why not just ignore the context parameter entirely? Or overwrite it with the one value that this code can do?

Since the sql statement api will support the sync mode soon, I wanted the user to set the flag explicitly.

Rolling upgrades then become easy since we donot have a default mode/overridden mode.

The mode also changes the way the results are fetched.
If the mode set is async, the results are fetched via the results api.
If the mode set is sync, then the results are streamed back in the same post request. (At-least, that's how I envision it. )

I went through the code, and ExecutionMode is currently unused. I have left it as is since I am not aware if it will be utilized later, however, I changed the exception to defensive since this parameter isn't supported, unset, and not documented, therefore it shouldn't have been set.

(cc @cryptoe, I can clean this up in case we can remove the class)

Its used in SqlStatementResource#contextChecks

LakshSingla · 2023-07-11T04:40:35Z

@imply-cheddar Thanks a lot for the review, I have revised the PR with the changes, and better exception messages.

cryptoe

Left some nit's.
LGTM otherwise.

cryptoe

Changes LGTM.

…14534) MSQ engine returns correct error codes for invalid user inputs in the query context. Also, using DruidExceptions for MSQ related errors happening in the Broker with improved error messages.

…#14586) MSQ engine returns correct error codes for invalid user inputs in the query context. Also, using DruidExceptions for MSQ related errors happening in the Broker with improved error messages.

…14534) MSQ engine returns correct error codes for invalid user inputs in the query context. Also, using DruidExceptions for MSQ related errors happening in the Broker with improved error messages.

initial commit

279379b

somu-imply reviewed Jul 6, 2023

View reviewed changes

LakshSingla mentioned this pull request Jul 7, 2023

Fix ColumnSignature error message and jdk17 test issue. #14538

Merged

imply-cheddar suggested changes Jul 7, 2023

View reviewed changes

cryptoe added this to the 27.0 milestone Jul 10, 2023

cryptoe added the Area - MSQ For multi stage queries - https://github.com/apache/druid/issues/12262 label Jul 10, 2023

LakshSingla added 6 commits July 10, 2023 16:12

review

bdb3911

Merge branch 'master' into error-code-msq

eee64a5

changes

e8abc83

Merge branch 'master' into error-code-msq

4ac10c9

Merge branch 'master' into error-code-msq

0e8f2a0

cleanup

b6b9130

github-advanced-security AI found potential problems Jul 11, 2023

View reviewed changes

Comment thread extensions-core/multi-stage-query/src/main/java/org/apache/druid/msq/sql/MSQTaskSqlEngine.java Fixed

LakshSingla added 2 commits July 11, 2023 12:16

fix test

c8cb821

throw better exception when issue in the msq context

ae71725

cryptoe reviewed Jul 11, 2023

View reviewed changes

Comment thread extensions-core/multi-stage-query/src/main/java/org/apache/druid/msq/sql/MSQTaskQueryMaker.java Outdated

Comment thread ...multi-stage-query/src/main/java/org/apache/druid/msq/sql/resources/SqlStatementResource.java Outdated

LakshSingla added 3 commits July 12, 2023 16:20

tests, changes

7ec32cf

static check, test fix

6a1e653

tests fix, build green now

af62a36

cryptoe reviewed Jul 13, 2023

View reviewed changes

LakshSingla added 3 commits July 13, 2023 13:08

review

f86c1e1

Trigger Build

b9c1218

build fix

47d004e

cryptoe approved these changes Jul 13, 2023

View reviewed changes

codecov

842c961

LakshSingla merged commit c1c7dff into apache:master Jul 13, 2023

LakshSingla mentioned this pull request Jul 14, 2023

Backports "Using DruidExceptions in MSQ (changes related to the Broker" (#14534) #14586

Merged

Conversation

LakshSingla commented Jul 6, 2023

Description

Release note

Uh oh!

somu-imply left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

imply-cheddar Jul 7, 2023

Choose a reason for hiding this comment

Uh oh!

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

Uh oh!

Choose a reason for hiding this comment

Uh oh!

imply-cheddar Jul 7, 2023

Choose a reason for hiding this comment

Uh oh!

imply-cheddar Jul 7, 2023

Choose a reason for hiding this comment

Uh oh!

imply-cheddar Jul 7, 2023

Choose a reason for hiding this comment

Uh oh!

LakshSingla Jul 10, 2023

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

imply-cheddar Jul 7, 2023

Choose a reason for hiding this comment

Uh oh!

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

Uh oh!

Choose a reason for hiding this comment

Uh oh!

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

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cryptoe Jul 11, 2023

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

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

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

cryptoe left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

cryptoe left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

LakshSingla Jul 10, 2023 •

edited

Loading

cryptoe Jul 10, 2023 •

edited

Loading

LakshSingla Jul 11, 2023 •

edited

Loading

LakshSingla commented Jul 11, 2023 •

edited

Loading

cryptoe left a comment •

edited

Loading