hudi-agent commented on code in PR #18710:
URL: https://github.com/apache/hudi/pull/18710#discussion_r3211477117
##########
hudi-common/src/main/java/org/apache/hudi/config/metrics/HoodieMetricsConfig.java:
##########
@@ -73,7 +73,10 @@ public class HoodieMetricsConfig extends HoodieConfig {
.defaultValue("")
.markAdvanced()
.sinceVersion("0.6.0")
- .withDocumentation("");
+ .withDocumentation("Fully qualified class name of a custom
MetricsReporter implementation. "
+ + "The class must extend MetricsReporter and support one of these
constructors (tried in order): "
+ + "(Properties, MetricRegistry), (HoodieMetricsConfig,
MetricRegistry), or no-arg. "
+ + "Set reporter.type=CUSTOM or leave it unset when using this
option.");
Review Comment:
🤖 The phrase "or leave it unset when using this option" doesn't match the
actual code behavior. `METRICS_REPORTER_TYPE_VALUE` defaults to `GRAPHITE` (not
unset), and even when it is null `createReporter` returns `Option.empty()` —
neither path will load the custom class. Users effectively must set
`reporter.type=CUSTOM`. Could you tighten this doc accordingly?
<sub><i>- AI-generated; verify before applying. React 👍/👎 to flag
quality.</i></sub>
##########
hudi-common/src/main/java/org/apache/hudi/metrics/MetricsReporterFactory.java:
##########
@@ -93,10 +80,35 @@ public static Option<MetricsReporter>
createReporter(HoodieMetricsConfig metrics
case SLF4J:
reporter = new Slf4jMetricsReporter(registry);
break;
+ case CUSTOM:
+ String reporterClassName = metricsConfig.getMetricReporterClassName();
+ if (!StringUtils.isNullOrEmpty(reporterClassName)) {
Review Comment:
🤖 nit: could you invert this into a guard clause? Throwing first and then
falling through to the happy-path `return` removes the `else` branch and makes
the intent clearer — e.g. `if (StringUtils.isNullOrEmpty(reporterClassName)) {
throw …; } return Option.of(loadCustomReporter(…));`
<sub><i>- AI-generated; verify before applying. React 👍/👎 to flag
quality.</i></sub>
##########
hudi-common/src/main/java/org/apache/hudi/metrics/MetricsReporterFactory.java:
##########
@@ -93,10 +80,35 @@ public static Option<MetricsReporter>
createReporter(HoodieMetricsConfig metrics
case SLF4J:
reporter = new Slf4jMetricsReporter(registry);
break;
+ case CUSTOM:
Review Comment:
🤖 This looks like a silent backward-incompatible change. Before this PR, if
a user set `hoodie.metrics.reporter.class=com.foo.MyReporter` without touching
`reporter.type`, the class-name check ran first and the custom reporter was
loaded. After this PR, since `METRICS_REPORTER_TYPE_VALUE` defaults to
`GRAPHITE`, the same config will silently fall into the `GRAPHITE` case and
ignore the user's custom class — they'll get a Graphite reporter (or a startup
failure) with no warning. Could you preserve the prior behavior — e.g., if
`reporter.class` is set and `reporter.type` is left at the default, treat it as
`CUSTOM` (or at least log a clear warning) so existing deployments aren't
silently broken? @yihua
<sub><i>- AI-generated; verify before applying. React 👍/👎 to flag
quality.</i></sub>
--
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]
