Fix concurrent append to interval with unused segments#18230
Merged
kfaraz merged 4 commits intoapache:masterfrom Jul 11, 2025
Merged
Fix concurrent append to interval with unused segments#18230kfaraz merged 4 commits intoapache:masterfrom
kfaraz merged 4 commits intoapache:masterfrom
Conversation
imply-cheddar
approved these changes
Jul 10, 2025
Contributor
imply-cheddar
left a comment
imply-cheddar
left a comment
There was a problem hiding this comment.
A few comments. Nothing to block merge over, so approving anyway.
| } | ||
|
|
||
| if (foundFreshVersion) { | ||
| return new SegmentIdWithShardSpec( |
Contributor
There was a problem hiding this comment.
This is probably such a seldom-done thing that an INFO log indicating that a new version was found and what it is shouldn't flood the logs, but also should provide decent context on what's going on in cases where this ends up getting called/run unexpectedly.
Comment on lines
+1145
to
+1147
| // Verify that the new segment gets a different version | ||
| Assert.assertEquals(SEGMENT_V0 + "S", pendingSegment2.getVersion()); | ||
| Assert.assertEquals(0, pendingSegment2.getShardSpec().getPartitionNum()); |
Contributor
There was a problem hiding this comment.
It would probably also be good to validate the "keeps appending" behavior in the tests as well (i.e. if one is created, marked unused, a new one created, marked unused, created again, marked unused, etc. it should just build mroe and more 'S's)
capistrant
pushed a commit
to capistrant/incubator-druid
that referenced
this pull request
Jul 17, 2025
This is a better approach to the fix in apache#18216 Changes: - When allocating the first segment in an interval which already contains an unused segment, use a fresh version rather than reusing the old version (now unused)
capistrant
added a commit
that referenced
this pull request
Jul 17, 2025
This is a better approach to the fix in #18216 Changes: - When allocating the first segment in an interval which already contains an unused segment, use a fresh version rather than reusing the old version (now unused) Co-authored-by: Kashif Faraz <kashif.faraz@gmail.com>
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
4 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.
Description
This is a better fix for #18216 as suggested by @imply-cheddar in the PR comments.
Note on the original fix
The case of version collision during segment allocation handled in the original PR can happen only if the following conditions are met:
appendToExisting: true)Despite this fact, appending to an already existing version as a side effect is not desirable and can lead to data consistency issues when this version is marked used / unused.
Changes
This PR has: