nsivabalan commented on code in PR #6502:
URL: https://github.com/apache/hudi/pull/6502#discussion_r965438383
##########
hudi-client/hudi-client-common/src/main/java/org/apache/hudi/metrics/HoodieMetrics.java:
##########
@@ -130,6 +140,13 @@ public Timer.Context getIndexCtx() {
return indexTimer == null ? null : indexTimer.time();
}
+ public Timer.Context getConflictResolutionCtx() {
+ if (config.isMetricsOn() && conflictResolutionTimer == null) {
Review Comment:
shouldn't we check for LockMetricsOn() as well ?
##########
hudi-client/hudi-client-common/src/main/java/org/apache/hudi/client/transaction/lock/LockManager.java:
##########
@@ -64,13 +69,18 @@ public void lock() {
boolean acquired = false;
while (retryCount <= maxRetries) {
try {
+ metrics.startLockApiTimerContext();
acquired =
lockProvider.tryLock(writeConfig.getLockAcquireWaitTimeoutInMs(),
TimeUnit.MILLISECONDS);
if (acquired) {
+ metrics.updateLockAcquiredMetric();
+ metrics.startLockHeldTimerContext();
Review Comment:
can we combine both these into single method. we don't have any caller which
calls either of these individually right?
##########
hudi-client/hudi-client-common/src/main/java/org/apache/hudi/config/metrics/HoodieMetricsConfig.java:
##########
@@ -83,6 +83,11 @@ public class HoodieMetricsConfig extends HoodieConfig {
.sinceVersion("0.7.0")
.withDocumentation("");
+ public static final ConfigProperty<Boolean> LOCK_METRICS_ENABLE =
ConfigProperty
+ .key(METRIC_PREFIX + ".lock.enable")
+ .defaultValue(false)
Review Comment:
actually we can add an infer function. if not explicitly set by user, we can
fetch the value for hoodie.metrics.enable.
and we may not need to set default value here.
Eg for infer function:
```
public static final ConfigProperty<String> METRICS_REPORTER_PREFIX =
ConfigProperty
.key(METRIC_PREFIX + ".reporter.metricsname.prefix")
.defaultValue("")
.sinceVersion("0.11.0")
.withInferFunction(cfg -> {
if (cfg.contains(HoodieTableConfig.NAME)) {
return Option.of(cfg.getString(HoodieTableConfig.NAME));
}
return Option.empty();
})
.withDocumentation("The prefix given to the metrics names.");
```
in HoodieMetricsConfig.
##########
hudi-client/hudi-spark-client/src/main/java/org/apache/hudi/client/SparkRDDWriteClient.java:
##########
@@ -460,8 +461,19 @@ protected void preCommit(HoodieInstant inflightInstant,
HoodieCommitMetadata met
// Create a Hoodie table after startTxn which encapsulated the commits and
files visible.
// Important to create this after the lock to ensure the latest commits
show up in the timeline without need for reload
HoodieTable table = createTable(config, hadoopConf);
- TransactionUtils.resolveWriteConflictIfAny(table,
this.txnManager.getCurrentTransactionOwner(),
- Option.of(metadata), config,
txnManager.getLastCompletedTransactionOwner(), false,
this.pendingInflightAndRequestedInstants);
+ Timer.Context indexTimer = metrics.getConflictResolutionCtx();
Review Comment:
minor. `indexTimer` -> `conflictResolutionTimer`
--
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: commits-unsubscr...@hudi.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
