hudi-agent commented on code in PR #18611:
URL: https://github.com/apache/hudi/pull/18611#discussion_r3580658578
##########
hudi-client/hudi-client-common/src/main/java/org/apache/hudi/metadata/HoodieBackedTableMetadataWriter.java:
##########
@@ -1185,7 +1193,7 @@ public void update(HoodieRestoreMetadata restoreMetadata,
String instantTime) {
// Restore requires the existing pipelines to be shutdown. So we can
safely scan the dataset to find the current
// list of files in the filesystem.
- List<DirectoryInfo> dirInfoList =
listAllPartitionsFromFilesystem(instantTime, Collections.emptySet());
+ List<DirectoryInfo> dirInfoList =
listAllPartitionsFromFilesystem(instantTime, Collections.emptySet(), false);
Review Comment:
🤖 Passing `false` here correctly avoids spuriously *deleting* tracked files
(per the thread above), but there's a reverse effect worth considering: if MDT
was initialized with skip enabled, zero-size files still on disk (e.g. orphans)
aren't in MDT — yet this scan picks them up and
`fetchOutofSyncFilesRecordsFromMetadataTable` (lines 1749-1752) re-adds them to
the FILES partition. So a restore would silently re-introduce the very files
the config was meant to exclude. Is that intended? @nsivabalan
<sub><i>⚠️ AI-generated; verify before applying. React 👍/👎 to flag
quality.</i></sub>
##########
hudi-common/src/main/java/org/apache/hudi/metadata/model/DirectoryInfo.java:
##########
@@ -89,7 +97,12 @@ public DirectoryInfo(String relativePath,
List<StoragePathInfo> pathInfos, Strin
String dataFileCommitTime =
FSUtils.getCommitTime(pathInfo.getPath().getName());
// Limit the file listings to files which were created by successful
commits before the maxInstant time.
if (!pendingDataInstants.contains(dataFileCommitTime) &&
compareTimestamps(dataFileCommitTime, LESSER_THAN_OR_EQUALS, maxInstantTime)) {
- filenameToSizeMap.put(pathInfo.getPath().getName(),
pathInfo.getLength());
+ if (pathInfo.getLength() > 0 || !skipZeroSizeFiles) {
Review Comment:
🤖 nit: the `|| !skipZeroSizeFiles` makes the happy-path harder to parse (you
have to mentally DeMorgan it). Could you invert the guard so the skip case
comes first? e.g. `if (skipZeroSizeFiles && pathInfo.getLength() == 0) {
log.warn(...); zeroSizeFileCount++; } else { filenameToSizeMap.put(...); }`
<sub><i>⚠️ AI-generated; verify before applying. React 👍/👎 to flag
quality.</i></sub>
##########
hudi-client/hudi-client-common/src/main/java/org/apache/hudi/metadata/HoodieBackedTableMetadataWriter.java:
##########
@@ -608,12 +613,13 @@ private List<DirectoryInfo>
listAllPartitionsFromFilesystem(String initializatio
List<DirectoryInfo> processedDirectories =
engineContext.map(pathsToProcess, path -> {
HoodieStorage storage = HoodieStorageUtils.getStorage(path,
storageConf);
String relativeDirPath =
FSUtils.getRelativePartitionPath(storageBasePath, path);
- return new DirectoryInfo(relativeDirPath,
storage.listDirectEntries(path), initializationTime, pendingDataInstants);
+ return new DirectoryInfo(relativeDirPath,
storage.listDirectEntries(path), initializationTime, pendingDataInstants, true,
skipZeroSizeFiles);
Review Comment:
🤖 nit: the two adjacent boolean literals `true, skipZeroSizeFiles` at this
call site are opaque — a future reader can't tell what `true` stands for
without jumping to the constructor. Could you add an explanatory comment inline
(`/* validateHoodiePartitions= */`) or introduce a named local variable to make
the intent clear?
<sub><i>⚠️ AI-generated; verify before applying. React 👍/👎 to flag
quality.</i></sub>
--
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]
