Segregate advance and advanceUninterruptibly flow in postJoinCursor to allow for interrupts in advance #15222

gargvishesh · 2023-10-20T18:01:12Z

Description

Currently advance function in postJoinCursor calls advanceUninterruptibly which in turn keeps calling baseCursor.advanceUninterruptibly until the post join condition matches, without checking for interrupts. This causes the CPU to hit 100% without getting a chance for query to be cancelled.

With this change, the call flow of advance and advanceUninterruptibly is separated out so that they call baseCursor.advance and baseCursor.advanceUninterruptibly in them, respectively, giving a chance for interrupts in the former case between successive calls to baseCursor.advance.

…r by calling baseCursor.advance and baseCursor.advanceInterruptibly resp.

pranavbhole · 2023-10-20T18:12:37Z

can we add test for this scenario if possible unless it is covered already in some other test cases?

soumyava

Can we add an example in CalciteJoinQueryTest of the form that was mentioned in the issue ? Since the PostJoinCursor is used used to wrap around the Unnest Cursor can you also please check if we are unnesting a high cardinality array column or a MVD the problem mentioned in the issue does not appear anymore

soumyava · 2023-10-20T18:25:02Z

processing/src/main/java/org/apache/druid/segment/join/PostJoinCursor.java

@@ -99,15 +108,15 @@ public Filter getPostJoinFilter()
  @Override
  public void advance()
  {
-    advanceUninterruptibly();
-    BaseQuery.checkInterrupted();
+    baseCursor.advance();


Will be good to put some comments as in why this change is being done in the code for a future developer to follow.

Added comments.

soumyava · 2023-10-20T18:25:30Z

processing/src/main/java/org/apache/druid/segment/join/PostJoinCursor.java

@@ -99,15 +108,15 @@ public Filter getPostJoinFilter()
  @Override
  public void advance()
  {
-    advanceUninterruptibly();
-    BaseQuery.checkInterrupted();


Can we add a note on why the checkInterrupted() is not needed anymore

Added a comment.

soumyava · 2023-10-20T18:32:31Z

I dunno why the checkstyle is failing though. If using IntelliJ it'll be good to add this https://raw.githubusercontent.com/apache/incubator-druid/master/dev/druid_intellij_formatting.xml to IntelliJ

LakshSingla · 2023-10-20T19:16:12Z

Error:  /home/runner/work/druid/druid/processing/src/main/java/org/apache/druid/segment/join/PostJoinCursor.java:22:8: Unused import - org.apache.druid.query.BaseQuery. [UnusedImports]

My intellij doesn't clean up unused imports automatically. I run mvn checkstyle:checkstyle to confirm that checkstyle is passing. Sometimes deeply nested stuff won't get flagged up in Intellij but will show up as a check style failure.

abhishekagarwal87

Please add few test cases. LGTM otherwise.

abhishekagarwal87 · 2023-10-23T14:57:08Z

processing/src/main/java/org/apache/druid/segment/join/PostJoinCursor.java

+    }
+  }
+
+  private void advanceToMatchUninterruptibly()


Please leave a comment here that this can be a long-running CPU call. Which is why advanceUninterruptibly is not directly used in advance() call unlike other cursors. Please link the github issue in the comment.

processing/src/test/java/org/apache/druid/segment/join/PostJoinCursorTest.java

gargvishesh

@pranavbhole
I've added a test now to mimic the original issue of postJoinCursor filter condition not matching any tuples coming out of high-cardinality joins, thereby sending it into an uninterruptible, cpu-intensive and long-running call.

@soumyava
For unnest cases, the scenario (with prior implementation), though possible, is less likely due to absence of (join's) output row_count multiplication property, leading to a much faster exhaustion of input if postJoinCursor filter condition is never met. But in principle it can potentially happen with any long-running advanceUninterruptibly() call at any level -- join itself not finding any matching rows for high cardinality tables, for example. Probably maxRowsPerSegment would act as the limit in such cases.

gargvishesh · 2023-10-26T10:06:39Z

processing/src/main/java/org/apache/druid/segment/join/PostJoinCursor.java

+    }
+  }
+
+  private void advanceToMatchUninterruptibly()


gargvishesh · 2023-10-26T10:07:15Z

processing/src/main/java/org/apache/druid/segment/join/PostJoinCursor.java

