Jackie-Jiang commented on code in PR #19400:
URL: https://github.com/apache/pinot/pull/19400#discussion_r4066977747
##########
pinot-core/src/main/java/org/apache/pinot/core/data/manager/realtime/PinotFSSegmentUploader.java:
##########
@@ -71,14 +79,12 @@ public URI uploadSegment(File segmentFile, LLCSegmentName
segmentName, int timeo
final String rawTableName =
TableNameBuilder.extractRawTableName(segmentName.getTableName());
Callable<URI> uploadTask = () -> {
URI destUri = new URI(StringUtil.join(File.separator,
_segmentStoreUriStr, segmentName.getTableName(),
-
SegmentCompletionUtils.generateTmpSegmentFileName(segmentName.getSegmentName())));
+
SegmentCompletionUtils.generateTmpSegmentFileName(segmentName.getSegmentName(),
_instanceId)));
Review Comment:
[P1] Reusing this destination allows a timed-out upload to invalidate a
successful retry. `future.get()` times out without stopping the worker. In
`LocalPinotFS.copy()`, both attempts now also share the internal `dest +
".tmp"` path: T1 can finish copying and stall before publication; T2 copies and
renames that intermediate to `dest`, then returns success; T1 resumes, renames
T2's destination to a backup, and fails because T2 already moved the
intermediate. The controller then cannot find the URI returned by the
successful retry. Removing the explicit delete does not prevent the
filesystem's own destructive rename. The controller-mediated upload path has
the same collision. Please preserve unique attempt destinations or guarantee
that outstanding writers cannot touch a successfully published upload, and add
a regression test with overlapping timeout/retry attempts.
##########
pinot-core/src/main/java/org/apache/pinot/core/data/manager/realtime/SegmentCompletionUtils.java:
##########
@@ -30,27 +34,39 @@ private SegmentCompletionUtils() {
private static final String TMP = ".tmp.";
/// Takes in a segment name, and returns a file name prefix that is used to
store all attempted uploads of this
- /// segment when a segment is uploaded using split commit. Each attempt has
a unique file name suffix
+ /// segment when a segment is uploaded using split commit.
/// @param segmentName segment name
- /// @return
+ /// @return temporary segment file name prefix
public static String getTmpSegmentNamePrefix(String segmentName) {
return segmentName + TMP;
}
+ /// Mints a leftover-style UUID temp name. Prefer {@link
#generateTmpSegmentFileName(String, String)} for new uploads.
public static String generateTmpSegmentFileName(String segmentNameStr) {
- return getTmpSegmentNamePrefix(segmentNameStr) + UUID.randomUUID();
+ return generateTmpSegmentFileName(segmentNameStr,
UUID.randomUUID().toString());
+ }
+
+ /// Returns `{segment}.tmp.{instanceId}` so retries from one server reuse a
single deep-store key.
+ public static String generateTmpSegmentFileName(String segmentNameStr,
String instanceId) {
+ if (StringUtils.isBlank(segmentNameStr)) {
+ throw new IllegalArgumentException("segmentName is required");
+ }
+ if (!isPathSafeTmpSuffix(instanceId)) {
+ throw new IllegalArgumentException("instanceId must be a non-empty
path-safe identifier: " + instanceId);
+ }
+ return getTmpSegmentNamePrefix(segmentNameStr) + instanceId;
}
public static boolean isTmpFile(String uri) {
String[] splits = StringUtils.splitByWholeSeparator(uri, TMP);
if (splits.length < 2) {
return false;
}
- try {
- UUID.fromString(splits[splits.length - 1]);
- return true;
- } catch (IllegalArgumentException e) {
- return false;
- }
+ // Accept leftover UUID temps and {segment}.tmp.{instanceId}. Reject empty
or path-like suffixes.
+ return isPathSafeTmpSuffix(splits[splits.length - 1]);
Review Comment:
[P1] Path safety does not establish that a file is temporary. For a hybrid
table `events`, a valid permanent OFFLINE segment named `batch.tmp.20260918` is
stored at `dataDir/events/batch.tmp.20260918` and now matches this predicate.
Realtime validation passes only `events_REALTIME` metadata to
`deleteTmpSegments()`, which scans the shared raw-table directory and protects
only those realtime download URLs. With
`controller.realtime.segment.tmpFileAsyncDeletionEnabled=true`, the live
offline file is therefore deleted after the temporary-file retention expires.
The previous UUID check rejected this name. Please retain strict temporary-file
recognition, or use an unambiguous temporary namespace, and ensure cleanup
protects permanent files belonging to both table types. Add a hybrid-table
regression test for this case.
--
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]
