voonhous commented on code in PR #19418:
URL: https://github.com/apache/hudi/pull/19418#discussion_r3694786303
##########
hudi-common/src/main/java/org/apache/hudi/metrics/MetricsReporterFactory.java:
##########
@@ -99,4 +101,27 @@ public static Option<MetricsReporter>
createReporter(HoodieMetricsConfig metrics
}
return Option.ofNullable(reporter);
}
+
+ /**
+ * The CloudWatch reporter ships in the optional {@code hudi-aws} module and
so is loaded reflectively.
+ * Not every engine bundle shades that module (notably {@code
hudi-spark-bundle} does not), in which
Review Comment:
Same staleness hook as the user-facing message below: `(notably {@code
hudi-spark-bundle} does not)` is a packaging fact that will not get updated
when packaging changes. Lower stakes in a comment, but `Not every engine bundle
shades that module` already carries the point without pinning it to one bundle.
##########
hudi-common/src/main/java/org/apache/hudi/metrics/MetricsReporterFactory.java:
##########
@@ -99,4 +101,27 @@ public static Option<MetricsReporter>
createReporter(HoodieMetricsConfig metrics
}
return Option.ofNullable(reporter);
}
+
+ /**
+ * The CloudWatch reporter ships in the optional {@code hudi-aws} module and
so is loaded reflectively.
+ * Not every engine bundle shades that module (notably {@code
hudi-spark-bundle} does not), in which
+ * case class loading fails with a message that names neither the missing
class nor a remedy. Translate
+ * that into an actionable error.
+ */
+ private static MetricsReporter createCloudWatchReporter(HoodieMetricsConfig
metricsConfig, MetricRegistry registry) {
+ try {
+ return (MetricsReporter)
ReflectionUtils.loadClass(CLOUDWATCH_REPORTER_CLASS,
+ new Class[] {HoodieMetricsConfig.class, MetricRegistry.class},
metricsConfig, registry);
+ } catch (HoodieException e) {
+ if (e.getCause() instanceof ClassNotFoundException) {
Review Comment:
Consider fixing the generic message in the same PR, since it is strictly
cheaper and has far more reach. The underlying complaint in #15293 is that
`Unable to load class` names no class at all, and that string comes from
`ReflectionUtils.getClass`:
```java
// hudi-io/src/main/java/org/apache/hudi/common/util/ReflectionUtils.java:53
throw new HoodieException("Unable to load class", e);
```
That is the message *every* reflective load in Hudi produces on a missing
class: payload classes, key generators, index types, sources, sinks, write
clients. Changing it to `"Unable to load class " + c` is one line and fixes
half of the reported defect for all of those call sites rather than just
`CLOUDWATCH`, and the CloudWatch-specific remedy here then layers on top of an
already-useful base message.
##########
hudi-common/src/test/java/org/apache/hudi/metrics/TestMetricsReporterFactory.java:
##########
@@ -94,6 +94,28 @@ void metricsReporterFactoryShouldReturnCloudWatchReporter() {
}
}
+ /**
+ * {@code hudi-aws} is deliberately absent from this module's test
classpath, which is exactly the
+ * situation a user hits with {@code hudi-spark-bundle}: that bundle does
not shade {@code hudi-aws},
+ * so the reflectively loaded CloudWatch reporter cannot be found. The
failure must name the missing
+ * class and how to fix it, not just report that some class could not be
loaded.
+ */
+ @Test
+ void
metricsReporterFactoryShouldExplainHowToEnableCloudWatchWhenHudiAwsIsMissing() {
+
when(metricsConfig.getMetricsReporterType()).thenReturn(MetricsReporterType.CLOUDWATCH);
+
+ HoodieException exception = assertThrows(HoodieException.class,
+ () -> MetricsReporterFactory.createReporter(metricsConfig, registry));
+
+ String message = exception.getMessage();
+
assertTrue(message.contains("org.apache.hudi.aws.metrics.cloudwatch.CloudWatchMetricsReporter"),
Review Comment:
The FQCN is now written in three places: the new `CLOUDWATCH_REPORTER_CLASS`
constant, line 85 above, and here. `hudi-common` already has
`org.apache.hudi.common.util.VisibleForTesting`, so making the constant
package-private and annotating it would let both tests reference it and keep
them honest if the class ever moves.
##########
hudi-common/src/test/java/org/apache/hudi/metrics/TestMetricsReporterFactory.java:
##########
@@ -94,6 +94,28 @@ void metricsReporterFactoryShouldReturnCloudWatchReporter() {
}
}
+ /**
+ * {@code hudi-aws} is deliberately absent from this module's test
classpath, which is exactly the
+ * situation a user hits with {@code hudi-spark-bundle}: that bundle does
not shade {@code hudi-aws},
+ * so the reflectively loaded CloudWatch reporter cannot be found. The
failure must name the missing
+ * class and how to fix it, not just report that some class could not be
loaded.
+ */
+ @Test
+ void
metricsReporterFactoryShouldExplainHowToEnableCloudWatchWhenHudiAwsIsMissing() {
+
when(metricsConfig.getMetricsReporterType()).thenReturn(MetricsReporterType.CLOUDWATCH);
+
+ HoodieException exception = assertThrows(HoodieException.class,
+ () -> MetricsReporterFactory.createReporter(metricsConfig, registry));
Review Comment:
The classpath premise here is solid and worth keeping: `hudi-common` cannot
depend on `hudi-aws` (the dependency runs the other way), so the absence is
structural rather than incidental, and this is a genuine end-to-end check that
`ReflectionUtils` still wraps the failure the way `createCloudWatchReporter`
assumes.
That said, the test passes for *any* reason the class fails to resolve, so
it verifies the symptom rather than the branch. Consider adding a stubbed
sibling that pins the mapping deterministically, mirroring
`metricsReporterFactoryShouldReturnCloudWatchReporter` above:
```java
mockedStatic.when(() -> ReflectionUtils.loadClass(
eq(CLOUDWATCH_REPORTER_CLASS), any(Class[].class), eq(metricsConfig),
eq(registry)))
.thenThrow(new HoodieException("Unable to load class",
new ClassNotFoundException(CLOUDWATCH_REPORTER_CLASS)));
```
##########
hudi-common/src/main/java/org/apache/hudi/metrics/MetricsReporterFactory.java:
##########
@@ -99,4 +101,27 @@ public static Option<MetricsReporter>
createReporter(HoodieMetricsConfig metrics
}
return Option.ofNullable(reporter);
}
+
+ /**
+ * The CloudWatch reporter ships in the optional {@code hudi-aws} module and
so is loaded reflectively.
+ * Not every engine bundle shades that module (notably {@code
hudi-spark-bundle} does not), in which
+ * case class loading fails with a message that names neither the missing
class nor a remedy. Translate
+ * that into an actionable error.
+ */
+ private static MetricsReporter createCloudWatchReporter(HoodieMetricsConfig
metricsConfig, MetricRegistry registry) {
+ try {
+ return (MetricsReporter)
ReflectionUtils.loadClass(CLOUDWATCH_REPORTER_CLASS,
+ new Class[] {HoodieMetricsConfig.class, MetricRegistry.class},
metricsConfig, registry);
+ } catch (HoodieException e) {
+ if (e.getCause() instanceof ClassNotFoundException) {
+ throw new HoodieException(String.format(
+ "Cannot report metrics to CloudWatch: %s was not found on the
classpath. It ships in the "
+ + "optional hudi-aws module, which is not included in every
engine bundle (for example "
+ + "hudi-spark-bundle does not bundle it). Add the
hudi-aws-bundle jar matching your Hudi "
+ + "version to the classpath, or set %s to a different reporter
type.",
+ CLOUDWATCH_REPORTER_CLASS,
HoodieMetricsConfig.METRICS_REPORTER_TYPE_VALUE.key()), e);
+ }
+ throw e;
Review Comment:
This pass-through branch is not covered by a test, and it is the one most
likely to regress: if someone later "improves" the cause check into a deep walk
of the cause chain, unrelated instantiation failures start getting rewritten
into a misleading "add hudi-aws-bundle" message and nothing would catch it.
Stubbing `ReflectionUtils.loadClass` to throw `new HoodieException("Unable
to instantiate class ...", new NoSuchMethodException())` and asserting the
message comes through unchanged would pin it.
##########
hudi-common/src/main/java/org/apache/hudi/metrics/MetricsReporterFactory.java:
##########
@@ -99,4 +101,27 @@ public static Option<MetricsReporter>
createReporter(HoodieMetricsConfig metrics
}
return Option.ofNullable(reporter);
}
+
+ /**
+ * The CloudWatch reporter ships in the optional {@code hudi-aws} module and
so is loaded reflectively.
+ * Not every engine bundle shades that module (notably {@code
hudi-spark-bundle} does not), in which
+ * case class loading fails with a message that names neither the missing
class nor a remedy. Translate
+ * that into an actionable error.
+ */
+ private static MetricsReporter createCloudWatchReporter(HoodieMetricsConfig
metricsConfig, MetricRegistry registry) {
+ try {
+ return (MetricsReporter)
ReflectionUtils.loadClass(CLOUDWATCH_REPORTER_CLASS,
+ new Class[] {HoodieMetricsConfig.class, MetricRegistry.class},
metricsConfig, registry);
+ } catch (HoodieException e) {
+ if (e.getCause() instanceof ClassNotFoundException) {
+ throw new HoodieException(String.format(
+ "Cannot report metrics to CloudWatch: %s was not found on the
classpath. It ships in the "
+ + "optional hudi-aws module, which is not included in every
engine bundle (for example "
+ + "hudi-spark-bundle does not bundle it). Add the
hudi-aws-bundle jar matching your Hudi "
+ + "version to the classpath, or set %s to a different reporter
type.",
Review Comment:
Naming specific bundle contents in a `hudi-common` error message couples
this string to `packaging/hudi-spark-bundle/pom.xml`, with nothing keeping the
two in sync. If that bundle ever shades `hudi-aws` (which is exactly what
#15293 asks for) this message silently becomes wrong and no test fails. It is
also noise for a Flink or Kafka Connect user who reaches this line for some
other reason.
Suggest dropping the parenthetical and keeping the durable part:
```suggestion
"Cannot report metrics to CloudWatch: %s was not found on the
classpath. It ships in the "
+ "optional hudi-aws module, which not every engine bundle
includes. Add the "
+ "hudi-aws-bundle jar matching your Hudi version to the
classpath, or set %s to a "
+ "different reporter type.",
```
--
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]
