rangareddy commented on code in PR #19418:
URL: https://github.com/apache/hudi/pull/19418#discussion_r3701440810
##########
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:
Done in bf4ef23, and agreed this is the higher-leverage half.
`ReflectionUtils.getClass` now throws `"Unable to load class " + c`, so every
reflective load in Hudi names the class it could not find, and the CloudWatch
remedy layers on top of that instead of compensating for it.
Checked the blast radius before changing it: the only assertion on that
string is `TestHoodieDeltaStreamer:444`, which uses `contains("Unable to load
class")`, so the prefix keeps it passing.
##########
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:
Applied your suggestion verbatim in bf4ef23. The staleness argument is the
convincing part — if `hudi-spark-bundle` ever does shade `hudi-aws`, which is
literally what #15293 asks for, the message would have become wrong with no
test to catch 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
Review Comment:
Fixed in the same commit. The javadoc now reads "Not every engine bundle
shades that module", with no bundle named.
##########
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:
Added `metricsReporterFactoryLeavesNonClassNotFoundFailuresUntouched` in
bf4ef23, stubbing `ReflectionUtils.loadClass` to throw `HoodieException("Unable
to instantiate class ...", new NoSuchMethodException(...))` and asserting the
message comes through byte-for-byte. That is exactly the regression you
describe: if the cause check is later widened into a deep walk, this fails.
##########
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:
Added `metricsReporterFactoryRewritesClassNotFoundIntoAnActionableMessage`
alongside it, stubbed the way you suggested. Kept the classpath-based test too,
for the reason you gave — `hudi-common` cannot depend on `hudi-aws`, so it is a
genuine end-to-end check that `ReflectionUtils` still wraps the failure the way
`createCloudWatchReporter` assumes. The two stubbed tests now pin the mapping
in both directions.
##########
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:
Done — `CLOUDWATCH_REPORTER_CLASS` is now package-private with
`@VisibleForTesting`, and both tests reference it, so the FQCN appears once.
--
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]
