-
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
Fix used segment retrieval in Kill tasks #15306
Fix used segment retrieval in Kill tasks #15306
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 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.
@@ -196,21 +195,40 @@ public TaskStatus runTask(TaskToolbox toolbox) throws Exception | |||
limit, | |||
numTotalBatches != null ? StringUtils.format(" in [%d] batches.", numTotalBatches) : "." | |||
); | |||
|
|||
RetrieveUsedSegmentsAction retrieveUsedSegmentsAction = new RetrieveUsedSegmentsAction( |
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.
nit: could be final
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 |
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.
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
markAsUnused
flag, so there's no guarantee that the segments will be marked as unused in the kill tasks' interval ifmarkAsUnused=false
// 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 |
@abhishekrb19 I'll address your feedback in a follow-up PR. |
Fix used segment retrieval in Kill tasks
Fix used segment retrieval in Kill tasks
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-revoked
locks 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: