xiangfu0 commented on code in PR #19776:
URL: https://github.com/apache/pinot/pull/19776#discussion_r4464754091
##########
pinot-controller/src/main/java/org/apache/pinot/controller/api/upload/ZKOperator.java:
##########
@@ -678,4 +682,22 @@ private void copyFromSegmentURIToDeepStore(URI
sourceDownloadURI, URI finalSegme
PinotFSFactory.create(finalSegmentLocationURI.getScheme()).copy(sourceDownloadURI,
finalSegmentLocationURI);
}
}
+
+ /// A refresh replaces the full segment, so a sidecar left by the previous
build would describe the wrong bytes (a
+ /// metadata push trusts an existing sidecar). Not needed when a METADATA
push already points at the final location:
+ /// nothing is copied and the sidecar there belongs to that push. Failing to
invalidate fails the refresh.
+ private void invalidateStaleMetadataTar(String segmentName, FileUploadType
uploadType, String sourceDownloadURIStr,
+ URI finalSegmentLocationURI)
+ throws Exception {
+ if (uploadType == FileUploadType.METADATA && new
URI(sourceDownloadURIStr).equals(finalSegmentLocationURI)) {
Review Comment:
**MINOR: this skip guard effectively never matches.** `sourceDownloadURIStr`
is the batch output (`…/<seg>.tar.gz`), while `finalSegmentLocationURI` is
`<dataDir>/<table>/<encoded seg>` with no extension. In the case the comment
describes (the job writes into the controller table dir with
`createMetadataTarGz`), the job's fresh sidecar is at
`generateSegmentMetadataURI(<seg>.tar.gz, seg)`, which is
`<dataDir>/<table>/<seg>.metadata.tar.gz`. That is exactly the path deleted
below, so a METADATA refresh with copy-to-deep-store deletes the sidecar that
the push just generated. It fails safe (callers fall back to the full segment),
but the sidecar is lost.
If the intent is to keep the push's own sidecar, compare against
`SegmentPushUtils.generateSegmentMetadataURI(sourceDownloadURIStr,
segmentName)`. Otherwise, drop the guard and its comment. Also note that this
check now runs for every OFFLINE refresh regardless of the new flags, and a
failed delete fails the refresh.
##########
pinot-controller/src/main/java/org/apache/pinot/controller/helix/core/SegmentDeletionManager.java:
##########
@@ -77,29 +78,36 @@ public class SegmentDeletionManager {
// file will be permanently deleted after Feb 2nd 2022 12PM.
public static final String DELETED_SEGMENTS = "Deleted_Segments";
private static final String RETENTION_UNTIL_SEPARATOR =
"__RETENTION_UNTIL__";
- private static final String RETENTION_DATE_FORMAT_STR = "yyyyMMddHHmm";
- private static final SimpleDateFormat RETENTION_DATE_FORMAT;
+ // Subdirectory of Deleted_Segments/<table>/ holding retained metadata
sidecars, named like the archived segment.
+ // '~' is never produced by URIUtils.encode (URLEncoder), so no archived
segment name can equal this directory name.
+ public static final String DELETED_SEGMENT_METADATA_DIR = "~metadata";
Review Comment:
**MINOR: the invariant in this comment doesn't hold on disk.**
`URIUtils.encode("~metadata")` is `%7Emetadata`, but PinotFS implementations
decode the path (`LocalPinotFS.toFile` URL-decodes). `~` is a legal
segment-name character, since `SegmentNameUtils` only rejects `\/:*?"<>|`. So a
segment named `~metadata` that is deleted with null (default) retention is
archived to `Deleted_Segments/<table>/~metadata`, and `move(..., true)`
replaces this directory.
I reproduced it at `e2581e5` with LocalPinotFS and `preserveMetadata=true`.
After archiving `seg1` with 7d retention, the table has
`~metadata/seg1__RETENTION_UNTIL__…`. After deleting segment `~metadata` with
null retention, `~metadata` is a regular file holding that segment, and
`seg1`'s retained sidecar is gone. This is the same failure mode as the earlier
`metadata` case, now much narrower. Segment data is unaffected.
A small guard would close it, e.g. always add a retention suffix (or refuse
the move) when `segmentId` equals `DELETED_SEGMENT_METADATA_DIR`. Please also
fix this comment and add a test.
--
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]
