hudi-agent commented on code in PR #18611:
URL: https://github.com/apache/hudi/pull/18611#discussion_r3531382802
##########
hudi-client/hudi-client-common/src/main/java/org/apache/hudi/metadata/HoodieBackedTableMetadataWriter.java:
##########
@@ -1076,7 +1076,9 @@ private List<DirectoryInfo>
listAllPartitionsFromFilesystem(String initializatio
final int fileListingParallelism =
metadataWriteConfig.getFileListingParallelism();
StorageConfiguration<?> storageConf = dataMetaClient.getStorageConf();
final String dirFilterRegex =
dataWriteConfig.getMetadataConfig().getDirectoryFilterRegex();
+ final boolean skipZeroSizeFiles =
dataWriteConfig.getMetadataConfig().shouldSkipZeroSizeFilesDuringBootstrap();
Review Comment:
🤖 `listAllPartitionsFromFilesystem` is also called from the restore-sync
path (`update(HoodieRestoreMetadata, ...)`, line 1744), so reading this config
here means it affects more than bootstrap. There the filtered listing feeds
`fetchOutofSyncFilesRecordsFromMetadataTable`, where a file present in MDT but
missing from the FS listing is scheduled for deletion (line 2321) — so a
zero-size file already tracked in MDT (incremental writes have no such guard)
would get a spurious delete record while still sitting on disk. @nsivabalan is
that restore behavior intended, or should the skip be scoped to the bootstrap
caller only?
<sub><i>⚠️ AI-generated; verify before applying. React 👍/👎 to flag
quality.</i></sub>
##########
hudi-common/src/main/java/org/apache/hudi/metadata/HoodieTableMetadataUtil.java:
##########
@@ -3130,11 +3136,20 @@ 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 combined `getLength() > 0 || !skipZeroSizeFiles` is a little hard
to parse — inverting to guard the skip case reads more directly, e.g. `if
(skipZeroSizeFiles && pathInfo.getLength() == 0) { warn + count } else { 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:
##########
@@ -1090,12 +1092,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 positional booleans `true, skipZeroSizeFiles` are
opaque at the call site — could you add an inline comment like `/*
validateHoodiePartitions */ true` to clarify which flag is which?
<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]
