rdblue commented on code in PR #16155:
URL: https://github.com/apache/iceberg/pull/16155#discussion_r3174393146
##########
flink/v2.1/flink/src/main/java/org/apache/iceberg/flink/sink/IcebergStreamWriterMetrics.java:
##########
@@ -97,4 +108,72 @@ public Counter getFlushedDataFiles() {
public Counter getFlushedDeleteFiles() {
return flushedDeleteFiles;
}
+
+ @VisibleForTesting
+ @Nullable
+ Histogram dataFilesSizeHistogram() {
+ return dataFilesSizeHistogram;
+ }
+
+ @VisibleForTesting
+ @Nullable
+ Histogram deleteFilesSizeHistogram() {
+ return deleteFilesSizeHistogram;
+ }
+
+ /**
+ * Checks whether the Dropwizard-based histogram wrapper provided through
Flink's optional
+ * flink-metrics-dropwizard dependency is available.
+ */
+ @SuppressWarnings("CatchBlockLogException")
+ private static Histogram loadHistogramIfAvailable(
+ MetricGroup group, String name, int reservoirSize, ClassLoader
classLoader) {
+
+ try {
+ Class<?> reservoirInterface =
+ DynClasses.builder()
+ .loader(classLoader)
+ .impl("com.codahale.metrics.Reservoir")
+ .buildChecked();
+ Class<?> slidingWindowReservoirClass =
+ DynClasses.builder()
+ .loader(classLoader)
+ .impl("com.codahale.metrics.SlidingWindowReservoir")
+ .buildChecked();
+ Class<?> codahaleHistogramClass =
+ DynClasses.builder()
+ .loader(classLoader)
+ .impl("com.codahale.metrics.Histogram")
+ .buildChecked();
+ Class<?> wrapperClass =
+ DynClasses.builder()
+ .loader(classLoader)
+
.impl("org.apache.flink.dropwizard.metrics.DropwizardHistogramWrapper")
+ .buildChecked();
+
+ Object reservoir =
+ DynConstructors.builder()
+ .impl(slidingWindowReservoirClass, int.class)
+ .buildChecked()
+ .newInstance(reservoirSize);
+ Object codahaleHistogram =
+ DynConstructors.builder()
+ .impl(codahaleHistogramClass, reservoirInterface)
+ .buildChecked()
+ .newInstance(reservoir);
+ Histogram wrapper =
+ (Histogram)
+ DynConstructors.builder()
+ .impl(wrapperClass, codahaleHistogramClass)
+ .buildChecked()
+ .newInstance(codahaleHistogram);
+
+ return group.histogram(name, wrapper);
+ } catch (ClassNotFoundException | NoSuchMethodException e) {
+ LOG.warn(
+ "flink-metrics-dropwizard is not on the classpath. '{}' histogram
metrics will be disabled. Add org.apache.flink:flink-metrics-dropwizard to
enable them.",
+ name);
Review Comment:
Do we want to warn every time or just once?
Also, we try to be very direct in warning messages, stating the problem and
context first. A full sentence is usually too verbose. I would change this to
`"Cannot load Dropwizard metrics"`. That can be followed by the actual error
(`": Failed to load %s"` for `ClassNotFoundException`) if you want to include
it but I'm fine omitting it. Then I think it makes sense to add the suggestion
to fix it, `"To fix, add dependency org.apache.flink:flink-metrics-dropwizard"`
or one that leaves room for other problems, like `"Is
org.apache.flink:flink-metrics-dropwizard in the classpath?"`
--
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]
