Fix used segment retrieval in Kill tasks#15306
Merged
AmatyaAvadhanula merged 1 commit intoapache:masterfrom Nov 2, 2023
Merged
Fix used segment retrieval in Kill tasks#15306AmatyaAvadhanula merged 1 commit intoapache:masterfrom
AmatyaAvadhanula merged 1 commit intoapache:masterfrom
Conversation
abhishekrb19
approved these changes
Nov 2, 2023
Contributor
abhishekrb19
left a comment
abhishekrb19
left a comment
There was a problem hiding this comment.
Thanks for the fix! If it's not too much effort, it'd also be nice to have a test that would otherwise fail without this patch.
| numTotalBatches != null ? StringUtils.format(" in [%d] batches.", numTotalBatches) : "." | ||
| ); | ||
|
|
||
| RetrieveUsedSegmentsAction retrieveUsedSegmentsAction = new RetrieveUsedSegmentsAction( |
Contributor
There was a problem hiding this comment.
nit: could be final
Suggested change
| RetrieveUsedSegmentsAction retrieveUsedSegmentsAction = new RetrieveUsedSegmentsAction( | |
| final RetrieveUsedSegmentsAction retrieveUsedSegmentsAction = new RetrieveUsedSegmentsAction( |
| ImmutableList.of(getInterval()), | ||
| Segments.INCLUDING_OVERSHADOWED | ||
| ); | ||
| // Fetch the load specs of all segments overlapping with the unused segment intervals |
Contributor
There was a problem hiding this comment.
Correct me if I'm wrong:
- The interval check is only done below from L246-249, so maybe this comment here is a bit confusing?
- Segments may/may not be marked as unused here depending on the deprecated
markAsUnusedflag, so there's no guarantee that the segments will be marked as unused in the kill tasks' interval ifmarkAsUnused=false
Suggested change
| // Fetch the load specs of all segments overlapping with the unused segment intervals | |
| // Fetch the load specs of all used segments in the kill task's interval |
10 tasks
abhishekagarwal87
approved these changes
Nov 2, 2023
Contributor
Author
|
@abhishekrb19 I'll address your feedback in a follow-up PR. |
AmatyaAvadhanula
added a commit
to AmatyaAvadhanula/druid
that referenced
this pull request
Nov 2, 2023
Fix used segment retrieval in Kill tasks
AmatyaAvadhanula
added a commit
that referenced
this pull request
Nov 2, 2023
CaseyPan
pushed a commit
to CaseyPan/druid
that referenced
this pull request
Nov 17, 2023
Fix used segment retrieval in Kill tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes a bug introduced because of #15107.
If the batch size is large and there are more than 120 intervals in the used segment fetch query, it may fail with the following error:
java.sql.SQLSyntaxErrorException: Statement too complex.Since a KillTask operates with an EXCLUSIVE lock, the set of used segments cannot change in between the task unless there is a revokal.
Changes in this PR:
non-revokedlocks of the task before each batch to ensure that none of them were revoked in between batch processing. This ensures that we kill only those segments which are currently locked by the kill task.This PR has: