deepakpanda93 commented on code in PR #19497:
URL: https://github.com/apache/hudi/pull/19497#discussion_r3871976534
##########
hudi-utilities/src/main/java/org/apache/hudi/utilities/HoodieMetadataTableValidator.java:
##########
@@ -810,21 +811,69 @@ List<String> validatePartitions(HoodieSparkEngineContext
engineContext, StorageP
}
}
if (misMatch.get()) {
+ // The partition lists genuinely disagree, so this run fails either
way. Before reporting it,
+ // validate the partitions both sides do agree on: throwing here skips
the per-partition file
+ // validation the caller would otherwise run, which leaves the
operator knowing only that the
+ // partition lists differ and nothing about whether the rest of the
table is consistent.
+ List<String> commonPartitions = new
ArrayList<>(allPartitionPathsFromFS);
+ commonPartitions.retainAll(allPartitionPathsMeta);
String message = "Compare Partitions Failed! " + " Additional "
+ additionalFromFS.size() + " partitions from FS, but missing from
MDT : \""
+ toStringWithThreshold(additionalFromFS, cfg.logDetailMaxLength)
+ "\" and additional " + actualAdditionalPartitionsInMDT.size()
+ " partitions from MDT, but missing from FS listing : \""
+ toStringWithThreshold(actualAdditionalPartitionsInMDT,
cfg.logDetailMaxLength)
+ "\".\n All " + allPartitionPathsFromFS.size() + " partitions
from FS listing "
- + toStringWithThreshold(allPartitionPathsFromFS,
cfg.logDetailMaxLength);
+ + toStringWithThreshold(allPartitionPathsFromFS,
cfg.logDetailMaxLength)
+ + "\n" + validateFilesInCommonPartitions(engineContext,
metaClient, commonPartitions, baseFilesForCleaning);
log.error(message);
throw new HoodieValidationException(message);
}
}
return allPartitionPathsMeta;
}
+ /**
+ * Validates the file listing for the partitions present on both sides, and
summarises the outcome.
+ *
+ * <p>Only called once a partition mismatch has already been detected and
this run is going to fail.
+ * The result is reported alongside the mismatch rather than thrown, so that
the partition list
+ * difference stays the reported cause; a file level problem found here is
extra detail about a run
+ * that was failing anyway. Any error raised while validating a common
partition is captured for the
Review Comment:
Good catch — applied. I checked the premise before renaming and it holds:
every other `validate*` in this class returns `void` and signals failure by
throwing. `validatePartitions` does return a `List<String>`, but that is its
product rather than a failure channel, and it still throws on mismatch. This
method was genuinely the only one reporting a problem through its return value,
so the name was misleading.
Renamed to `summarizeFileValidationForCommonPartitions`, and I made the
contract explicit in the javadoc rather than leaving it to the name alone:
```java
/**
* Validates the file listing for the partitions present on both sides and
returns a summary of the
* outcome. Unlike the other {@code validate} methods here, this one never
throws: it reports through
* its return value instead.
* ...
*/
private String summarizeFileValidationForCommonPartitions(...)
```
Also fixed a spelling slip of mine in the same javadoc while I was there: it
said "summarises", which was the only British spelling of that word in the repo.
Re-ran `TestHoodieMetadataTableValidator` (spark3.5): 50/50 pass. checkstyle
and apache-rat clean on `hudi-utilities`.
--
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]