@@ -99,15 +108,15 @@ public Filter getPostJoinFilter()
  @Override
  public void advance()
  {
-    advanceUninterruptibly();
-    BaseQuery.checkInterrupted();
+    baseCursor.advance();


Added comments.

gargvishesh · 2023-10-26T10:09:45Z

processing/src/main/java/org/apache/druid/segment/join/PostJoinCursor.java

@@ -99,15 +108,15 @@ public Filter getPostJoinFilter()
  @Override
  public void advance()
  {
-    advanceUninterruptibly();
-    BaseQuery.checkInterrupted();


Added a comment.

soumyava · 2023-10-27T05:04:41Z

processing/src/test/java/org/apache/druid/segment/join/PostJoinCursorTest.java

@@ -0,0 +1,230 @@
+package org.apache.druid.segment.join;


Please add the license header here

soumyava · 2023-10-27T05:07:32Z

processing/src/test/java/org/apache/druid/segment/join/PostJoinCursorTest.java

+
+    cursor.advance();
+  }
+}


I think we need to leave a blank line at the end of each file. Not sure though. If checkstyle oks it am good with it

Yeah it is needed, the checkstyle is complaining. It is good to run a mvn checkstyle:checkstyle beforehand as Laksh have suggested earlier. Another thing I do is in IntelliJ when adding a new section of code have the druid coding style and run a reformat code that fixes most of the codestyling for me

Shall follow. Thanks!

abhishekagarwal87 · 2023-10-27T05:14:50Z

processing/src/test/java/org/apache/druid/segment/join/PostJoinCursorTest.java

