voonhous commented on code in PR #19477:
URL: https://github.com/apache/hudi/pull/19477#discussion_r3704129477
##########
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:
**This branch does not fire for the case the message names.**
`Class.getConstructor` resolves the parameter types of *all* public
constructors, not just the requested one. #19193 (`3cc8fd128b40`) moved
`HoodieMetricsConfig` from `org.apache.hudi.config.metrics` to
`org.apache.hudi.common.config.metrics` with no compat stub, so on a plain
"older hudi-aws + newer engine bundle" classpath the lookup dies resolving the
*other* constructor's parameter type and throws `NoClassDefFoundError`. That is
an `Error`, so `ReflectionUtils.loadClass` never catches it (its catch lists
only `Exception` subtypes) and the `catch (HoodieException e)` above never runs.
Reproduced on JDK 11, released `hudi-aws-bundle-1.0.2.jar` on the classpath
with this branch's `hudi-common`:
```
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(HoodieException))? true
```
`javap -cp hudi-aws-bundle-1.0.2.jar
org.apache.hudi.aws.metrics.cloudwatch.CloudWatchMetricsReporter` shows why:
its constructors take `org.apache.hudi.config.metrics.HoodieMetricsConfig` and
`org.apache.hudi.com.codahale.metrics.MetricRegistry`.
Adding a stale `hudi-common` + `hudi-client-common` to that same classpath
flips the probe to `NoSuchMethodException` - so this branch fires on duplicate
jars, not on the clean version skew it describes.
Before merge: widen the catch at line 117 to `catch (HoodieException |
NoClassDefFoundError e)` and route `NoClassDefFoundError` here. Its
`getMessage()` names the class that vanished, which is the strongest evidence
of skew available.
##########
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:
`getSimpleName()` erases the only two things that discriminate this failure.
In the reachable trace the requested signature is
`(org.apache.hudi.common.config.metrics.HoodieMetricsConfig,
com.codahale.metrics.MetricRegistry)` while the jar declares
`(org.apache.hudi.config.metrics.HoodieMetricsConfig,
org.apache.hudi.com.codahale.metrics.MetricRegistry)` - verified with `javap
-cp hudi-aws-bundle-1.0.2.jar
org.apache.hudi.aws.metrics.cloudwatch.CloudWatchMetricsReporter`. Under simple
names both render as `(HoodieMetricsConfig, MetricRegistry)`, so a user who
runs javap sees a constructor that appears to match and concludes the error is
lying. The package move and the codahale relocation are precisely the evidence
they need.
```suggestion
CLOUDWATCH_REPORTER_CLASS, HoodieMetricsConfig.class.getName(),
MetricRegistry.class.getName(),
HoodieMetricsConfig.METRICS_REPORTER_TYPE_VALUE.key()), e);
```
##########
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:
nit, feel free to ignore: "its two distinct failures" undercounts.
`ReflectionUtils.loadClass` collapses four exception types
(`InstantiationException`, `IllegalAccessException`,
`InvocationTargetException`, `NoSuchMethodException`), plus the
`ClassNotFoundException` that `getClass` throws separately. This method handles
two of them by choice, not because there are only two. Wording it as "two of
several" also survives adding the `NoClassDefFoundError` branch from my comment
on line 126.
```suggestion
* Reflection collapses several unrelated failures into the same opaque
{@link HoodieException}. Two of
* them have distinct remedies - the module is absent, or it is present
but does not declare the
* constructor - so translate those two, and leave every other failure
untouched.
```
##########
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:
**The remedy is inverted relative to when this branch actually fires.**
Per the probe in my comment on line 126: a clean older `hudi-aws-bundle`
throws `NoClassDefFoundError` and never reaches here. What does reach here is a
classpath carrying a stale or duplicate copy of the class or of its parameter
types. So the message prescribes "use a matching hudi-aws-bundle" exactly when
that is not the fix, and never in the case where it is.
That also holds for #12902, which this PR closes. 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 did declare the requested constructor:
```
git show
release-0.15.0:hudi-client/hudi-client-common/src/main/java/org/apache/hudi/metrics/cloudwatch/CloudWatchMetricsReporter.java
| sed -n '52p'
-> public CloudWatchMetricsReporter(HoodieMetricsConfig metricsConfig,
MetricRegistry registry) {
```
Your own closing comment on that issue reaches the right diagnosis - a stale
duplicate copy, remedied by "pin every Hudi artifact to the same version". The
message says something else.
Please point at the cause that actually produces this branch:
```suggestion
"Cannot report metrics to CloudWatch: %s was found on the
classpath but does not declare a "
+ "(%s, %s) constructor. Some jar on the classpath is
supplying a stale or duplicate copy "
+ "of this class or of its parameter types. Check for more
than one Hudi version on the "
+ "classpath - a leftover hudi-common or hudi-client-common
is the usual culprit - and "
+ "align every Hudi artifact, including the engine bundle,
to one version. Or set %s to "
+ "a different reporter type.",
```
This drops the "different Hudi version" phrase, so the assertion at
`TestMetricsReporterFactory.java:161` needs to change with it.
##########
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:
All three mismatch tests stub `ReflectionUtils` with `mockStatic`, so none
of them asserts that real reflection ever produces the shape being handled -
they assert the shape the production code assumes. That is exactly how the
`NoClassDefFoundError` gap in my comment on `MetricsReporterFactory.java:126`
stays invisible with this suite green.
Please add one non-mocked test: a fixture class in the test tree whose
public constructor deliberately does not match `(HoodieMetricsConfig,
MetricRegistry)`, loaded through the real `ReflectionUtils.loadClass`,
asserting on the actual throwable.
One constraint worth knowing before you try it: the fixture cannot be named
`CLOUDWATCH_REPORTER_CLASS`, because that FQCN would then resolve on
hudi-common's test classpath and break
`metricsReporterFactoryShouldExplainHowToEnableCloudWatchWhenHudiAwsIsMissing`
at line 106, which depends on that name being absent. So the translation would
need to take the class name as a parameter to be reachable this way.
##########
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:
**Nothing here pins the original failure into the cause chain.**
Delete the `, e` from the `throw new HoodieException(...)` at
`MetricsReporterFactory.java:136` and all three assertions below still pass -
the buried `NoSuchMethodException` would be dropped silently and CI stays
green. That exception is exactly the evidence a maintainer had to dig out of
the stack to resolve #12902, so it is the one thing in this test worth pinning.
```suggestion
.thenThrow(new HoodieException("Unable to instantiate class " +
MetricsReporterFactory.CLOUDWATCH_REPORTER_CLASS,
new NoSuchMethodException("<init>")));
HoodieException exception = assertThrows(HoodieException.class,
() -> MetricsReporterFactory.createReporter(metricsConfig,
registry));
assertEquals(NoSuchMethodException.class,
exception.getCause().getCause().getClass(),
"The original NoSuchMethodException must stay in the chain - it is
the evidence #12902 needed");
```
`assertEquals` is already imported at line 43, so this needs no new import.
##########
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:
This test asserts less about its message than the ClassNotFound test asserts
about its own. Lines 113-118 check that the message names the reporter class
and the config to change; the mismatch format string emits both
(`MetricsReporterFactory.java:135-136`) but neither is checked here, so a
regression that dropped either would go unnoticed.
```suggestion
assertTrue(message.contains("different Hudi version"),
() -> "The failure should name version skew as the likely cause,
but was: " + message);
assertTrue(message.contains(MetricsReporterFactory.CLOUDWATCH_REPORTER_CLASS),
() -> "The failure should name the reporter class, but was: " +
message);
assertTrue(message.contains(HoodieMetricsConfig.METRICS_REPORTER_TYPE_VALUE.key()),
() -> "The failure should name the config to change, but was: " +
message);
```
The two added assertions survive any reword of the message. The `different
Hudi version` one does not - if you take the reword in my comment on
`MetricsReporterFactory.java:131`, replace it with whatever phrase you land on.
##########
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:
nit, feel free to ignore: this couples the test to the *other* branch's
prose. Reword the ClassNotFound message (say, to "could not be found on the
classpath") and this assertion goes vacuously true and stops discriminating -
and a vacuous `assertFalse` never fails, so nothing tells you it stopped
working. Asserting positively on this branch's own phrase is self-contained.
```suggestion
assertTrue(message.contains("was found on the classpath but"),
() -> "A class that resolved must not be reported as missing, but
was: " + message);
```
If you take this, drop the now-unused `assertFalse` import at line 44 or
checkstyle will flag it.
##########
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:
nit, feel free to ignore: the three mocked-static tests now repeat the same
four-line stub. Low value on its own, but it composes with the cause-assertion
suggestion above if you take that one, since the caller then owns the stubbed
exception and can assert on it directly:
```java
private HoodieException captureCloudWatchFailure(Throwable
reflectionFailure) {
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(reflectionFailure);
return assertThrows(HoodieException.class, () ->
MetricsReporterFactory.createReporter(metricsConfig, registry));
}
}
```
Do not fold the three into a `@ParameterizedTest` though - they assert
structurally different things (substring, substring plus absence, exact
equality), so a shared assertion block would read worse than the duplication
does.
##########
hudi-common/src/test/java/org/apache/hudi/metrics/TestMetricsReporterFactory.java:
##########
@@ -137,22 +139,51 @@ void
metricsReporterFactoryRewritesClassNotFoundIntoAnActionableMessage() {
}
Review Comment:
**Both ClassNotFound tests stop discriminating between the two branches once
this PR lands.**
`metricsReporterFactoryShouldExplainHowToEnableCloudWatchWhenHudiAwsIsMissing`
(lines 113-118) asserts the reporter class name, `hudi-aws-bundle`, and the
config key. This test (line 136) asserts `hudi-aws-bundle`. All of those
strings appear verbatim in the new constructor-mismatch message too, so both
tests pass unchanged against either message. The new test guards its own
direction with the `assertFalse` at line 163; nothing guards this one.
Please add a positive assertion on the distinguishing phrase to both tests,
e.g.
```java
assertTrue(message.contains("was not found on the classpath"),
() -> "A missing class must not be reported as a constructor mismatch,
but was: " + message);
```
so that a future merge or reorder of the two branches cannot ship green.
--
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]
