Add undocumented taskLockType to MSQ. by cryptoe · Pull Request #15168 · apache/druid

cryptoe · 2023-10-16T14:23:12Z

With #14258 going in, we want these lock types to be used by MSQ. This pr adds an undocumented parameter taskLockType to MSQ so that we can start enabling this feature for users who are interested in testing the new lock types.

This context parameter should be removed once these new lock types are battle tested.

…_lock_types

LakshSingla

Added a few passing comments on the formatting of the error messages in accordance to https://github.com/apache/druid/blob/master/dev/style-conventions.md

LakshSingla · 2023-10-16T15:45:19Z

+    } else if (taskLockType.equals(TaskLockType.SHARED)) {
+      return SegmentTransactionalInsertAction.appendAction(segments, null, null);
+    } else {
+      throw DruidException.defensive("Invalid lock type %s received for append action", taskLockType);


nit: extrapolation

Suggested change

throw DruidException.defensive("Invalid lock type %s received for append action", taskLockType);

throw DruidException.defensive("Invalid lock type [%s] received for append action", taskLockType);

LakshSingla · 2023-10-16T15:46:08Z

+    } else if (taskLockType.equals(TaskLockType.EXCLUSIVE)) {
+      return SegmentTransactionalInsertAction.overwriteAction(null, segmentsWithTombstones);
+    } else {
+      throw DruidException.defensive("Invalid lock type %s received for overwrite action", taskLockType);


Suggested change

throw DruidException.defensive("Invalid lock type %s received for overwrite action", taskLockType);

throw DruidException.defensive("Invalid lock type [%s] received for overwrite action", taskLockType);

LakshSingla · 2023-10-16T15:47:22Z

+      throw DruidException.forPersona(DruidException.Persona.USER)
+                          .ofCategory(DruidException.Category.INVALID_INPUT)
+                          .build(
+                              "TaskLock must be of type %s or %s for a replace query. Found type %s set."


Capitalization of REPLACE to be consistent with error messages across MSQ

Reworded the second sentence so that it makes sense without extrapolation as well - https://github.com/apache/druid/blob/master/dev/style-conventions.md?plain=1#L51

Suggested change

"TaskLock must be of type %s or %s for a replace query. Found type %s set."

"TaskLock must be of type %s or %s for a REPLACE query. Invalid lock type [%s] found."

LakshSingla · 2023-10-16T15:49:11Z

+      throw DruidException.forPersona(DruidException.Persona.USER)
+                          .ofCategory(DruidException.Category.INVALID_INPUT)
+                          .build(
+                              "TaskLock must be of type %s or %s for an insert query. Found type %s set."


Suggested change

"TaskLock must be of type %s or %s for an insert query. Found type %s set."

"TaskLock must be of type %s or %s for an INSERT query. Invalid lock type [%s] found."

LakshSingla · 2023-10-16T15:50:03Z

+    for (TaskLockType taskLockType : new TaskLockType[]{TaskLockType.EXCLUSIVE, TaskLockType.REPLACE}) {
+      testLockTypes(
+          taskLockType,
+          "insert into foo1 select * from foo partitioned by day",


nit: Better capitalization in the test cases.

Suggested change

"insert into foo1 select * from foo partitioned by day",

"INSERT INTO foo1 SELECT * FROM foo PARTITIONED BY DAY",

LakshSingla · 2023-10-16T15:50:54Z

+                  .putAll(DEFAULT_MSQ_CONTEXT)
+                  .put(
+                      Tasks.TASK_LOCK_TYPE,
+                      TaskLockType.REPLACE.name().toLowerCase(Locale.ENGLISH)


Suggested change

TaskLockType.REPLACE.name().toLowerCase(Locale.ENGLISH)

StringUtils.toLowerCase(TaskLockType.REPLACE.name())

kfaraz · 2023-10-16T18:08:25Z

+    } else if (taskLockType.equals(TaskLockType.SHARED)) {
+      return SegmentTransactionalInsertAction.appendAction(segments, null, null);
+    } else {
+      throw DruidException.defensive("Invalid lock type %s received for append action", taskLockType);


Don't think we really need these exceptions (esp since the methods are private) here as we would reach these methods only after the validation is already done.

I just like a more defensive approach here in case we break something above. I guess this check does not hurt functionality.

kfaraz · 2023-10-16T18:13:41Z

+      if (isReplaceQuery) {
+        return TaskLockType.EXCLUSIVE;
+      } else {
+        return TaskLockType.APPEND;


Typo: I think this should be SHARED (as indicated in the javadoc of this method too)

kfaraz

Apart from minor comments and existing style feedback from @LakshSingla , the changes look good to me.

Thanks a lot for the quick fix, @cryptoe !

cryptoe · 2023-10-17T07:53:27Z

@LakshSingla @kfaraz I have addressed the comments. PTAL soon since this is a druid 28.0 work item.

AmatyaAvadhanula · 2023-10-17T08:17:45Z

          segmentSortOrder,
          replaceTimeChunks
      );
+      MultiStageQueryContext.validateAndGetTaskLockType(sqlQueryContext,


The return value is not being utilized. Is this call made to validate the lock type?

AmatyaAvadhanula

LGTM! Thank you @cryptoe

abhishekrb19

LGTM!

abhishekrb19 · 2023-10-17T16:08:43Z

+   * {@link TaskLockType#SHARED} is used for insert queries.
+   * If the queryContext contains the taskLockType, then it is validated and returned.
+   */
+  public static TaskLockType validateAndGetTaskLockType(QueryContext queryContext, boolean isReplaceQuery)


In my mind, !isReplaceQuery means an INSERT or SELECT query. I see it's invoked only in the context of ingestion, but would also be good to clarify that in the javadoc since this class is common to all queries.

@abhishekrb19 I will fix this as part of a upcoming PR. I kind of assumed TaskLocks means something to do with ingestion but yes I can be more clear there.

Patch adds an undocumented parameter taskLockType to MSQ so that we can start enabling this feature for users who are interested in testing the new lock types.

Patch adds an undocumented parameter taskLockType to MSQ so that we can start enabling this feature for users who are interested in testing the new lock types. Co-authored-by: Karan Kumar <[email protected]>

Patch adds an undocumented parameter taskLockType to MSQ so that we can start enabling this feature for users who are interested in testing the new lock types.

Add undocumented taskLockType to MSQ.

3ed9cf9

github-actions Bot added Area - Batch Ingestion Area - Querying Area - MSQ For multi stage queries - https://github.com/apache/druid/issues/12262 labels Oct 16, 2023

cryptoe added this to the 28.0 milestone Oct 16, 2023

cryptoe requested a review from AmatyaAvadhanula October 16, 2023 14:25

Merge remote-tracking branch 'apache/master' into adding_configurable…

350a1fe

…_lock_types

LakshSingla reviewed Oct 16, 2023

View reviewed changes

kfaraz reviewed Oct 16, 2023

View reviewed changes

Comments.

c1a9ef3

cryptoe added 2 commits October 17, 2023 13:24

Comments.

40e9965

Comments.

b1c5df7

AmatyaAvadhanula reviewed Oct 17, 2023

View reviewed changes

Fixing bugs.

99c9211

github-actions Bot added the Area - Ingestion label Oct 17, 2023

AmatyaAvadhanula approved these changes Oct 17, 2023

View reviewed changes

abhishekrb19 approved these changes Oct 17, 2023

View reviewed changes

cryptoe merged commit 953ce79 into apache:master Oct 17, 2023

LakshSingla mentioned this pull request Oct 17, 2023

[Backport] Add undocumented taskLockType to MSQ. #15186

Merged

	throw DruidException.defensive("Invalid lock type %s received for append action", taskLockType);
	throw DruidException.defensive("Invalid lock type [%s] received for append action", taskLockType);

	"TaskLock must be of type %s or %s for a replace query. Found type %s set."
	"TaskLock must be of type %s or %s for a REPLACE query. Invalid lock type [%s] found."

	"insert into foo1 select * from foo partitioned by day",
	"INSERT INTO foo1 SELECT * FROM foo PARTITIONED BY DAY",

	TaskLockType.REPLACE.name().toLowerCase(Locale.ENGLISH)
	StringUtils.toLowerCase(TaskLockType.REPLACE.name())

Uh oh!

Conversation

cryptoe commented Oct 16, 2023

Uh oh!

LakshSingla left a comment

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 Oct 16, 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!

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!

Choose a reason for hiding this comment

Uh oh!

kfaraz left a comment

Choose a reason for hiding this comment

Uh oh!

cryptoe commented Oct 17, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AmatyaAvadhanula left a comment

Choose a reason for hiding this comment

Uh oh!

abhishekrb19 left a comment

Choose a reason for hiding this comment

Uh oh!

abhishekrb19 Oct 17, 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!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

LakshSingla Oct 16, 2023 •

edited

Loading

abhishekrb19 Oct 17, 2023 •

edited

Loading