+      @Override
+      public boolean isDone()
+      {
+        return cursor.isDone();


shouldn't this return false? May not matter but given its infinite cursor.

abhishekagarwal87 · 2023-10-27T05:17:35Z

processing/src/test/java/org/apache/druid/segment/join/PostJoinCursorTest.java

+    catch (InterruptedException e) {
+      throw new RuntimeException(e);
+    }


nit : catch clause can be removed and exception declared on method.

abhishekagarwal87 · 2023-10-27T05:20:04Z

processing/src/test/java/org/apache/druid/segment/join/PostJoinCursorTest.java

+      sleep(5000);
+      joinCursorThread.interrupt();
+      sleep(5000);


A 10 second test is expensive. Rather, we should use a count latch or something to wait for X records to be processed. The latch wait can have 5 second timeout. Similarly, the 5 second sleep after interrupting should be removed and we can wait in a loop or use count latch again in ExceptionHandler.

I also like https://github.com/awaitility/awaitility/wiki/Usage#simple in case you want to bring that in here.

Went with a latch, but thanks for the pointer to this library.

abhishekagarwal87 · 2023-10-30T04:46:34Z

processing/src/test/java/org/apache/druid/segment/join/PostJoinCursorTest.java

+      if (exceptionHandler.getException() == null) {
+        sleep(1);
+      } else {
+        assertTrue(exceptionHandler.getException() instanceof QueryInterruptedException);


you should just return the call here and also add assertFalse at the end of the method. So we are checking that we did get QueryInterruptedException.

Makes sense. Done - thanks.

…o allow for interrupts in advance (apache#15222) Currently advance function in postJoinCursor calls advanceUninterruptibly which in turn keeps calling baseCursor.advanceUninterruptibly until the post join condition matches, without checking for interrupts. This causes the CPU to hit 100% without getting a chance for query to be cancelled. With this change, the call flow of advance and advanceUninterruptibly is separated out so that they call baseCursor.advance and baseCursor.advanceUninterruptibly in them, respectively, giving a chance for interrupts in the former case between successive calls to baseCursor.advance.

…o allow for interrupts in advance (#15222) (#15278) Currently advance function in postJoinCursor calls advanceUninterruptibly which in turn keeps calling baseCursor.advanceUninterruptibly until the post join condition matches, without checking for interrupts. This causes the CPU to hit 100% without getting a chance for query to be cancelled. With this change, the call flow of advance and advanceUninterruptibly is separated out so that they call baseCursor.advance and baseCursor.advanceUninterruptibly in them, respectively, giving a chance for interrupts in the former case between successive calls to baseCursor.advance. Co-authored-by: Vishesh Garg <[email protected]>

…o allow for interrupts in advance (apache#15222) Currently advance function in postJoinCursor calls advanceUninterruptibly which in turn keeps calling baseCursor.advanceUninterruptibly until the post join condition matches, without checking for interrupts. This causes the CPU to hit 100% without getting a chance for query to be cancelled. With this change, the call flow of advance and advanceUninterruptibly is separated out so that they call baseCursor.advance and baseCursor.advanceUninterruptibly in them, respectively, giving a chance for interrupts in the former case between successive calls to baseCursor.advance.

Segregate advance and advanceInterruptibly call flow in postJoinCurso…

4d22ab3

…r by calling baseCursor.advance and baseCursor.advanceInterruptibly resp.

github-actions bot added the Area - Segment Format and Ser/De label Oct 20, 2023

soumyava reviewed Oct 20, 2023

View reviewed changes

abhishekagarwal87 reviewed Oct 23, 2023

View reviewed changes

gargvishesh added 3 commits October 25, 2023 23:36

Add comments

c91dc76

Add valueMatcher setter for testing

8be0109

Add tests for advance with interruption for post join cursor.

0e2f01d

github-advanced-security bot found potential problems Oct 26, 2023

View reviewed changes

processing/src/test/java/org/apache/druid/segment/join/PostJoinCursorTest.java Fixed Show fixed Hide fixed

gargvishesh commented Oct 26, 2023

View reviewed changes

soumyava reviewed Oct 27, 2023

View reviewed changes

Add apache licence header to the test file.

33e5483

soumyava reviewed Oct 27, 2023

View reviewed changes

abhishekagarwal87 reviewed Oct 27, 2023

View reviewed changes

Address review comments

5f1d7f4

abhishekagarwal87 reviewed Oct 30, 2023

View reviewed changes

abhishekagarwal87 added the Bug label Oct 30, 2023

gargvishesh added 3 commits October 30, 2023 10:25

Fail test for no exception case

6fea6df

Always return false for TestCursor isDone.

3fd3f5c

Fix unused import

cd7078a

abhishekagarwal87 approved these changes Oct 30, 2023

View reviewed changes

abhishekagarwal87 added this to the 28.0 milestone Oct 30, 2023

gargvishesh changed the title ~~Segregate advance and advanceUninterruptibly flow in postJoinCursor~~ Segregate advance and advanceUninterruptibly flow in postJoinCursor to allow for interrupts in advance Oct 30, 2023

abhishekagarwal87 merged commit a27598a into apache:master Oct 30, 2023
82 checks passed

LakshSingla mentioned this pull request Oct 30, 2023

[Backport] Segregate advance and advanceUninterruptibly flow in postJoinCursor to allow for interrupts in advance #15278

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Segregate advance and advanceUninterruptibly flow in postJoinCursor to allow for interrupts in advance #15222

Segregate advance and advanceUninterruptibly flow in postJoinCursor to allow for interrupts in advance #15222

gargvishesh commented Oct 20, 2023 •

edited

Loading

pranavbhole commented Oct 20, 2023 •

edited

Loading

soumyava left a comment

soumyava Oct 20, 2023

gargvishesh Oct 26, 2023

soumyava Oct 20, 2023

gargvishesh Oct 26, 2023

soumyava commented Oct 20, 2023

LakshSingla commented Oct 20, 2023

abhishekagarwal87 left a comment

abhishekagarwal87 Oct 23, 2023

gargvishesh Oct 26, 2023

gargvishesh left a comment

gargvishesh Oct 26, 2023

gargvishesh Oct 26, 2023

gargvishesh Oct 26, 2023

soumyava Oct 27, 2023

soumyava Oct 27, 2023 •

edited

Loading

soumyava Oct 27, 2023 •

edited

Loading

gargvishesh Oct 28, 2023

abhishekagarwal87 Oct 27, 2023

gargvishesh Oct 28, 2023

abhishekagarwal87 Oct 27, 2023

gargvishesh Oct 28, 2023

abhishekagarwal87 Oct 27, 2023

gargvishesh Oct 28, 2023

abhishekagarwal87 Oct 30, 2023

gargvishesh Oct 30, 2023

Segregate advance and advanceUninterruptibly flow in postJoinCursor to allow for interrupts in advance #15222

Segregate advance and advanceUninterruptibly flow in postJoinCursor to allow for interrupts in advance #15222

Conversation

gargvishesh commented Oct 20, 2023 • edited Loading

Description

pranavbhole commented Oct 20, 2023 • edited Loading

soumyava left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

soumyava commented Oct 20, 2023

LakshSingla commented Oct 20, 2023

abhishekagarwal87 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gargvishesh left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

soumyava Oct 27, 2023 • edited Loading

Choose a reason for hiding this comment

soumyava Oct 27, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gargvishesh commented Oct 20, 2023 •

edited

Loading

pranavbhole commented Oct 20, 2023 •

edited

Loading

soumyava Oct 27, 2023 •

edited

Loading

soumyava Oct 27, 2023 •

edited

Loading