-
Notifications
You must be signed in to change notification settings - Fork 2.5k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[HUDI-7071] Throw exceptions when clustering/index job fail #10050
Conversation
LOG.error(errorMessage); | ||
if (t instanceof HoodieException) { | ||
throw new HoodieException(errorMessage, t); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's not modify the very basic utilities, it is shared by many components.
Is it fixed via: #10108 ? |
It's good. All services those calling |
if (ret != 0) { | ||
throw new HoodieException("Fail to run compaction for " + cfg.tableName + ", return code: " + ret); | ||
} | ||
LOG.info("Success to run compaction for " + cfg.tableName); | ||
jsc.stop(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you help me understand why remove try-catch block here?
If L175 failed and threw an exception then L180 would not be executed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
L175 will calls UtilHelpers.retry
internal, which has try-catch block. This try-catch block is unreachable.
} | ||
LOG.info(resultMsg + " success"); | ||
jsc.stop(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same here, I think we can add a try catch block here to make sure jsc exit gracefully
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like HoodieIndexer
also uses UtilHelpers.retry
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes. HoodieCompactor/HoodieClusteringJob/HoodieIndexer
use UtilHelpers.retry
, so it's no need to add try catch block in main method.
} | ||
LOG.info(resultMsg + " success"); | ||
jsc.stop(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was no try-catch block in HoodieClusteringJob originally. If cluster throws HoodieException, the job returns -1 and jsc stops normally
plz cc @CTTY @danny0405 |
cc @CTTY for the review~ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Change Logs
When jobs throw exeption
org.apache.hudi.exception.HoodieException: unable to read next record from parquet file
, we do not deal with this exception. This results the job not failing and final state is success.Impact
none
Risk level (write none, low medium or high below)
If medium or high, explain what verification was done to mitigate the risks.
Documentation Update
Describe any necessary documentation update if there is any new feature, config, or user-facing change
ticket number here and follow the instruction to make
changes to the website.
Contributor's checklist