danny0405 commented on code in PR #19575:
URL: https://github.com/apache/hudi/pull/19575#discussion_r3877789261
##########
hudi-client/hudi-spark-client/src/main/java/org/apache/hudi/client/SparkRDDWriteClient.java:
##########
@@ -146,8 +147,16 @@ public boolean commit(String instantTime,
JavaRDD<WriteStatus> rawWriteStatuses,
// when streaming writes are enabled, writeStatuses is a mix of data
table write status and mdt write status
List<HoodieWriteStat> dataTableHoodieWriteStats =
slimWriteStatsList.stream().filter(entry ->
!entry.isMetadataTable()).map(SlimWriteStats::getWriteStat).collect(Collectors.toList());
List<HoodieWriteStat> partialMetadataTableWriteStats =
slimWriteStatsList.stream().filter(entry ->
entry.isMetadataTable).map(SlimWriteStats::getWriteStat).collect(Collectors.toList());
- return commitStats(instantTime, new
TableWriteStats(dataTableHoodieWriteStats, partialMetadataTableWriteStats),
extraMetadata, commitActionType, partitionToReplacedFileIds, extraPreCommitFunc,
- false, Option.of(table));
+ boolean committed = commitStats(instantTime, new
TableWriteStats(dataTableHoodieWriteStats, partialMetadataTableWriteStats),
extraMetadata, commitActionType, partitionToReplacedFileIds,
+ extraPreCommitFunc, false, Option.of(table));
+ // Publish only if this write tagged. requiresTagging is the same
predicate BaseWriteHelper uses
+ // to decide whether to look up at all, so publishing mirrors
collecting: an INSERT or a
+ // BULK_INSERT never collects, and must not publish counters an earlier
write left behind.
+ if (committed && getOperationType() != null
Review Comment:
and it seems that `commitStats(` will never return false, it either return
true or throws.
also can we move the rli lookup metrics publish into the `postCommit`
method? since that is where the metrics publish should be located.
##########
hudi-client/hudi-spark-client/src/main/java/org/apache/hudi/client/SparkRDDWriteClient.java:
##########
@@ -146,8 +147,16 @@ public boolean commit(String instantTime,
JavaRDD<WriteStatus> rawWriteStatuses,
// when streaming writes are enabled, writeStatuses is a mix of data
table write status and mdt write status
List<HoodieWriteStat> dataTableHoodieWriteStats =
slimWriteStatsList.stream().filter(entry ->
!entry.isMetadataTable()).map(SlimWriteStats::getWriteStat).collect(Collectors.toList());
List<HoodieWriteStat> partialMetadataTableWriteStats =
slimWriteStatsList.stream().filter(entry ->
entry.isMetadataTable).map(SlimWriteStats::getWriteStat).collect(Collectors.toList());
- return commitStats(instantTime, new
TableWriteStats(dataTableHoodieWriteStats, partialMetadataTableWriteStats),
extraMetadata, commitActionType, partitionToReplacedFileIds, extraPreCommitFunc,
- false, Option.of(table));
+ boolean committed = commitStats(instantTime, new
TableWriteStats(dataTableHoodieWriteStats, partialMetadataTableWriteStats),
extraMetadata, commitActionType, partitionToReplacedFileIds,
+ extraPreCommitFunc, false, Option.of(table));
+ // Publish only if this write tagged. requiresTagging is the same
predicate BaseWriteHelper uses
+ // to decide whether to look up at all, so publishing mirrors
collecting: an INSERT or a
+ // BULK_INSERT never collects, and must not publish counters an earlier
write left behind.
+ if (committed && getOperationType() != null
Review Comment:
can we check if the rli lookup metrics is enabled in the very first place so
that we avoid the unnecessary check, inside
`RecordIndexLookupMetrics.publishAndRelease`, assumes the metircs is enabled,
and if it is skipped, log some warning msg there.
```java
if (<committed> && <rli lookup metrics enabled> && ...) {
RecordIndexLookupMetrics.publishAndRelease(config, metrics);
}
```
and it seems that `commitStats(` will never return false, it either return
true or throws.
also can we move the rli lookup metrics publish into the `postCommit`
method? since that is where the metrics publish should be located.
--
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]
