-
Notifications
You must be signed in to change notification settings - Fork 3.7k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Allow cancellation of MSQ tasks if they are waiting for segments to load #15000
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for taking this up! Looks good apart from some minor comments
@@ -90,11 +94,15 @@ public class SegmentLoadWaiter | |||
private final Set<String> versionsToAwait; | |||
private final int totalSegmentsGenerated; | |||
private final boolean doWait; | |||
// since live reports fetch the value in another thread, we need to use AtomicReference | |||
private final AtomicReference<SegmentLoadWaiterStatus> status; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we are renaming the class, the status class might need to be renamed too
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not really The status is an internal class class so I think we can let it remain as it is.
...ns-core/multi-stage-query/src/test/java/org/apache/druid/msq/exec/SegmentLoadWaiterTest.java
Outdated
Show resolved
Hide resolved
...ns-core/multi-stage-query/src/test/java/org/apache/druid/msq/exec/SegmentLoadWaiterTest.java
Outdated
Show resolved
Hide resolved
...ns-core/multi-stage-query/src/test/java/org/apache/druid/msq/exec/SegmentLoadWaiterTest.java
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changes LGTM!
With PR #14322 , MSQ insert/Replace q's will wait for segment to be loaded on the historical's before finishing.
The patch introduces a bug where in the main thread had a thread.sleep() which could not be interuptted via the cancel calls from the overlord.
This new patch addressed that problem by moving the thread.sleep inside a thread of its own. Thus the main thread is now waiting on the future object of this execution.
The cancel call can now shutdown the executor service via another method thus unblocking the main thread to proceed.
This PR has: