Jackie-Jiang commented on code in PR #19776:
URL: https://github.com/apache/pinot/pull/19776#discussion_r4264036829
##########
pinot-controller/src/main/java/org/apache/pinot/controller/helix/core/SegmentDeletionManager.java:
##########
@@ -369,6 +370,33 @@ private void moveSegmentsToDeletedDir(String segmentId,
Long deletedSegmentsRete
LOGGER.warn("Could not move segment {} from {} to {}", segmentId,
fileToDeleteURI.toString(),
deletedSegmentMoveDestURI.toString(), e);
}
+ // Independent of whether the segment object existed, so no orphan sidecar
is left in the live directory.
+ moveSegmentMetadataToDeletedDir(pinotFS, fileToDeleteURI, segmentId,
rawTableName, retentionSuffix);
+ }
+
+ private void moveSegmentMetadataToDeletedDir(PinotFS pinotFS, URI
segmentFileUri, String segmentId,
+ String rawTableName, String retentionSuffix) {
+ // Live sidecar is <segment>.metadata.tar.gz; in Deleted_Segments it is
renamed to <segment>.metadata<suffix> so
+ // that, like the segment object, it carries no .tar.gz extension and the
retention suffix stays last.
+ try {
+ URI segmentMetadataUri =
SegmentPushUtils.generateSegmentMetadataURI(segmentFileUri.toString(),
segmentId);
+ if (!pinotFS.exists(segmentMetadataUri)) {
+ return;
+ }
+ URI deletedMetadataUri = URIUtils.getUri(_dataDir, DELETED_SEGMENTS,
rawTableName,
+ URIUtils.encode(segmentId) + DELETED_METADATA_FILE_SUFFIX +
retentionSuffix);
+ if (pinotFS.move(segmentMetadataUri, deletedMetadataUri, true)) {
Review Comment:
[P1] Keep retained sidecars out of the segment filename namespace
With conventional extensionless OFFLINE objects, the full segments `foo` and
`foo.metadata` can coexist with `foo`'s sidecar `foo.metadata.tar.gz`. This new
move renames that sidecar to `foo.metadata<retentionSuffix>`, exactly the
archive destination of the full segment `foo.metadata`. Deleting `foo.metadata`
before `foo` therefore overwrites the retained full segment with metadata-only
content when the suffixes match (including default positive retention with no
suffix). Before this PR, the sidecar was deleted and these two archived segment
files remained distinct. Similar naming risks exist for other live-file
layouts, but this particular archive collision is introduced here. Please use a
separate metadata namespace and cover both deletion orders.
##########
pinot-controller/src/main/java/org/apache/pinot/controller/helix/core/realtime/PinotLLCRealtimeSegmentManager.java:
##########
@@ -622,6 +628,31 @@ private boolean isPeerUrl(String segmentLocation) {
CommonConstants.Segment.PEER_SEGMENT_DOWNLOAD_SCHEME.length());
}
+ /// Moves the metadata sidecar the server uploaded under a tmp name to
`<segment>.metadata.tar.gz`, like
+ /// [#moveSegmentFile] does for the segment file. Best effort: a failure is
logged and counted, and never fails the
+ /// commit.
+ private void moveMetadataTarFile(String realtimeTableName, String
rawTableName, String segmentName,
+ @Nullable String metadataTarLocation, PinotFS pinotFS) {
+ if (metadataTarLocation == null) {
+ return;
Review Comment:
[P1] Invalidate the previous sidecar when replacing a segment
A commit can publish its segment and sidecar, then fail the ZK metadata
commit before DONE. The aborted FSM is removed, allowing another replica to win
a retry and overwrite the full segment. If that replica's metadata upload
fails, or its server does not enable/support the feature, this null return
preserves the first replica's final sidecar. A failed sidecar move has the same
problem. Replica builds can have different CRCs, and
`SegmentPushUtils.getSegmentMetadataFile()` trusts an existing sidecar without
comparing it with the full segment, so a later METADATA push/recovery can use
metadata from the wrong build. Please ensure publishing a replacement segment
replaces or invalidates the previous sidecar, and test a failed first commit
followed by a different build with missing/failed sidecar upload.
##########
pinot-controller/src/main/java/org/apache/pinot/controller/helix/core/realtime/PinotLLCRealtimeSegmentManager.java:
##########
@@ -622,6 +628,31 @@ private boolean isPeerUrl(String segmentLocation) {
CommonConstants.Segment.PEER_SEGMENT_DOWNLOAD_SCHEME.length());
}
+ /// Moves the metadata sidecar the server uploaded under a tmp name to
`<segment>.metadata.tar.gz`, like
+ /// [#moveSegmentFile] does for the segment file. Best effort: a failure is
logged and counted, and never fails the
+ /// commit.
+ private void moveMetadataTarFile(String realtimeTableName, String
rawTableName, String segmentName,
+ @Nullable String metadataTarLocation, PinotFS pinotFS) {
+ if (metadataTarLocation == null) {
+ return;
+ }
+ URI uriToMoveTo = createSegmentPath(rawTableName, segmentName +
METADATA_TAR_GZ_FILE_EXT);
+ try {
+ if (pinotFS.move(URIUtils.getUri(metadataTarLocation), uriToMoveTo,
true)) {
Review Comment:
[P2] Protect the new sidecars from untracked-segment cleanup
When `controller.retentionManager.untrackedSegmentDeletionEnabled=true`,
`RetentionManager.extractSegmentName()` strips only `.tar.gz`, so this file is
interpreted as segment `<segment>.metadata`. That name is absent from ZK even
while `<segment>` is registered. Once the untracked retention period elapses
(three days by default), cleanup locates the sidecar through the segment
`.tar.gz` fallback and moves/deletes it independently of the active segment.
This is an existing OFFLINE weakness that the new REALTIME producer also
encounters, undermining preservation until the owning segment expires. Please
recognize sidecars explicitly and associate them with their owning segment
during cleanup. Add OFFLINE/REALTIME coverage showing an aged sidecar survives
while its segment remains registered, with orphan cleanup still handled.
--
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]
