Davis-Zhang-Onehouse commented on code in PR #12562:
URL: https://github.com/apache/hudi/pull/12562#discussion_r1901218368
##########
hudi-client/hudi-client-common/src/main/java/org/apache/hudi/client/BaseHoodieWriteClient.java:
##########
@@ -256,17 +256,7 @@ public boolean commitStats(String instantTime,
HoodieData<WriteStatus> writeStat
// trigger clean and archival.
// Each internal call should ensure to lock if required.
mayBeCleanAndArchive(table);
- // 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 {
- // do this outside of lock since compaction, clustering can be time
taking and we don't need a lock for the entire execution period
- runTableServicesInline(table, metadata, extraMetadata);
- } catch (Exception e) {
Review Comment:
moved try catch inside the function
##########
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:
It makes sense. Let me move it out
##########
hudi-client/hudi-client-common/src/main/java/org/apache/hudi/client/BaseHoodieWriteClient.java:
##########
@@ -578,12 +568,28 @@ protected void postCommit(HoodieTable table,
HoodieCommitMetadata metadata, Stri
* @param table instance of {@link HoodieTable} of interest.
*/
protected void mayBeCleanAndArchive(HoodieTable table) {
- autoCleanOnCommit();
- autoArchiveOnCommit(table);
+ try {
+ autoCleanOnCommit();
+ autoArchiveOnCommit(table);
+ } catch (Throwable t) {
+ LOG.error(String.format("Inline cleaning or clustering failed for {}",
table.getConfig().getBasePath()), t);
+ throw t;
+ }
}
protected void runTableServicesInline(HoodieTable table,
HoodieCommitMetadata metadata, Option<Map<String, String>> extraMetadata) {
- tableServiceClient.runTableServicesInline(table, metadata, extraMetadata);
+ // 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 {
+ // do this outside of lock since compaction, clustering can be time
taking and we don't need a lock for the entire execution period
+ tableServiceClient.runTableServicesInline(table, metadata,
extraMetadata);
+ } catch (Throwable t) {
+ LOG.error(String.format("Inline compaction or clustering failed for
table {}.", table.getConfig().getBasePath()), t);
+ // Throw if this is exception and the exception is configured to throw
or if it is something else like Error.
+ if (config.isFailOnInlineTableServiceExceptionEnabled() || !(t
instanceof Exception)) {
Review Comment:
throw unconditionally if it is not an exception
##########
hudi-client/hudi-client-common/src/main/java/org/apache/hudi/client/BaseHoodieWriteClient.java:
##########
@@ -578,12 +568,28 @@ protected void postCommit(HoodieTable table,
HoodieCommitMetadata metadata, Stri
* @param table instance of {@link HoodieTable} of interest.
*/
protected void mayBeCleanAndArchive(HoodieTable table) {
- autoCleanOnCommit();
- autoArchiveOnCommit(table);
+ try {
+ autoCleanOnCommit();
+ autoArchiveOnCommit(table);
+ } catch (Throwable t) {
+ LOG.error(String.format("Inline cleaning or clustering failed for {}",
table.getConfig().getBasePath()), t);
+ throw t;
+ }
}
protected void runTableServicesInline(HoodieTable table,
HoodieCommitMetadata metadata, Option<Map<String, String>> extraMetadata) {
- tableServiceClient.runTableServicesInline(table, metadata, extraMetadata);
+ // 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 {
+ // do this outside of lock since compaction, clustering can be time
taking and we don't need a lock for the entire execution period
+ tableServiceClient.runTableServicesInline(table, metadata,
extraMetadata);
+ } catch (Throwable t) {
+ LOG.error(String.format("Inline compaction or clustering failed for
table {}.", table.getConfig().getBasePath()), t);
Review Comment:
log throwable while previously it is only exception
##########
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()
Review Comment:
done, also I catch Error in this branch and after we log that it is thrown
unconditionally
##########
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:
added test dimension to the existing test.
--
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]
