Optimize unused segment query for segment allocation#16623
Merged
cryptoe merged 4 commits intoapache:masterfrom Jun 18, 2024
Merged
Optimize unused segment query for segment allocation#16623cryptoe merged 4 commits intoapache:masterfrom
cryptoe merged 4 commits intoapache:masterfrom
Conversation
kfaraz
reviewed
Jun 18, 2024
kfaraz
reviewed
Jun 18, 2024
kfaraz
reviewed
Jun 18, 2024
kfaraz
approved these changes
Jun 18, 2024
Contributor
kfaraz
left a comment
kfaraz
left a comment
There was a problem hiding this comment.
Changes look good, left a few non-blocking suggestions.
Comment on lines
+285
to
+286
| log.debug("Found [%,d] unused segments for datasource[%s] for interval[%s] and version[%s].", | ||
| matchingSegments.size(), dataSource, interval, version); |
Contributor
There was a problem hiding this comment.
Nit:
Suggested change
| log.debug("Found [%,d] unused segments for datasource[%s] for interval[%s] and version[%s].", | |
| matchingSegments.size(), dataSource, interval, version); | |
| log.debug( | |
| "Found [%,d] unused segments for datasource[%s], interval[%s] and version[%s].", | |
| matchingSegments.size(), dataSource, interval, version | |
| ); |
| new NumberedShardSpec(0, 0) | ||
| ); | ||
| DataSegment unusedSegmentForDifferentInterval = createSegment( | ||
| Intervals.of("2023/2024"), |
Contributor
There was a problem hiding this comment.
Rather than a disjoint interval, a better test would be to verify that a segment in an overlapping (but not identical) interval is not returned.
| @Test | ||
| public void testRetrieveUnusedSegmentsForExactIntervalAndVersion() throws Exception | ||
| { | ||
| DataSegment unusedForDifferentVersion = createSegment( |
Contributor
There was a problem hiding this comment.
Suggested change
| DataSegment unusedForDifferentVersion = createSegment( | |
| final DataSegment unusedSegmentMay2024V0 = createSegment( |
| public void testRetrieveUnusedSegmentsForExactIntervalAndVersion() throws Exception | ||
| { | ||
| DataSegment unusedForDifferentVersion = createSegment( | ||
| Intervals.of("2024/2025"), |
Contributor
There was a problem hiding this comment.
Nit: use an interval which is easier to use in a name. You may even assign this interval value to a field named Interval may2024 so that you can reuse it in multiple places.
Suggested change
| Intervals.of("2024/2025"), | |
| Intervals.of("2024-05/P1M"), |
| "v0", | ||
| new NumberedShardSpec(0, 0) | ||
| ); | ||
| DataSegment unusedSegmentForExactIntervalAndVersion = createSegment( |
Contributor
There was a problem hiding this comment.
Suggested change
| DataSegment unusedSegmentForExactIntervalAndVersion = createSegment( | |
| final DataSegment unusedSegmentMay2024V1 = createSegment( |
| new NumberedShardSpec(0, 0) | ||
| ); | ||
| DataSegment unusedSegmentForExactIntervalAndVersion = createSegment( | ||
| Intervals.of("2024/2025"), |
Contributor
There was a problem hiding this comment.
Suggested change
| Intervals.of("2024/2025"), | |
| Intervals.of("2024-05/P1M"), |
| "v1", | ||
| new NumberedShardSpec(0, 0) | ||
| ); | ||
| DataSegment unusedSegmentForDifferentInterval = createSegment( |
Contributor
There was a problem hiding this comment.
Suggested change
| DataSegment unusedSegmentForDifferentInterval = createSegment( | |
| final DataSegment unusedSegmentYear2024V1 = createSegment( |
| new NumberedShardSpec(0, 0) | ||
| ); | ||
| DataSegment unusedSegmentForDifferentInterval = createSegment( | ||
| Intervals.of("2023/2024"), |
Contributor
There was a problem hiding this comment.
Suggested change
| Intervals.of("2023/2024"), | |
| Intervals.of("2024/P1Y"), |
| ); | ||
| coordinator.markSegmentsAsUnusedWithinInterval(DS.WIKI, Intervals.ETERNITY); | ||
|
|
||
| DataSegment usedSegmentForExactIntervalAndVersion = createSegment( |
Contributor
There was a problem hiding this comment.
Suggested change
| DataSegment usedSegmentForExactIntervalAndVersion = createSegment( | |
| final DataSegment usedSegmentMay2024V1 = createSegment( |
| coordinator.markSegmentsAsUnusedWithinInterval(DS.WIKI, Intervals.ETERNITY); | ||
|
|
||
| DataSegment usedSegmentForExactIntervalAndVersion = createSegment( | ||
| Intervals.of("2024/2025"), |
Contributor
There was a problem hiding this comment.
Suggested change
| Intervals.of("2024/2025"), | |
| Intervals.of("2024-05/P1M"), |
cryptoe
approved these changes
Jun 18, 2024
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.
#16380 utilized an existing metadata query to fetch unused segments for a given datasource, interval and version but this appeared to take a long time despite the indexes, and could have potential overlord stability implications.
This PR optimizes the query by using an equality check on the interval start and end as it is a special case for segment allocation, instead of using the OVERLAPS or CONTAINS match modes.
On a cluster with 1.8M unused segments for a given datasource, the query which relied on the existing method took over 30s on average, while the new query takes less than a second.
This PR has: