nsivabalan commented on code in PR #18196:
URL: https://github.com/apache/hudi/pull/18196#discussion_r2849028245
##########
hudi-client/hudi-client-common/src/main/java/org/apache/hudi/metrics/HoodieMetrics.java:
##########
@@ -341,6 +342,13 @@ public void updateRollbackMetrics(long durationInMs, long
numFilesDeleted) {
}
}
+ public void updatePostCommitMetrics(boolean status, long durationInMs) {
+ if (config.isMetricsOn()) {
+ metrics.registerGauge(getMetricsName(POST_COMMIT_STR, status ?
SUCCESS_COUNTER : FAILURE_COUNTER), 1);
Review Comment:
why gauge for the counter value
it has to be `counter` right?
##########
hudi-client/hudi-client-common/src/main/java/org/apache/hudi/config/HoodieWriteConfig.java:
##########
@@ -3425,6 +3435,11 @@ public Builder
withWriteConcurrencyMode(WriteConcurrencyMode concurrencyMode) {
return this;
}
+ public Builder withIgnorePostCommitFailure(boolean
postCommitFailureIgnored) {
Review Comment:
lets fix all method naming based on config key change
##########
hudi-client/hudi-client-common/src/main/java/org/apache/hudi/client/BaseHoodieWriteClient.java:
##########
@@ -580,8 +595,22 @@ public O postWrite(HoodieWriteMetadata<O> result, String
instantTime, HoodieTabl
result.getWriteStats().get().size());
}
- postCommit(hoodieTable, result.getCommitMetadata().get(), instantTime,
Option.empty());
- mayBeCleanAndArchive(hoodieTable);
+ boolean postCommitStatus = true;
+ HoodieTimer postCommitTimer = HoodieTimer.start();
+ try {
+ postCommit(hoodieTable, result.getCommitMetadata().get(), instantTime,
Option.empty());
+ mayBeCleanAndArchive(hoodieTable);
+ } catch (Exception e) {
+ postCommitStatus = false;
+ if (config.postCommitFailuresIngnored()) {
+ LOG.error("Ignoring exception in perform post commit processing", e);
Review Comment:
can we fix the log msg to call out that this could either be post commit or
table services post commit
##########
hudi-client/hudi-client-common/src/main/java/org/apache/hudi/client/BaseHoodieWriteClient.java:
##########
@@ -278,8 +278,23 @@ public boolean commitStats(String instantTime,
TableWriteStats tableWriteStats,
// trigger clean and archival.
// Each internal call should ensure to lock if required.
- mayBeCleanAndArchive(table);
- runTableServicesInline(table, metadata, extraMetadata);
+ boolean postCommitStatus = true;
+ HoodieTimer postCommitTimer = HoodieTimer.start();
+ try {
+ postCommit(table, metadata, instantTime, extraMetadata);
+ mayBeCleanAndArchive(table);
+ runTableServicesInline(table, metadata, extraMetadata);
+ } catch (Exception e) {
+ postCommitStatus = false;
+ if (config.postCommitFailuresIngnored()) {
+ LOG.error("Ignoring exception in perform post commit processing", e);
+ } else {
+ throw e;
+ }
+ } finally {
+ long duration = postCommitTimer.endTimer();
Review Comment:
do you think its makes sense to update this only when any of table services
are enabled?
if all of them are disabled, there is no point in emitting these metrics
right?
##########
hudi-client/hudi-client-common/src/main/java/org/apache/hudi/config/HoodieWriteConfig.java:
##########
@@ -2755,6 +2761,10 @@ public boolean shouldBlockWritesOnSpeculativeExecution()
{
return getBoolean(BLOCK_WRITES_ON_SPECULATIVE_EXECUTION);
}
+ public Boolean postCommitFailuresIngnored() {
Review Comment:
`canIgnorePostCommitFailures`
##########
hudi-client/hudi-client-common/src/main/java/org/apache/hudi/config/HoodieWriteConfig.java:
##########
@@ -720,6 +720,12 @@ public class HoodieWriteConfig extends HoodieConfig {
.markAdvanced()
.withDocumentation("");
+ public static final ConfigProperty<String> POST_COMMIT_FAILURES_IGNORED =
ConfigProperty
+ .key("hoodie.post.commit.failures.ignored")
Review Comment:
how about `hoodie.write.can.ignore.post.commit.failures`?
--
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]
