nastra commented on code in PR #5780:
URL: https://github.com/apache/iceberg/pull/5780#discussion_r974960351
##########
api/src/main/java/org/apache/iceberg/metrics/ScanReport.java:
##########
@@ -419,86 +154,60 @@ public static class ScanMetrics {
public static final String TOTAL_FILE_SIZE_IN_BYTES =
"total-file-size-in-bytes";
public static final String TOTAL_DELETE_FILE_SIZE_IN_BYTES =
"total-delete-file-size-in-bytes";
public static final String SKIPPED_DATA_MANIFESTS =
"skipped-data-manifests";
- private final Timer totalPlanningDuration;
- private final Counter resultDataFiles;
- private final Counter resultDeleteFiles;
- private final Counter totalDataManifests;
- private final Counter totalDeleteManifests;
- private final Counter scannedDataManifests;
- private final Counter skippedDataManifests;
- private final Counter totalFileSizeInBytes;
- private final Counter totalDeleteFileSizeInBytes;
-
- public ScanMetrics(MetricsContext metricsContext) {
- Preconditions.checkArgument(null != metricsContext, "Invalid metrics
context: null");
- this.totalPlanningDuration =
- metricsContext.timer(TOTAL_PLANNING_DURATION, TimeUnit.NANOSECONDS);
- this.resultDataFiles = metricsContext.counter(RESULT_DATA_FILES,
MetricsContext.Unit.COUNT);
- this.resultDeleteFiles =
- metricsContext.counter(RESULT_DELETE_FILES,
MetricsContext.Unit.COUNT);
- this.scannedDataManifests =
- metricsContext.counter(SCANNED_DATA_MANIFESTS,
MetricsContext.Unit.COUNT);
- this.totalDataManifests =
- metricsContext.counter(TOTAL_DATA_MANIFESTS,
MetricsContext.Unit.COUNT);
- this.totalDeleteManifests =
- metricsContext.counter(TOTAL_DELETE_MANIFESTS,
MetricsContext.Unit.COUNT);
- this.totalFileSizeInBytes =
- metricsContext.counter(TOTAL_FILE_SIZE_IN_BYTES,
MetricsContext.Unit.BYTES);
- this.totalDeleteFileSizeInBytes =
- metricsContext.counter(TOTAL_DELETE_FILE_SIZE_IN_BYTES,
MetricsContext.Unit.BYTES);
- this.skippedDataManifests =
- metricsContext.counter(SKIPPED_DATA_MANIFESTS,
MetricsContext.Unit.COUNT);
+
+ public static ScanMetrics noop() {
+ return ScanMetrics.of(MetricsContext.nullMetrics());
}
+ public abstract MetricsContext metricsContext();
+
+ @Value.Derived
public Timer totalPlanningDuration() {
- return totalPlanningDuration;
+ return metricsContext().timer(TOTAL_PLANNING_DURATION,
TimeUnit.NANOSECONDS);
Review Comment:
I see the `MetricsContext` parameter here less as a "this class needs to
keep track of this" but rather "this is what's required to create an instance
of this class". Your comment is perfectly valid here and I initially had the
exact same thought.
Therefore it make sense to have a `ScanMetrics.from(MetricsContext)`. In
fact we have a `ScanMetrics.of(MetricsContext)` that does exactly that.
We could go ahead and change `ScanMetrics.of(...)` to:
```
public static ScanMetrics of(MetricsContext metricsContext) {
return ImmutableScanMetrics.builder()
.metricsContext(metricsContext)
.totalPlanningDuration(
metricsContext.timer(TOTAL_PLANNING_DURATION,
TimeUnit.NANOSECONDS))
.resultDataFiles(metricsContext.counter(RESULT_DATA_FILES,
MetricsContext.Unit.COUNT))
.resultDeleteFiles(metricsContext.counter(RESULT_DELETE_FILES,
MetricsContext.Unit.COUNT))
.scannedDataManifests(
metricsContext.counter(SCANNED_DATA_MANIFESTS,
MetricsContext.Unit.COUNT))
.totalDataManifests(
metricsContext.counter(TOTAL_DATA_MANIFESTS,
MetricsContext.Unit.COUNT))
.totalDeleteManifests(
metricsContext.counter(TOTAL_DELETE_MANIFESTS,
MetricsContext.Unit.COUNT))
.totalFileSizeInBytes(
metricsContext.counter(TOTAL_FILE_SIZE_IN_BYTES,
MetricsContext.Unit.BYTES))
.totalDeleteFileSizeInBytes(
metricsContext.counter(TOTAL_DELETE_FILE_SIZE_IN_BYTES,
MetricsContext.Unit.BYTES))
.skippedDataManifests(
metricsContext.counter(SKIPPED_DATA_MANIFESTS,
MetricsContext.Unit.COUNT))
.build();
}
```
It would produce the exact same results. The only reason I went with
`@Value.Derived` is because it does not allow manual creation of metric
instances.
`ScanMetrics.of(...)` is just a utility method and it doesn't prevent anyone
from using the Builder directly. Using `@Value.Derived` in this context means
essentially that all those metric instances are derived from `MetricsContext`
but **cannot be manually set**, thus providing a builder where only
`ImmutableScanMetrics.builder() .metricsContext(metricsContext).build()`
can be set.
The lazy initialization of metrics that `@Value.Derived` provides is just a
nice thing to have that we get here, but my main motivation was to prevent the
different counters/timers to be manually set. This gives us the exact same
semantics we had in the previous `ScanMetrics` API, since all the metrics were
initialized in the constructor.
--
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]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
