codope commented on code in PR #12562:
URL: https://github.com/apache/hudi/pull/12562#discussion_r1900570723
##########
hudi-client/hudi-client-common/src/main/java/org/apache/hudi/client/BaseHoodieWriteClient.java:
##########
@@ -329,6 +316,23 @@ private void saveInternalSchema(HoodieTable table, String
instantTime, HoodieCom
}
}
+ void runTableServicesInline(Option<Map<String, String>> extraMetadata,
HoodieTable table, HoodieCommitMetadata metadata) {
+ // We don't want to fail the commit if
hoodie.fail.writes.on.inline.table.service.exception is false. We catch warn if
false
+ try {
+ // trigger clean and archival.
+ // Each internal call should ensure to lock if required.
+ mayBeCleanAndArchive(table);
+ // do this outside of lock since compaction, clustering can be time
taking and we don't need a lock for the entire execution period
+ runCompactionClusteringInline(table, metadata, extraMetadata);
+ } catch (Exception e) {
+ if (config.isFailOnInlineTableServiceExceptionEnabled()) {
Review Comment:
Can we also enable cleaning for some existing test that validates the
behavior due to this config? If this config is not being used in any test, then
we should add a test (not a blocker, can do in a followup if no such test
exists).
##########
hudi-client/hudi-client-common/src/main/java/org/apache/hudi/client/BaseHoodieWriteClient.java:
##########
@@ -329,6 +316,23 @@ private void saveInternalSchema(HoodieTable table, String
instantTime, HoodieCom
}
}
+ void runTableServicesInline(Option<Map<String, String>> extraMetadata,
HoodieTable table, HoodieCommitMetadata metadata) {
+ // We don't want to fail the commit if
hoodie.fail.writes.on.inline.table.service.exception is false. We catch warn if
false
+ try {
+ // trigger clean and archival.
+ // Each internal call should ensure to lock if required.
+ mayBeCleanAndArchive(table);
+ // do this outside of lock since compaction, clustering can be time
taking and we don't need a lock for the entire execution period
+ runCompactionClusteringInline(table, metadata, extraMetadata);
+ } catch (Exception e) {
+ if (config.isFailOnInlineTableServiceExceptionEnabled()) {
+ throw e;
+ }
+ LOG.warn("Inline table services failed with exception: " + e.getMessage()
+ + ". Moving further since
\"hoodie.fail.writes.on.inline.table.service.exception\" is set to false.");
+ }
Review Comment:
This changes the behavior for cleaner and archiver, if
`config.isFailOnInlineTableServiceExceptionEnabled()` is false. With this
change, cleaner/archiver failure will not fail the commit if that config is
overridden. Is that expected? If so, then please fix the config docs as well.
However, if the goal is better logging and error handling, then can't we simply
improve those in cleaner and archiver classes?
--
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]
