You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@druid.apache.org by "gianm (via GitHub)" <gi...@apache.org> on 2025/07/18 23:01:46 UTC
[PR] Fix race in SegmentLoadDropHandler. (druid)
gianm opened a new pull request, #18291:
URL: https://github.com/apache/druid/pull/18291
SegmentLoadDropHandler has a race that causes dropSegmentDelayMillis to not be respected. It happens in this sequence of events:
- removeSegment is called, which adds the segment to segmentsToDelete and schedules a future drop.
- addSegment is called, which removes the segment from segmentsToDelete and proceeds to load it.
- removeSegment is called again, which adds the segment back to segmentsToDelete, before the originally-scheduled drop executes.
- The originally-scheduled drop executes, sees the segment is in segmentsToDelete, and removes it. This could happen immediately after the second call to removeSegment, defeating dropSegmentDelayMillis.
This can occur in practice when the coordinator period is less than the dropSegmentDelayMillis. By default, it's not (default is 1m Coordinator period and 30s dropSegmentDelayMillis).
This patch addresses it by ensuring that delayed drops only happen if the entry in segmentDropLatches corresponds to the removeSegment call that scheduled the drop.
--
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] Fix race in SegmentLoadDropHandler. (druid)
Posted by "kfaraz (via GitHub)" <gi...@apache.org>.
kfaraz commented on code in PR #18291:
URL: https://github.com/apache/druid/pull/18291#discussion_r2217118020
##########
server/src/main/java/org/apache/druid/server/coordination/SegmentLoadDropHandler.java:
##########
@@ -216,21 +208,30 @@ void removeSegment(
SegmentChangeStatus result = null;
try {
announcer.unannounceSegment(segment);
- segmentsToDelete.add(segment);
+
+ final SegmentDropLatch dropLatch = new SegmentDropLatch();
+ final SegmentDropLatch priorLatch = segmentDropLatches.putIfAbsent(segment, dropLatch);
+
+ if (priorLatch != null) {
+ log.warn("Cannot drop segment[%s] that already has a drop pending. Ignoring.", segment.getId());
+ return;
+ }
Runnable runnable = () -> {
try {
- synchronized (segmentDeleteLock) {
- if (segmentsToDelete.remove(segment)) {
- segmentManager.dropSegment(segment);
- }
+ if (dropLatch.startDropping()) {
+ segmentManager.dropSegment(segment);
}
}
catch (Exception e) {
log.makeAlert(e, "Failed to remove segment! Possible resource leak!")
.addData("segment", segment)
.emit();
}
+ finally {
+ dropLatch.doneDropping();
+ segmentDropLatches.remove(segment, dropLatch);
Review Comment:
Based on the changes made to `addSegment` and `removeSegment`, I think we can get away with calling `Map.remove(key)` here instead of `Map.remove(key, value)`.
--
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] Fix race in SegmentLoadDropHandler. (druid)
Posted by "cryptoe (via GitHub)" <gi...@apache.org>.
cryptoe merged PR #18291:
URL: https://github.com/apache/druid/pull/18291
--
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] Fix race in SegmentLoadDropHandler. (druid)
Posted by "kfaraz (via GitHub)" <gi...@apache.org>.
kfaraz commented on PR #18291:
URL: https://github.com/apache/druid/pull/18291#issuecomment-3097468081
> I tried, but had a hard time writing one that actually did lots of moves. I tried adding 3 historicals, but even so the system ended up deciding not to move very much. I am not entirely sure what set of conditions causes that to happen.
Ah, I see. Since we want to test a drop-add-drop sequence, probably even constantly updating load rules with fast coordinator cycles could do the trick with a single historical.
--
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] Fix race in SegmentLoadDropHandler. (druid)
Posted by "gianm (via GitHub)" <gi...@apache.org>.
gianm commented on PR #18291:
URL: https://github.com/apache/druid/pull/18291#issuecomment-3097415919
> Should we add an embedded test to verify the fix?
I tried, but had a hard time writing one that actually did lots of moves. I tried adding 3 historicals, but even so the system ended up deciding not to move very much. I am not entirely sure what set of conditions causes that to happen.
--
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