xiangfu0 commented on code in PR #19468:
URL: https://github.com/apache/pinot/pull/19468#discussion_r4088439471
##########
pinot-core/src/main/java/org/apache/pinot/core/data/manager/realtime/RealtimeTableDataManager.java:
##########
@@ -542,7 +550,29 @@ private void doAddConsumingSegment(String segmentName)
return;
}
IndexLoadingConfig indexLoadingConfig = fetchIndexLoadingConfig();
- handleSegmentPreload(zkMetadata, indexLoadingConfig);
+ LLCSegmentName llcSegmentName = new LLCSegmentName(segmentName);
+ int partitionGroupId = llcSegmentName.getPartitionGroupId();
+ PartitionUpsertMetadataManager partitionUpsertMetadataManager;
+ PartitionDedupMetadataManager partitionDedupMetadataManager;
+ synchronized (_segmentDataManagerMap) {
Review Comment:
Good catch — confirmed and fixed in 30033c3.
The race was real: shutdown does not wait for in-flight consuming adds, so
an admitted callback could resume after `deleteTable()` completed and a
same-name table was recreated, then run
`FileUtils.deleteQuietly(_indexDir/segmentName)` against the new owner's
directory (same-minute LLC names restarting at partition sequence 0 collide).
Rather than draining admitted consuming callbacks before releasing the
lifecycle lock, the fix re-checks `_shutDown` under the segment-map monitor
immediately before the directory cleanup and aborts the stale add. Two reasons
for this shape:
1. `deleteTable()` holds the table lifecycle lock through the full shutdown,
so draining would stall table deletion on arbitrarily slow
preloads/construction, and it deadlocks the existing contract where shutdown
returns while an add paused mid-construction self-rejects at publish
(`testConsumingSegmentConstructedDuringShutdownIsDestroyed`).
2. The check-to-delete gap is closed by the per-segment lock: it comes from
the instance-wide `SegmentLocks` shared across table data managers, so the
recreated table's add for the colliding segment name blocks until the stale add
returns — it cannot have created files for the old callback to delete.
The regression test you asked for is in
`RealtimeTableDataManagerTest#testRecreatedTableConsumingAddSerializesBehindStaleAdd`:
the old callback pauses before the filesystem cleanup while a second manager
sharing `SegmentLocks` and the data directory adds the same segment name; it
verifies the new add blocks on the shared lock, the stale add aborts without
deleting the directory, and the recreated add completes with its files intact.
`testStaleConsumingAddAbortsBeforeDirCleanupAfterShutdown` covers the
single-manager abort. Both fail with the re-check removed.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
