nsivabalan commented on code in PR #18585:
URL: https://github.com/apache/hudi/pull/18585#discussion_r3406729381
##########
hudi-client/hudi-spark-client/src/main/java/org/apache/hudi/client/validator/SparkPreCommitValidator.java:
##########
@@ -82,6 +82,11 @@ public void validate(String instantTime,
HoodieWriteMetadata<O> writeResult, Dat
HoodieTimer timer = HoodieTimer.start();
try {
validateRecordsBeforeAndAfter(before, after,
getPartitionsModified(writeResult));
+ } catch (HoodieValidationException e) {
+ throw e;
+ } catch (Exception e) {
Review Comment:
Heads up on a subtle behavior change here that I think is worth either
calling out explicitly or pushing back on:
The new `catch (Exception e)` block wraps **any** non-validation exception
(NPE, ClassCastException, IllegalStateException from a buggy validator, etc.)
into a `HoodieValidationException`. Combined with
`SparkValidatorUtils.runValidator()` which catches only
`HoodieValidationException` and returns `false`, the net effect is:
```
validator throws NPE
-> validate() wraps it as HoodieValidationException
-> runValidator catches HoodieValidationException, returns false
-> allSuccess = false -> throws "... validation failed ..." (single
message, NPE buried in cause chain)
```
Pre-PR, a bug-in-validator NPE would propagate up through
`CompletableFuture.join()` as a `CompletionException` and abort the commit with
the original stack trace visible to the operator.
Two paths I'd prefer over the current shape:
1. **If intentional (treat all validator failures uniformly):** worth a
one-line note in the PR description so users know that validator bugs no longer
crash-loud — they now crash-silent-then-fail. Operators chasing a flaky NPE in
a custom validator will be confused why the top-level exception just says
"validation failed."
2. **If unintentional:** I'd narrow the catch in `validate()` to only the
exception types you actually expect (e.g. SQL/Spark exceptions the validator
might propagate), or have `runValidator` distinguish "validator returned false
via legitimate validation failure" from "validator threw unexpected" and
rethrow the latter.
Either way it'd be good to lock down the contract with a test in
`TestSparkValidatorUtils` where a validator throws a
non-`HoodieValidationException` (NPE/IllegalStateException) and assert the
desired behavior.
--
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]
