danny0405 commented on code in PR #11035:
URL: https://github.com/apache/hudi/pull/11035#discussion_r1599312674
##########
hudi-client/hudi-client-common/src/main/java/org/apache/hudi/table/action/commit/BaseCommitActionExecutor.java:
##########
@@ -112,6 +113,10 @@ public BaseCommitActionExecutor(HoodieEngineContext
context, HoodieWriteConfig c
public abstract HoodieWriteMetadata<O> execute(I inputRecords);
+ public HoodieWriteMetadata<O> execute(I inputRecords, Option<HoodieTimer>
sourceReadAndIndexTimer) {
+ return this.execute(inputRecords);
Review Comment:
Not sure why we need a new `#execute` interface, I see that all the impl
executors initialize the timer on the fly while invoking this method, so why
not just initialize the timer in the `#execute`itself.
##########
hudi-client/hudi-client-common/src/main/java/org/apache/hudi/table/action/HoodieWriteMetadata.java:
##########
@@ -34,6 +34,7 @@ public class HoodieWriteMetadata<O> {
private O writeStatuses;
private Option<Duration> indexLookupDuration = Option.empty();
Review Comment:
Should we remove this?
##########
hudi-client/hudi-client-common/src/main/java/org/apache/hudi/table/action/commit/BaseWriteHelper.java:
##########
@@ -46,22 +47,31 @@ public HoodieWriteMetadata<O> write(String instantTime,
int configuredShuffleParallelism,
BaseCommitActionExecutor<T, I, K, O, R>
executor,
WriteOperationType operationType) {
+ return this.write(instantTime, inputRecords, context, table,
shouldCombine, configuredShuffleParallelism, executor, operationType,
Option.empty());
+ }
+
+ public HoodieWriteMetadata<O> write(String instantTime,
+ I inputRecords,
+ HoodieEngineContext context,
+ HoodieTable<T, I, K, O> table,
+ boolean shouldCombine,
+ int configuredShuffleParallelism,
+ BaseCommitActionExecutor<T, I, K, O, R>
executor,
+ WriteOperationType operationType,
+ Option<HoodieTimer>
sourceReadAndIndexTimer) {
try {
// De-dupe/merge if needed
I dedupedRecords =
combineOnCondition(shouldCombine, inputRecords,
configuredShuffleParallelism, table);
- Instant lookupBegin = Instant.now();
I taggedRecords = dedupedRecords;
Review Comment:
Same question, why not just initialzie the timer here so that we can avoid
to introduce a new method.
##########
hudi-client/hudi-spark-client/src/main/java/org/apache/hudi/client/SparkRDDWriteClient.java:
##########
@@ -141,8 +141,8 @@ public JavaRDD<WriteStatus> upsert(JavaRDD<HoodieRecord<T>>
records, String inst
preWrite(instantTime, WriteOperationType.UPSERT, table.getMetaClient());
HoodieWriteMetadata<HoodieData<WriteStatus>> result =
table.upsert(context, instantTime, HoodieJavaRDD.of(records));
HoodieWriteMetadata<JavaRDD<WriteStatus>> resultRDD =
result.clone(HoodieJavaRDD.getJavaRDD(result.getWriteStatuses()));
- if (result.getIndexLookupDuration().isPresent()) {
- metrics.updateIndexMetrics(LOOKUP_STR,
result.getIndexLookupDuration().get().toMillis());
+ if (result.getSourceReadAndIndexDurationMs().isPresent()) {
+ metrics.updateSourceReadAndIndexMetrics(LOOKUP_STR,
result.getSourceReadAndIndexDurationMs().get());
Review Comment:
Should we still use `LOOKUP_STR` here?
--
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]
