rangareddy commented on code in PR #19477:
URL: https://github.com/apache/hudi/pull/19477#discussion_r3710088977
##########
hudi-common/src/main/java/org/apache/hudi/metrics/MetricsReporterFactory.java:
##########
@@ -122,6 +123,18 @@ private static MetricsReporter
createCloudWatchReporter(HoodieMetricsConfig metr
+ "different reporter type.",
CLOUDWATCH_REPORTER_CLASS,
HoodieMetricsConfig.METRICS_REPORTER_TYPE_VALUE.key()), e);
}
+ if (e.getCause() instanceof NoSuchMethodException) {
Review Comment:
You are right, and I reproduced it independently rather than take it on
trust. Released `hudi-aws-bundle-1.1.0-rc1` against this branch's
`hudi-common`, JDK 11:
```
STEP1 Class.forName OK -> class
org.apache.hudi.aws.metrics.cloudwatch.CloudWatchMetricsReporter
STEP2 getConstructor THREW java.lang.NoClassDefFoundError:
org/apache/hudi/config/metrics/HoodieMetricsConfig
is NoSuchMethodException? false
is Error (escapes catch(Exception))? true
```
Corroborating details I checked: `HoodieMetricsConfig` exists **only** at
`org.apache.hudi.common.config.metrics` on master, so there is no stub at the
old path; `ReflectionUtils.loadClass` catches exactly `InstantiationException |
IllegalAccessException | InvocationTargetException | NoSuchMethodException`, so
an `Error` walks straight past it; and `3cc8fd128b40` is indeed the move.
So the branch fired on duplicate jars and never on clean skew, exactly as
you said.
Fixed by adding a `catch (NoClassDefFoundError e)` ahead of the
`HoodieException` catch, with its own message that quotes `e.getMessage()` —
the vanished type is, as you say, the strongest evidence available. There are
now three branches with three different remedies rather than two.
##########
hudi-common/src/main/java/org/apache/hudi/metrics/MetricsReporterFactory.java:
##########
@@ -122,6 +123,18 @@ private static MetricsReporter
createCloudWatchReporter(HoodieMetricsConfig metr
+ "different reporter type.",
CLOUDWATCH_REPORTER_CLASS,
HoodieMetricsConfig.METRICS_REPORTER_TYPE_VALUE.key()), e);
}
+ if (e.getCause() instanceof NoSuchMethodException) {
+ // The class resolved, so hudi-aws is present; only the constructor
did not match. That means the
+ // jar providing it was built against a different Hudi version, which
is a mismatch rather than
+ // something missing, and the two need different remedies.
+ throw new HoodieException(String.format(
+ "Cannot report metrics to CloudWatch: %s was found on the
classpath but has no (%s, %s) "
+ + "constructor. The jar providing it was most likely built
against a different Hudi "
+ + "version. Use a hudi-aws-bundle whose version matches the
Hudi bundle in use, or set %s "
+ + "to a different reporter type.",
Review Comment:
Applied, and the #12902 point is the part that convinced me. That report had
`hudi-spark3.5-bundle_2.12:0.15.0` and `hudi-aws-bundle:0.15.0` — already
matching — and 0.15.0 declared the requested constructor, so "use a matching
hudi-aws-bundle" was advice that could not have helped. My own closing comment
on that issue said "pin every Hudi artifact to the same version", which is the
stale-duplicate remedy, so the message and the diagnosis disagreed.
Took your wording essentially as written. The `NoSuchMethodException` branch
now says the classpath is supplying a stale or duplicate copy and points at a
leftover `hudi-common` or `hudi-client-common`, and the clean-skew advice moved
to the new `NoClassDefFoundError` branch where it is actually true.
The `different Hudi version` assertion moved with it: the mismatch test now
asserts `stale or duplicate copy`, and a new test asserts `built against a
different Hudi version` against the `NoClassDefFoundError` branch.
##########
hudi-common/src/main/java/org/apache/hudi/metrics/MetricsReporterFactory.java:
##########
@@ -122,6 +123,18 @@ private static MetricsReporter
createCloudWatchReporter(HoodieMetricsConfig metr
+ "different reporter type.",
CLOUDWATCH_REPORTER_CLASS,
HoodieMetricsConfig.METRICS_REPORTER_TYPE_VALUE.key()), e);
}
+ if (e.getCause() instanceof NoSuchMethodException) {
+ // The class resolved, so hudi-aws is present; only the constructor
did not match. That means the
+ // jar providing it was built against a different Hudi version, which
is a mismatch rather than
+ // something missing, and the two need different remedies.
+ throw new HoodieException(String.format(
+ "Cannot report metrics to CloudWatch: %s was found on the
classpath but has no (%s, %s) "
+ + "constructor. The jar providing it was most likely built
against a different Hudi "
+ + "version. Use a hudi-aws-bundle whose version matches the
Hudi bundle in use, or set %s "
+ + "to a different reporter type.",
+ CLOUDWATCH_REPORTER_CLASS,
HoodieMetricsConfig.class.getSimpleName(),
+ MetricRegistry.class.getSimpleName(),
HoodieMetricsConfig.METRICS_REPORTER_TYPE_VALUE.key()), e);
Review Comment:
Applied — `getName()` for both. Your point is confirmed by javap on the
released bundle:
```
declared: (org.apache.hudi.config.metrics.HoodieMetricsConfig,
org.apache.hudi.com.codahale.metrics.MetricRegistry)
requested: (org.apache.hudi.common.config.metrics.HoodieMetricsConfig,
com.codahale.metrics.MetricRegistry)
```
Both differ, and for two independent reasons — the package move and the
shaded-codahale relocation — yet under simple names both render
`(HoodieMetricsConfig, MetricRegistry)`. A user running javap would have seen
an apparently matching constructor and concluded the error was wrong. I put
that reasoning in a comment above the format string so it does not get
"simplified" back later, and the mismatch test now asserts both fully qualified
names are present.
##########
hudi-common/src/main/java/org/apache/hudi/metrics/MetricsReporterFactory.java:
##########
@@ -106,8 +106,9 @@ public static Option<MetricsReporter>
createReporter(HoodieMetricsConfig metrics
/**
* The CloudWatch reporter ships in the optional {@code hudi-aws} module and
so is loaded reflectively.
- * Not every engine bundle shades that module, in which case class loading
fails without pointing at a
- * remedy. Translate that into an actionable error.
+ * Reflection reports its two distinct failures - the module is absent, or
the module is present but was
+ * built against a different Hudi version - as the same opaque {@link
HoodieException}. Translate each into
+ * an error that names its own remedy, and leave anything else untouched.
Review Comment:
Applied, with one adjustment: it is now three rather than two, since the
`NoClassDefFoundError` branch from your other comment lands here too.
```java
* Reflection collapses several unrelated failures into the same opaque
{@link HoodieException}. Three of
* them have distinct remedies - the module is absent, it was built against a
Hudi that has since moved a
* class, or the classpath carries a stale duplicate - so translate those
three, and leave every other
* failure untouched.
```
"several ... three of them" also survives the next branch, as you intended.
##########
hudi-common/src/test/java/org/apache/hudi/metrics/TestMetricsReporterFactory.java:
##########
@@ -137,22 +139,51 @@ void
metricsReporterFactoryRewritesClassNotFoundIntoAnActionableMessage() {
}
Review Comment:
Applied to both. You are right that every string they asserted also appears
in the mismatch message, so neither was discriminating:
```java
assertTrue(message.contains("was not found on the classpath"),
() -> "A missing class must not be reported as a constructor mismatch,
but was: " + message);
```
on the classpath-based test, and the same phrase asserted on the mocked one.
With that, merging or reordering the branches can no longer ship green.
##########
hudi-common/src/test/java/org/apache/hudi/metrics/TestMetricsReporterFactory.java:
##########
@@ -137,22 +139,51 @@ void
metricsReporterFactoryRewritesClassNotFoundIntoAnActionableMessage() {
}
/**
- * The other direction, and the branch most likely to regress: a failure
that is not a missing class must
- * pass through untouched, so an unrelated instantiation error is never
rewritten into "add hudi-aws-bundle".
+ * A missing constructor means hudi-aws resolved but was built against a
different Hudi version, which is
+ * a mismatch rather than something absent. Reflection reports it as the
same opaque HoodieException as a
+ * missing class, and the bare "Unable to instantiate class" that resulted
took a maintainer reading the
+ * buried NoSuchMethodException to explain (#12902).
*/
@Test
- void metricsReporterFactoryLeavesNonClassNotFoundFailuresUntouched() {
+ void metricsReporterFactoryExplainsAConstructorMismatch() {
when(metricsConfig.getMetricsReporterType()).thenReturn(MetricsReporterType.CLOUDWATCH);
try (MockedStatic<ReflectionUtils> mockedStatic =
Mockito.mockStatic(ReflectionUtils.class)) {
Review Comment:
Added, and it is the comment I am most glad you made — a suite that only
asserts the shapes the production code assumes is exactly how the
`NoClassDefFoundError` gap stayed invisible.
`metricsReporterFactoryTranslatesARealReflectionFailure` drives the real
`ReflectionUtils.loadClass` with a fixture whose only public constructor takes
a `String`, and asserts both the translated message and that the real
`NoSuchMethodException` is chained.
Your constraint about the FQCN was right, and it is why the translation now
takes the class name as a parameter:
```java
@VisibleForTesting
static MetricsReporter createCloudWatchReporter(String reporterClass,
HoodieMetricsConfig metricsConfig,
MetricRegistry registry)
```
The no-arg overload keeps `CLOUDWATCH_REPORTER_CLASS` for production
callers, so
`metricsReporterFactoryShouldExplainHowToEnableCloudWatchWhenHudiAwsIsMissing`
still depends on that name being absent from the test classpath and still
passes.
##########
hudi-common/src/test/java/org/apache/hudi/metrics/TestMetricsReporterFactory.java:
##########
@@ -137,22 +139,51 @@ void
metricsReporterFactoryRewritesClassNotFoundIntoAnActionableMessage() {
}
/**
- * The other direction, and the branch most likely to regress: a failure
that is not a missing class must
- * pass through untouched, so an unrelated instantiation error is never
rewritten into "add hudi-aws-bundle".
+ * A missing constructor means hudi-aws resolved but was built against a
different Hudi version, which is
+ * a mismatch rather than something absent. Reflection reports it as the
same opaque HoodieException as a
+ * missing class, and the bare "Unable to instantiate class" that resulted
took a maintainer reading the
+ * buried NoSuchMethodException to explain (#12902).
*/
@Test
- void metricsReporterFactoryLeavesNonClassNotFoundFailuresUntouched() {
+ void metricsReporterFactoryExplainsAConstructorMismatch() {
when(metricsConfig.getMetricsReporterType()).thenReturn(MetricsReporterType.CLOUDWATCH);
try (MockedStatic<ReflectionUtils> mockedStatic =
Mockito.mockStatic(ReflectionUtils.class)) {
mockedStatic.when(() -> ReflectionUtils.loadClass(
eq(MetricsReporterFactory.CLOUDWATCH_REPORTER_CLASS),
any(Class[].class), eq(metricsConfig), eq(registry)))
.thenThrow(new HoodieException("Unable to instantiate class " +
MetricsReporterFactory.CLOUDWATCH_REPORTER_CLASS,
new NoSuchMethodException("<init>")));
+ HoodieException exception = assertThrows(HoodieException.class,
+ () -> MetricsReporterFactory.createReporter(metricsConfig,
registry));
Review Comment:
Applied. You are right that deleting `, e` left all three assertions green —
the buried exception was the one thing worth pinning and nothing pinned it.
```java
assertEquals(NoSuchMethodException.class,
exception.getCause().getCause().getClass(),
"The original NoSuchMethodException must stay in the chain - it is the
evidence #12902 needed");
```
The new `NoClassDefFoundError` test pins its cause the same way, and the
non-mocked test asserts the chain on a real reflection failure rather than a
stubbed one.
##########
hudi-common/src/test/java/org/apache/hudi/metrics/TestMetricsReporterFactory.java:
##########
@@ -137,22 +139,51 @@ void
metricsReporterFactoryRewritesClassNotFoundIntoAnActionableMessage() {
}
/**
- * The other direction, and the branch most likely to regress: a failure
that is not a missing class must
- * pass through untouched, so an unrelated instantiation error is never
rewritten into "add hudi-aws-bundle".
+ * A missing constructor means hudi-aws resolved but was built against a
different Hudi version, which is
+ * a mismatch rather than something absent. Reflection reports it as the
same opaque HoodieException as a
+ * missing class, and the bare "Unable to instantiate class" that resulted
took a maintainer reading the
+ * buried NoSuchMethodException to explain (#12902).
*/
@Test
- void metricsReporterFactoryLeavesNonClassNotFoundFailuresUntouched() {
+ void metricsReporterFactoryExplainsAConstructorMismatch() {
when(metricsConfig.getMetricsReporterType()).thenReturn(MetricsReporterType.CLOUDWATCH);
try (MockedStatic<ReflectionUtils> mockedStatic =
Mockito.mockStatic(ReflectionUtils.class)) {
mockedStatic.when(() -> ReflectionUtils.loadClass(
eq(MetricsReporterFactory.CLOUDWATCH_REPORTER_CLASS),
any(Class[].class), eq(metricsConfig), eq(registry)))
.thenThrow(new HoodieException("Unable to instantiate class " +
MetricsReporterFactory.CLOUDWATCH_REPORTER_CLASS,
new NoSuchMethodException("<init>")));
+ HoodieException exception = assertThrows(HoodieException.class,
+ () -> MetricsReporterFactory.createReporter(metricsConfig,
registry));
+ String message = exception.getMessage();
+ assertTrue(message.contains("constructor"),
+ () -> "The failure should say the constructor did not match, but
was: " + message);
+ assertTrue(message.contains("different Hudi version"),
+ () -> "The failure should name version skew as the likely cause, but
was: " + message);
Review Comment:
Applied, including replacing `different Hudi version` with the phrase this
branch actually lands on. The mismatch test now asserts: `constructor`, `stale
or duplicate copy`, the reporter class, the config key, both fully qualified
parameter types, `was found on the classpath but`, and the chained
`NoSuchMethodException`.
The two assertions you flagged as reword-proof — reporter class and config
key — are the ones I would have missed.
##########
hudi-common/src/test/java/org/apache/hudi/metrics/TestMetricsReporterFactory.java:
##########
@@ -137,22 +139,51 @@ void
metricsReporterFactoryRewritesClassNotFoundIntoAnActionableMessage() {
}
/**
- * The other direction, and the branch most likely to regress: a failure
that is not a missing class must
- * pass through untouched, so an unrelated instantiation error is never
rewritten into "add hudi-aws-bundle".
+ * A missing constructor means hudi-aws resolved but was built against a
different Hudi version, which is
+ * a mismatch rather than something absent. Reflection reports it as the
same opaque HoodieException as a
+ * missing class, and the bare "Unable to instantiate class" that resulted
took a maintainer reading the
+ * buried NoSuchMethodException to explain (#12902).
*/
@Test
- void metricsReporterFactoryLeavesNonClassNotFoundFailuresUntouched() {
+ void metricsReporterFactoryExplainsAConstructorMismatch() {
when(metricsConfig.getMetricsReporterType()).thenReturn(MetricsReporterType.CLOUDWATCH);
try (MockedStatic<ReflectionUtils> mockedStatic =
Mockito.mockStatic(ReflectionUtils.class)) {
mockedStatic.when(() -> ReflectionUtils.loadClass(
eq(MetricsReporterFactory.CLOUDWATCH_REPORTER_CLASS),
any(Class[].class), eq(metricsConfig), eq(registry)))
.thenThrow(new HoodieException("Unable to instantiate class " +
MetricsReporterFactory.CLOUDWATCH_REPORTER_CLASS,
new NoSuchMethodException("<init>")));
+ HoodieException exception = assertThrows(HoodieException.class,
+ () -> MetricsReporterFactory.createReporter(metricsConfig,
registry));
+ String message = exception.getMessage();
+ assertTrue(message.contains("constructor"),
+ () -> "The failure should say the constructor did not match, but
was: " + message);
+ assertTrue(message.contains("different Hudi version"),
+ () -> "The failure should name version skew as the likely cause, but
was: " + message);
+ assertFalse(message.contains("was not found on the classpath"),
+ () -> "A class that resolved must not be reported as missing, but
was: " + message);
Review Comment:
Applied — positive assertion on this branch's own phrase, and the now-unused
`assertFalse` import is gone (checkstyle flagged it on the first run, exactly
as you predicted):
```java
assertTrue(message.contains("was found on the classpath but"),
() -> "A class that resolved must not be reported as missing, but was: "
+ message);
```
The point that a vacuous `assertFalse` never fails, so nothing tells you it
stopped discriminating, is the part worth remembering.
##########
hudi-common/src/test/java/org/apache/hudi/metrics/TestMetricsReporterFactory.java:
##########
@@ -137,22 +139,51 @@ void
metricsReporterFactoryRewritesClassNotFoundIntoAnActionableMessage() {
}
/**
- * The other direction, and the branch most likely to regress: a failure
that is not a missing class must
- * pass through untouched, so an unrelated instantiation error is never
rewritten into "add hudi-aws-bundle".
+ * A missing constructor means hudi-aws resolved but was built against a
different Hudi version, which is
+ * a mismatch rather than something absent. Reflection reports it as the
same opaque HoodieException as a
+ * missing class, and the bare "Unable to instantiate class" that resulted
took a maintainer reading the
+ * buried NoSuchMethodException to explain (#12902).
*/
@Test
- void metricsReporterFactoryLeavesNonClassNotFoundFailuresUntouched() {
+ void metricsReporterFactoryExplainsAConstructorMismatch() {
when(metricsConfig.getMetricsReporterType()).thenReturn(MetricsReporterType.CLOUDWATCH);
try (MockedStatic<ReflectionUtils> mockedStatic =
Mockito.mockStatic(ReflectionUtils.class)) {
mockedStatic.when(() -> ReflectionUtils.loadClass(
eq(MetricsReporterFactory.CLOUDWATCH_REPORTER_CLASS),
any(Class[].class), eq(metricsConfig), eq(registry)))
.thenThrow(new HoodieException("Unable to instantiate class " +
MetricsReporterFactory.CLOUDWATCH_REPORTER_CLASS,
new NoSuchMethodException("<init>")));
+ HoodieException exception = assertThrows(HoodieException.class,
+ () -> MetricsReporterFactory.createReporter(metricsConfig,
registry));
+ String message = exception.getMessage();
+ assertTrue(message.contains("constructor"),
+ () -> "The failure should say the constructor did not match, but
was: " + message);
+ assertTrue(message.contains("different Hudi version"),
+ () -> "The failure should name version skew as the likely cause, but
was: " + message);
+ assertFalse(message.contains("was not found on the classpath"),
+ () -> "A class that resolved must not be reported as missing, but
was: " + message);
+ }
+ }
+
+ /**
+ * The other direction, and the branch most likely to regress: a failure
that is neither a missing class
+ * nor a missing constructor must pass through untouched, so an error raised
by the reporter's own
+ * constructor is never rewritten into a classpath diagnosis.
+ */
+ @Test
+ void metricsReporterFactoryLeavesOtherFailuresUntouched() {
+
when(metricsConfig.getMetricsReporterType()).thenReturn(MetricsReporterType.CLOUDWATCH);
+ try (MockedStatic<ReflectionUtils> mockedStatic =
Mockito.mockStatic(ReflectionUtils.class)) {
Review Comment:
Taken, and it did compose with the cause assertion as you expected —
`captureCloudWatchFailure(Throwable)` now owns the stub and returns the caught
exception, so each test asserts on it directly:
```java
HoodieException exception = captureCloudWatchFailure(
new HoodieException("Unable to instantiate class " +
CLOUDWATCH_REPORTER_CLASS,
new NoSuchMethodException("<init>")));
```
It also took the `NoClassDefFoundError` case for free, since that is just a
different `Throwable` argument.
Left them as separate `@Test` methods rather than a `@ParameterizedTest`,
for the reason you gave — they assert structurally different things.
--
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]
