You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@druid.apache.org by "AmatyaAvadhanula (via GitHub)" <gi...@apache.org> on 2024/05/03 05:30:50 UTC
[PR] Do not allocate ids conflicting with existing segment ids (druid)
AmatyaAvadhanula opened a new pull request, #16380:
URL: https://github.com/apache/druid/pull/16380
### Bug
Consider a situation where there are:
1) No used segments
2) Unused segments with ids ranging from 1 to 10
3) Pending segments with ids ranging from 1 to 5 due to partial clean up
Previously, new segment allocations would consider only used and pending segments and allocate a segment with id 6.
This is problematic as it conflicts with an existing unused entry in the segments table.
### Changes
After a segment has been allocated, check that it doesn't correspond to any used or unused segment id in druid_segments table.
If it does, try to look at the unused segments and increment the partitionNum to max(unusedSegment.partitionNum) + 1 if needed.
Even though this requires fetching all unused segments for a given (datasource, version, interval) when there is a conflict, the conflict would arise due to unused segments at most once and would not affect batch segment allocation's performance.
### Alternatives
Unused pending segments must be cleaned up or ignored when they are not associated with any existing active lock.
This PR has:
- [x] been self-reviewed.
- [ ] using the [concurrency checklist](https://github.com/apache/druid/blob/master/dev/code-review/concurrency.md) (Remove this item if the PR doesn't have any relation to concurrency.)
- [ ] added documentation for new or modified features or behaviors.
- [ ] a release note entry in the PR description.
- [ ] added Javadocs for most classes and all non-trivial methods. Linked related entities via Javadoc links.
- [ ] added or updated version, license, or notice information in [licenses.yaml](https://github.com/apache/druid/blob/master/dev/license.md)
- [x] added comments explaining the "why" and the intent of the code wherever would not be obvious for an unfamiliar reader.
- [x] added unit tests or modified existing tests to cover new code paths, ensuring the threshold for [code coverage](https://github.com/apache/druid/blob/master/dev/code-review/code-coverage.md) is met.
- [ ] added integration tests.
- [ ] been tested in a test Druid cluster.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
Re: [PR] Do not allocate ids conflicting with existing segment ids (druid)
Posted by "AmatyaAvadhanula (via GitHub)" <gi...@apache.org>.
AmatyaAvadhanula commented on code in PR #16380:
URL: https://github.com/apache/druid/pull/16380#discussion_r1588767430
##########
server/src/main/java/org/apache/druid/metadata/IndexerSQLMetadataStorageCoordinator.java:
##########
@@ -1013,33 +1012,6 @@ private Map<SegmentCreateRequest, SegmentIdWithShardSpec> allocatePendingSegment
return allocatedSegmentIds;
}
- @SuppressWarnings("UnstableApiUsage")
- private String getSequenceNameAndPrevIdSha(
Review Comment:
Yes
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
Re: [PR] Do not allocate ids conflicting with existing segment ids (druid)
Posted by "AmatyaAvadhanula (via GitHub)" <gi...@apache.org>.
AmatyaAvadhanula commented on code in PR #16380:
URL: https://github.com/apache/druid/pull/16380#discussion_r1588804613
##########
server/src/test/java/org/apache/druid/metadata/IndexerSQLMetadataStorageCoordinatorTest.java:
##########
@@ -3192,4 +3192,59 @@ public void testTimelineWith1CorePartitionTombstone() throws IOException
SegmentTimeline segmentTimeline = SegmentTimeline.forSegments(allUsedSegments);
Assert.assertEquals(0, segmentTimeline.lookup(interval).size());
}
+
+ @Test
+ public void testSegmentIdMustNotBeReused() throws IOException
Review Comment:
Done.
Did not rename the method however since the version is not necessarily unused. It can occur if any unused or used segment conflicts with an allocated pending segment id.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
Re: [PR] Do not allocate ids conflicting with existing segment ids (druid)
Posted by "AmatyaAvadhanula (via GitHub)" <gi...@apache.org>.
AmatyaAvadhanula commented on code in PR #16380:
URL: https://github.com/apache/druid/pull/16380#discussion_r1588805459
##########
server/src/main/java/org/apache/druid/metadata/IndexerSQLMetadataStorageCoordinator.java:
##########
@@ -1886,7 +1856,67 @@ private SegmentIdWithShardSpec createNewSegment(
committedMaxId == null ? 0 : committedMaxId.getShardSpec().getNumCorePartitions()
)
);
+ return getTrueAllocatedId(allocatedId);
+ }
+ }
+
+ private SegmentIdWithShardSpec getTrueAllocatedId(
+ SegmentIdWithShardSpec allocatedId
+ )
+ {
+ // Check if there is a conflict with an existing entry in the segments table
+ if (retrieveSegmentForId(allocatedId.asSegmentId().toString(), true) == null) {
+ return allocatedId;
+ }
+
+ // If yes, try to compute allocated partition num using the max unused segment shard spec
+ SegmentIdWithShardSpec unusedMaxId = getUnusedMaxId(
+ allocatedId.getDataSource(),
+ allocatedId.getInterval(),
+ allocatedId.getVersion()
+ );
+ // No unused segment. Just return the allocated id
+ if (unusedMaxId == null) {
+ return allocatedId;
+ }
+
+ int maxPartitionNum = Math.max(
+ allocatedId.getShardSpec().getPartitionNum(),
+ unusedMaxId.getShardSpec().getPartitionNum() + 1
Review Comment:
Similar to the description, it's possible that the user chose to drop only segments 6-10 instead of all the segments.
The conflict would have arisen in that case as well, and we could not have ignored the version.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
Re: [PR] Do not allocate ids conflicting with existing segment ids (druid)
Posted by "AmatyaAvadhanula (via GitHub)" <gi...@apache.org>.
AmatyaAvadhanula commented on PR #16380:
URL: https://github.com/apache/druid/pull/16380#issuecomment-2093046332
@kfaraz @cryptoe Thank you
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
Re: [PR] Do not allocate ids conflicting with existing segment ids (druid)
Posted by "AmatyaAvadhanula (via GitHub)" <gi...@apache.org>.
AmatyaAvadhanula merged PR #16380:
URL: https://github.com/apache/druid/pull/16380
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
Re: [PR] Do not allocate ids conflicting with existing segment ids (druid)
Posted by "kfaraz (via GitHub)" <gi...@apache.org>.
kfaraz commented on code in PR #16380:
URL: https://github.com/apache/druid/pull/16380#discussion_r1588756852
##########
server/src/main/java/org/apache/druid/metadata/IndexerSQLMetadataStorageCoordinator.java:
##########
@@ -1013,33 +1012,6 @@ private Map<SegmentCreateRequest, SegmentIdWithShardSpec> allocatePendingSegment
return allocatedSegmentIds;
}
- @SuppressWarnings("UnstableApiUsage")
- private String getSequenceNameAndPrevIdSha(
Review Comment:
unused method?
##########
server/src/test/java/org/apache/druid/metadata/IndexerSQLMetadataStorageCoordinatorTest.java:
##########
@@ -3192,4 +3192,59 @@ public void testTimelineWith1CorePartitionTombstone() throws IOException
SegmentTimeline segmentTimeline = SegmentTimeline.forSegments(allUsedSegments);
Assert.assertEquals(0, segmentTimeline.lookup(interval).size());
}
+
+ @Test
+ public void testSegmentIdMustNotBeReused() throws IOException
Review Comment:
This test should probably live in `SegmentAllocateActionTest` next to the sister test `testSegmentIsAllocatedForLatestUsedSegmentVersion`. It should also be renamed to include some more info, such as
`testSegmentIsNotAllocatedForUnusedSegmentVersion`.
##########
server/src/main/java/org/apache/druid/metadata/IndexerSQLMetadataStorageCoordinator.java:
##########
@@ -1886,7 +1856,67 @@ private SegmentIdWithShardSpec createNewSegment(
committedMaxId == null ? 0 : committedMaxId.getShardSpec().getNumCorePartitions()
)
);
+ return getTrueAllocatedId(allocatedId);
+ }
+ }
+
+ private SegmentIdWithShardSpec getTrueAllocatedId(
+ SegmentIdWithShardSpec allocatedId
+ )
+ {
+ // Check if there is a conflict with an existing entry in the segments table
+ if (retrieveSegmentForId(allocatedId.asSegmentId().toString(), true) == null) {
+ return allocatedId;
+ }
+
+ // If yes, try to compute allocated partition num using the max unused segment shard spec
+ SegmentIdWithShardSpec unusedMaxId = getUnusedMaxId(
+ allocatedId.getDataSource(),
+ allocatedId.getInterval(),
+ allocatedId.getVersion()
+ );
+ // No unused segment. Just return the allocated id
+ if (unusedMaxId == null) {
+ return allocatedId;
+ }
+
+ int maxPartitionNum = Math.max(
+ allocatedId.getShardSpec().getPartitionNum(),
+ unusedMaxId.getShardSpec().getPartitionNum() + 1
Review Comment:
Rather than doing unusedMaxId + 1, we should probably not use this version at all, if there is any existing unused segment for that version.
I think it would be better to filter out all the invalid versions:
- used but overshadowed versions (fixed in #15459 )
- unused versions
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
Re: [PR] Do not allocate ids conflicting with existing segment ids (druid)
Posted by "AmatyaAvadhanula (via GitHub)" <gi...@apache.org>.
AmatyaAvadhanula commented on code in PR #16380:
URL: https://github.com/apache/druid/pull/16380#discussion_r1588768115
##########
server/src/main/java/org/apache/druid/metadata/IndexerSQLMetadataStorageCoordinator.java:
##########
@@ -1886,7 +1856,67 @@ private SegmentIdWithShardSpec createNewSegment(
committedMaxId == null ? 0 : committedMaxId.getShardSpec().getNumCorePartitions()
)
);
+ return getTrueAllocatedId(allocatedId);
+ }
+ }
+
+ private SegmentIdWithShardSpec getTrueAllocatedId(
+ SegmentIdWithShardSpec allocatedId
+ )
+ {
+ // Check if there is a conflict with an existing entry in the segments table
+ if (retrieveSegmentForId(allocatedId.asSegmentId().toString(), true) == null) {
+ return allocatedId;
+ }
+
+ // If yes, try to compute allocated partition num using the max unused segment shard spec
+ SegmentIdWithShardSpec unusedMaxId = getUnusedMaxId(
+ allocatedId.getDataSource(),
+ allocatedId.getInterval(),
+ allocatedId.getVersion()
+ );
+ // No unused segment. Just return the allocated id
+ if (unusedMaxId == null) {
+ return allocatedId;
+ }
+
+ int maxPartitionNum = Math.max(
+ allocatedId.getShardSpec().getPartitionNum(),
+ unusedMaxId.getShardSpec().getPartitionNum() + 1
Review Comment:
Isn't it possible that someone dropped the last segment and there's a conflict because of that one segment?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org