[MINOR] Handle cancellation error with HoodieMetadataTableValidator#18371
[MINOR] Handle cancellation error with HoodieMetadataTableValidator#18371lokeshj1703 wants to merge 1 commit intoapache:masterfrom
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #18371 +/- ##
============================================
- Coverage 68.38% 66.11% -2.28%
+ Complexity 27505 22394 -5111
============================================
Files 2428 1994 -434
Lines 132926 110480 -22446
Branches 16000 13897 -2103
============================================
- Hits 90902 73043 -17859
+ Misses 34964 31238 -3726
+ Partials 7060 6199 -861
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
yihua
left a comment
yihua
left a comment
There was a problem hiding this comment.
🤖 This review was generated by an AI agent and may contain mistakes. Please verify any suggestions before applying.
Thanks for contributing! The cancellation-vs-validation distinction is a real issue worth fixing, and the ExceptionUtil helper is a nice reusable addition. There's one null-safety bug in the new utility that could cause an NPE at runtime, and a question about the string-match approach being tied to Spark's internal message wording — see inline comments.
| Throwable cause = t; | ||
| while (cause != null) { | ||
| if (cause.getMessage().contains(errorMsg)) { | ||
| return true; |
There was a problem hiding this comment.
🤖 cause.getMessage() can return null when an exception is constructed without a message (e.g. new RuntimeException(anotherException)), which would throw an NPE here. Could you guard this with something like cause.getMessage() != null && cause.getMessage().contains(errorMsg)?
| @@ -690,6 +691,8 @@ public boolean doMetadataTableValidation() { | |||
| } catch (SparkException sparkException) { | |||
| if (sparkException.getCause() instanceof HoodieValidationException) { | |||
| throw (HoodieValidationException) sparkException.getCause(); | |||
There was a problem hiding this comment.
🤖 The string "cancelled because SparkContext was shut down" is an internal Spark message that could silently change between Spark versions (Hudi's CI matrix already covers 3.3 through 4.0). Have you verified the wording is stable across all supported Spark versions? It might also be worth checking sparkException.getMessage() directly at the top level first, before walking nested causes, since this particular message typically surfaces at the SparkException level itself.
5 participants
Describe the issue this Pull Request addresses
Currently cancellation error is not handled in HoodieMetadataTableValidator and all spark exceptions are thrown as validation errors. The PR fixes the checks so that cancellation errors are not thrown as validation exceptions.
Summary and Changelog
Currently cancellation error is not handled in HoodieMetadataTableValidator and all spark exceptions are thrown as validation errors. The PR fixes the checks so that cancellation errors are not thrown as validation exceptions.
Impact
NA
Risk Level
Low
Documentation Update
NA
Contributor's checklist