-
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
Clean up logs for batch segment allocation #14727
Conversation
existingVersion, | ||
overallMaxId | ||
return SegmentAllocateResult.failure( | ||
"Segment[%s] in conflicting interval[%s] has higher version[%s].", |
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.
This error message is inaccurate. It's also possible that the interval corresponding to overallMaxId isn't the same as the request's interval.
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 catching this, I will break it up.
* <p> | ||
* This class does not implement equals() or hashCode() so that each request is | ||
* treated as unique even if all the parameters are the same. | ||
* TODO: why?? |
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.
Is this pending or a leftover comment?
); | ||
return null; | ||
} else if (committedMaxId != null | ||
&& committedMaxId.getShardSpec().getNumCorePartitions() | ||
== SingleDimensionShardSpec.UNKNOWN_NUM_CORE_PARTITIONS) { | ||
log.warn( |
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.
Is this log needed if the SegmentAllocationResult's failure logs the message as well?
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.
removed.
This pull request has been marked as stale due to 60 days of inactivity. |
This pull request has been marked as stale due to 60 days of inactivity. |
This pull request has been marked as stale due to 60 days of inactivity. |
Closed this PR as there are several merge conflicts now. |
Motivation
Batch segment allocation is often difficult to debug due to the actual error messages being hidden in the logs.
Changes
IndexerMetadataStorageCoordinator.allocatePendingSegments
to returnList<SegmentAllocateResult>
instead of just the allocatedSegmentIdWithShardSpec
SegmentAllocationQueue
where it is logged when abandoning a request.Old logs in case of a conflicting segment
New logs in case of a conflicting segment
This PR has: