hudi-agent commented on code in PR #19243:
URL: https://github.com/apache/hudi/pull/19243#discussion_r3566150038
##########
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:
🤖 Skipped files stay on disk but are left out of the MDT, creating a lasting
filesystem-vs-MDT divergence — note the new test has to `Files.delete()` the
file before `validateMetadata()` passes for exactly this reason. In production
the file remains, so consistency checks would flag it, the cleaner won't track
it, and queries silently omit its records rather than surfacing the bad file.
Could you call out this trade-off in the config's documentation and confirm
skip (vs. fail-fast with a pointer to the bad file) is the intended behavior?
<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:
##########
@@ -1736,7 +1744,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:
🤖 The restore path hardcodes `skipZeroSizeFiles=false`, so it doesn't honor
the new config. If a user enabled it,
`fetchOutofSyncFilesRecordsFromMetadataTable` will see these files as FS-only
(in the listing but not in MDT), re-add them via `partitionFilesToAdd`, and the
following column-stats/bloom-filter sync could then fail reading the zero-size
file — the same failure this PR avoids on init. Is leaving restore unguarded
intentional?
<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]
