You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@hudi.apache.org by "rangareddy (via GitHub)" <gi...@apache.org> on 2026/08/03 09:27:31 UTC
[PR] fix(metrics): tell a CloudWatch reporter version mismatch apart from a missing bundle [hudi]
rangareddy opened a new pull request, #19477:
URL: https://github.com/apache/hudi/pull/19477
### Describe the issue this Pull Request addresses
Closes #12902. Enabling the CloudWatch reporter fails with nothing but:
```
org.apache.hudi.exception.HoodieException: Unable to instantiate class
org.apache.hudi.metrics.cloudwatch.CloudWatchMetricsReporter
```
The real cause is several frames down:
```
java.lang.NoSuchMethodException: org.apache.hudi.metrics.cloudwatch.CloudWatchMetricsReporter.<init>(
org.apache.hudi.config.metrics.HoodieMetricsConfig, com.codahale.metrics.MetricRegistry)
at java.lang.Class.getConstructor0(Class.java:3110)
at org.apache.hudi.common.util.ReflectionUtils.loadClass(ReflectionUtils.java:73)
```
The class resolved — so `hudi-aws` *is* on the classpath — but its constructor did not match, because the
jar providing it was built against a different Hudi version. Working that out from the reported message took
a maintainer digging the buried `NoSuchMethodException` out of the stack and pointing at the AWS SDK version
matrix; the message itself says only that instantiation failed.
`ReflectionUtils.loadClass` collapses `InstantiationException`, `IllegalAccessException`,
`InvocationTargetException` and `NoSuchMethodException` into one `HoodieException` with a fixed message, and
notes as much in its own TODO. #19418 added a translation for the missing-module case
(`ClassNotFoundException`) but deliberately left every other cause untouched, which is why this one still
surfaces bare.
### Summary and Changelog
- `MetricsReporterFactory#createCloudWatchReporter` now also translates a `NoSuchMethodException` cause,
reporting that the class was found but has no `(HoodieMetricsConfig, MetricRegistry)` constructor, that
the likely cause is a jar built against a different Hudi version, and that the remedy is a matching
`hudi-aws-bundle` — as distinct from the missing-module case, which says to *add* the bundle. Getting
those two the wrong way round is what makes the current message unhelpful: absent and mismatched need
different fixes.
- Tests: adds `metricsReporterFactoryExplainsAConstructorMismatch`, and renames
`metricsReporterFactoryLeavesNonClassNotFoundFailuresUntouched` to
`metricsReporterFactoryLeavesOtherFailuresUntouched`. That test used a `NoSuchMethodException` cause to
stand for "some other failure", which is now a handled case, so it uses an `InvocationTargetException`
instead — a failure raised by the reporter's own constructor, which is the case that must still pass
through untranslated.
### Verification
With the new branch reverted, exactly one test fails, and it reproduces the reported message verbatim:
```
[ERROR] metricsReporterFactoryExplainsAConstructorMismatch
AssertionFailedError: The failure should say the constructor did not match, but was: Unable to
instantiate class org.apache.hudi.aws.metrics.cloudwatch.CloudWatchMetricsReporter
```
`metricsReporterFactoryLeavesOtherFailuresUntouched` stays green in that run, which is the point of keeping
it: it shows the pass-through branch is genuinely exercised and not accidentally absorbed by the new one.
`TestMetricsReporterFactory` runs 12 tests green, the whole `org.apache.hudi.metrics` package 21, with
`checkstyle:check` and `apache-rat:check` clean.
### Impact
Error text only. Nothing that used to succeed behaves differently, and a failure that is neither a missing
class nor a missing constructor is still reported exactly as before.
Worth noting for whoever picks up `ReflectionUtils`: its TODO ("ReflectionUtils should throw a specific
exception to indicate Reflection problem") is the general fix, and would remove the need for
cause-sniffing here and anywhere else that wants to tell these failures apart. Not attempted in this PR —
`loadClass` has many callers and changing what it throws is a much larger change than the one this issue
needs.
### Risk Level
none — one additional branch on an existing error path, plus tests.
### Documentation Update
none
### Contributor's checklist
- [x] Read through [contributor's guide](https://hudi.apache.org/contribute/how-to-contribute)
- [x] Enough context is provided in the sections above
- [x] Adequate tests were added if applicable
- [x] CI passes on my PR
--
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: commits-unsubscribe@hudi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix(metrics): tell a CloudWatch reporter version mismatch apart from a missing bundle [hudi]
Posted by "rangareddy (via GitHub)" <gi...@apache.org>.
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: commits-unsubscribe@hudi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix(metrics): route the reflection failures a CloudWatch skew actually produces [hudi]
Posted by "hudi-bot (via GitHub)" <gi...@apache.org>.
hudi-bot commented on PR #19477:
URL: https://github.com/apache/hudi/pull/19477#issuecomment-5177991750
<!--
Meta data
{
"version" : 1,
"metaDataEntries" : [ {
"hash" : "e0ed1438ac1cbe7cfaef6a90737a55343c75388f",
"status" : "DELETED",
"url" : "https://dev.azure.com/apachehudi/a1a51da7-8592-47d4-88dc-fd67bed336bb/_build/results?buildId=15981",
"triggerID" : "e0ed1438ac1cbe7cfaef6a90737a55343c75388f",
"triggerType" : "PUSH"
}, {
"hash" : "ab7da92714849543927ad4ca9aded895d18a4d53",
"status" : "DELETED",
"url" : "https://dev.azure.com/apachehudi/a1a51da7-8592-47d4-88dc-fd67bed336bb/_build/results?buildId=15987",
"triggerID" : "ab7da92714849543927ad4ca9aded895d18a4d53",
"triggerType" : "PUSH"
}, {
"hash" : "1708133afa465ead0dd01e864e31c37d8a94042c",
"status" : "SUCCESS",
"url" : "https://dev.azure.com/apachehudi/a1a51da7-8592-47d4-88dc-fd67bed336bb/_build/results?buildId=16036",
"triggerID" : "1708133afa465ead0dd01e864e31c37d8a94042c",
"triggerType" : "PUSH"
} ]
}-->
## CI report:
* 1708133afa465ead0dd01e864e31c37d8a94042c Azure: [SUCCESS](https://dev.azure.com/apachehudi/a1a51da7-8592-47d4-88dc-fd67bed336bb/_build/results?buildId=16036)
<details>
<summary>Bot commands</summary>
@hudi-bot supports the following commands:
- `@hudi-bot run azure` re-run the last Azure build
</details>
--
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: commits-unsubscribe@hudi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix(metrics): tell a CloudWatch reporter version mismatch apart from a missing bundle [hudi]
Posted by "hudi-bot (via GitHub)" <gi...@apache.org>.
hudi-bot commented on PR #19477:
URL: https://github.com/apache/hudi/pull/19477#issuecomment-5164744364
<!--
Meta data
{
"version" : 1,
"metaDataEntries" : [ {
"hash" : "e0ed1438ac1cbe7cfaef6a90737a55343c75388f",
"status" : "PENDING",
"url" : "https://dev.azure.com/apachehudi/a1a51da7-8592-47d4-88dc-fd67bed336bb/_build/results?buildId=15981",
"triggerID" : "e0ed1438ac1cbe7cfaef6a90737a55343c75388f",
"triggerType" : "PUSH"
} ]
}-->
## CI report:
* e0ed1438ac1cbe7cfaef6a90737a55343c75388f Azure: [PENDING](https://dev.azure.com/apachehudi/a1a51da7-8592-47d4-88dc-fd67bed336bb/_build/results?buildId=15981)
<details>
<summary>Bot commands</summary>
@hudi-bot supports the following commands:
- `@hudi-bot run azure` re-run the last Azure build
</details>
--
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: commits-unsubscribe@hudi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix(metrics): tell a CloudWatch reporter version mismatch apart from a missing bundle [hudi]
Posted by "hudi-bot (via GitHub)" <gi...@apache.org>.
hudi-bot commented on PR #19477:
URL: https://github.com/apache/hudi/pull/19477#issuecomment-5170935405
<!--
Meta data
{
"version" : 1,
"metaDataEntries" : [ {
"hash" : "e0ed1438ac1cbe7cfaef6a90737a55343c75388f",
"status" : "SUCCESS",
"url" : "https://dev.azure.com/apachehudi/a1a51da7-8592-47d4-88dc-fd67bed336bb/_build/results?buildId=15981",
"triggerID" : "e0ed1438ac1cbe7cfaef6a90737a55343c75388f",
"triggerType" : "PUSH"
}, {
"hash" : "ab7da92714849543927ad4ca9aded895d18a4d53",
"status" : "PENDING",
"url" : "https://dev.azure.com/apachehudi/a1a51da7-8592-47d4-88dc-fd67bed336bb/_build/results?buildId=15987",
"triggerID" : "ab7da92714849543927ad4ca9aded895d18a4d53",
"triggerType" : "PUSH"
} ]
}-->
## CI report:
* e0ed1438ac1cbe7cfaef6a90737a55343c75388f Azure: [SUCCESS](https://dev.azure.com/apachehudi/a1a51da7-8592-47d4-88dc-fd67bed336bb/_build/results?buildId=15981)
* ab7da92714849543927ad4ca9aded895d18a4d53 Azure: [PENDING](https://dev.azure.com/apachehudi/a1a51da7-8592-47d4-88dc-fd67bed336bb/_build/results?buildId=15987)
<details>
<summary>Bot commands</summary>
@hudi-bot supports the following commands:
- `@hudi-bot run azure` re-run the last Azure build
</details>
--
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: commits-unsubscribe@hudi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix(metrics): tell a CloudWatch reporter version mismatch apart from a missing bundle [hudi]
Posted by "codecov-commenter (via GitHub)" <gi...@apache.org>.
codecov-commenter commented on PR #19477:
URL: https://github.com/apache/hudi/pull/19477#issuecomment-5165985453
## [Codecov](https://app.codecov.io/gh/apache/hudi/pull/19477?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
:x: Patch coverage is `0%` with `4 lines` in your changes missing coverage. Please review.
:white_check_mark: Project coverage is 71.47%. Comparing base ([`d98f2f1`](https://app.codecov.io/gh/apache/hudi/commit/d98f2f1b4752845167527a69f1912d6ad4980921?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)) to head ([`ab7da92`](https://app.codecov.io/gh/apache/hudi/commit/ab7da92714849543927ad4ca9aded895d18a4d53?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)).
:warning: Report is 1 commits behind head on master.
| [Files with missing lines](https://app.codecov.io/gh/apache/hudi/pull/19477?dropdown=coverage&src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Patch % | Lines |
|---|---|---|
| [...rg/apache/hudi/metrics/MetricsReporterFactory.java](https://app.codecov.io/gh/apache/hudi/pull/19477?src=pr&el=tree&filepath=hudi-common%2Fsrc%2Fmain%2Fjava%2Forg%2Fapache%2Fhudi%2Fmetrics%2FMetricsReporterFactory.java&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-aHVkaS1jb21tb24vc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL2h1ZGkvbWV0cmljcy9NZXRyaWNzUmVwb3J0ZXJGYWN0b3J5LmphdmE=) | 0.00% | [4 Missing :warning: ](https://app.codecov.io/gh/apache/hudi/pull/19477?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
> :exclamation: There is a different number of reports uploaded between BASE (d98f2f1) and HEAD (ab7da92). Click for more details.
>
> <details><summary>HEAD has 14 uploads less than BASE</summary>
>
>| Flag | BASE (d98f2f1) | HEAD (ab7da92) |
>|------|------|------|
>|spark-scala-tests|14|1|
>|flink-integration-tests|2|1|
></details>
<details><summary>Additional details and impacted files</summary>
```diff
@@ Coverage Diff @@
## master #19477 +/- ##
============================================
- Coverage 76.96% 71.47% -5.50%
+ Complexity 33850 31997 -1853
============================================
Files 2575 2575
Lines 143372 143382 +10
Branches 17572 17574 +2
============================================
- Hits 110349 102483 -7866
- Misses 24758 33257 +8499
+ Partials 8265 7642 -623
```
| [Components](https://app.codecov.io/gh/apache/hudi/pull/19477/components?src=pr&el=components&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Coverage Δ | |
|---|---|---|
| [hudi-common](https://app.codecov.io/gh/apache/hudi/pull/19477/components?src=pr&el=component&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `81.47% <0.00%> (-0.81%)` | :arrow_down: |
| [hudi-client](https://app.codecov.io/gh/apache/hudi/pull/19477/components?src=pr&el=component&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `80.67% <ø> (-1.15%)` | :arrow_down: |
| [hudi-flink](https://app.codecov.io/gh/apache/hudi/pull/19477/components?src=pr&el=component&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `82.49% <ø> (-1.48%)` | :arrow_down: |
| [hudi-spark-datasource](https://app.codecov.io/gh/apache/hudi/pull/19477/components?src=pr&el=component&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `48.19% <ø> (-26.92%)` | :arrow_down: |
| [hudi-utilities](https://app.codecov.io/gh/apache/hudi/pull/19477/components?src=pr&el=component&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `73.63% <ø> (ø)` | |
| [hudi-cli](https://app.codecov.io/gh/apache/hudi/pull/19477/components?src=pr&el=component&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `15.32% <ø> (ø)` | |
| [hudi-hadoop](https://app.codecov.io/gh/apache/hudi/pull/19477/components?src=pr&el=component&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `62.80% <ø> (-0.70%)` | :arrow_down: |
| [hudi-sync](https://app.codecov.io/gh/apache/hudi/pull/19477/components?src=pr&el=component&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `70.82% <ø> (-0.06%)` | :arrow_down: |
| [hudi-io](https://app.codecov.io/gh/apache/hudi/pull/19477/components?src=pr&el=component&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `79.60% <100.00%> (ø)` | |
| [hudi-timeline-service](https://app.codecov.io/gh/apache/hudi/pull/19477/components?src=pr&el=component&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `83.44% <ø> (-0.30%)` | :arrow_down: |
| [hudi-cloud](https://app.codecov.io/gh/apache/hudi/pull/19477/components?src=pr&el=component&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `64.00% <ø> (ø)` | |
| [hudi-kafka-connect](https://app.codecov.io/gh/apache/hudi/pull/19477/components?src=pr&el=component&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `53.20% <ø> (ø)` | |
| [Flag](https://app.codecov.io/gh/apache/hudi/pull/19477/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Coverage Δ | |
|---|---|---|
| [flink-integration-tests](https://app.codecov.io/gh/apache/hudi/pull/19477/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `47.84% <0.00%> (-0.96%)` | :arrow_down: |
| [hadoop-mr-java-client](https://app.codecov.io/gh/apache/hudi/pull/19477/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `43.77% <0.00%> (+<0.01%)` | :arrow_up: |
| [spark-client-hadoop-common](https://app.codecov.io/gh/apache/hudi/pull/19477/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `48.67% <0.00%> (-0.01%)` | :arrow_down: |
| [spark-scala-tests](https://app.codecov.io/gh/apache/hudi/pull/19477/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `32.58% <0.00%> (-14.86%)` | :arrow_down: |
Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#carryforward-flags-in-the-pull-request-comment) to find out more.
| [Files with missing lines](https://app.codecov.io/gh/apache/hudi/pull/19477?dropdown=coverage&src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Coverage Δ | |
|---|---|---|
| [...rg/apache/hudi/metrics/MetricsReporterFactory.java](https://app.codecov.io/gh/apache/hudi/pull/19477?src=pr&el=tree&filepath=hudi-common%2Fsrc%2Fmain%2Fjava%2Forg%2Fapache%2Fhudi%2Fmetrics%2FMetricsReporterFactory.java&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-aHVkaS1jb21tb24vc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL2h1ZGkvbWV0cmljcy9NZXRyaWNzUmVwb3J0ZXJGYWN0b3J5LmphdmE=) | `70.83% <0.00%> (-21.28%)` | :arrow_down: |
... and [355 files with indirect coverage changes](https://app.codecov.io/gh/apache/hudi/pull/19477/indirect-changes?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)
</details>
<details><summary> :rocket: New features to boost your workflow: </summary>
- :snowflake: [Test Analytics](https://docs.codecov.com/docs/test-analytics): Detect flaky tests, report on failures, and find test suite problems.
- :package: [JS Bundle Analysis](https://docs.codecov.com/docs/javascript-bundle-analysis): Save yourself from yourself by tracking and limiting bundle sizes in JS merges.
</details>
--
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: commits-unsubscribe@hudi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix(metrics): tell a CloudWatch reporter version mismatch apart from a missing bundle [hudi]
Posted by "hudi-agent (via GitHub)" <gi...@apache.org>.
hudi-agent commented on code in PR #19477:
URL: https://github.com/apache/hudi/pull/19477#discussion_r3702885321
##########
hudi-common/src/test/java/org/apache/hudi/metrics/TestMetricsReporterFactory.java:
##########
@@ -137,22 +139,50 @@ 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)) {
+ 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 InvocationTargetException(new IllegalStateException("no AWS region configured"))));
+
HoodieException exception = assertThrows(HoodieException.class,
() -> MetricsReporterFactory.createReporter(metricsConfig, registry));
Review Comment:
🤖 nit: "neither cause" reads like a dropped word — could you clarify it to something like `"A failure that is neither a missing class nor a missing constructor must not be rewritten"`?
<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: commits-unsubscribe@hudi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix(metrics): tell a CloudWatch reporter version mismatch apart from a missing bundle [hudi]
Posted by "hudi-bot (via GitHub)" <gi...@apache.org>.
hudi-bot commented on PR #19477:
URL: https://github.com/apache/hudi/pull/19477#issuecomment-5164946250
<!--
Meta data
{
"version" : 1,
"metaDataEntries" : [ {
"hash" : "e0ed1438ac1cbe7cfaef6a90737a55343c75388f",
"status" : "PENDING",
"url" : "https://dev.azure.com/apachehudi/a1a51da7-8592-47d4-88dc-fd67bed336bb/_build/results?buildId=15981",
"triggerID" : "e0ed1438ac1cbe7cfaef6a90737a55343c75388f",
"triggerType" : "PUSH"
}, {
"hash" : "ab7da92714849543927ad4ca9aded895d18a4d53",
"status" : "UNKNOWN",
"url" : "TBD",
"triggerID" : "ab7da92714849543927ad4ca9aded895d18a4d53",
"triggerType" : "PUSH"
} ]
}-->
## CI report:
* e0ed1438ac1cbe7cfaef6a90737a55343c75388f Azure: [PENDING](https://dev.azure.com/apachehudi/a1a51da7-8592-47d4-88dc-fd67bed336bb/_build/results?buildId=15981)
* ab7da92714849543927ad4ca9aded895d18a4d53 UNKNOWN
<details>
<summary>Bot commands</summary>
@hudi-bot supports the following commands:
- `@hudi-bot run azure` re-run the last Azure build
</details>
--
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: commits-unsubscribe@hudi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix(metrics): tell a CloudWatch reporter version mismatch apart from a missing bundle [hudi]
Posted by "hudi-bot (via GitHub)" <gi...@apache.org>.
hudi-bot commented on PR #19477:
URL: https://github.com/apache/hudi/pull/19477#issuecomment-5175623246
<!--
Meta data
{
"version" : 1,
"metaDataEntries" : [ {
"hash" : "e0ed1438ac1cbe7cfaef6a90737a55343c75388f",
"status" : "DELETED",
"url" : "https://dev.azure.com/apachehudi/a1a51da7-8592-47d4-88dc-fd67bed336bb/_build/results?buildId=15981",
"triggerID" : "e0ed1438ac1cbe7cfaef6a90737a55343c75388f",
"triggerType" : "PUSH"
}, {
"hash" : "ab7da92714849543927ad4ca9aded895d18a4d53",
"status" : "SUCCESS",
"url" : "https://dev.azure.com/apachehudi/a1a51da7-8592-47d4-88dc-fd67bed336bb/_build/results?buildId=15987",
"triggerID" : "ab7da92714849543927ad4ca9aded895d18a4d53",
"triggerType" : "PUSH"
}, {
"hash" : "1708133afa465ead0dd01e864e31c37d8a94042c",
"status" : "UNKNOWN",
"url" : "TBD",
"triggerID" : "1708133afa465ead0dd01e864e31c37d8a94042c",
"triggerType" : "PUSH"
} ]
}-->
## CI report:
* ab7da92714849543927ad4ca9aded895d18a4d53 Azure: [SUCCESS](https://dev.azure.com/apachehudi/a1a51da7-8592-47d4-88dc-fd67bed336bb/_build/results?buildId=15987)
* 1708133afa465ead0dd01e864e31c37d8a94042c UNKNOWN
<details>
<summary>Bot commands</summary>
@hudi-bot supports the following commands:
- `@hudi-bot run azure` re-run the last Azure build
</details>
--
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: commits-unsubscribe@hudi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix(metrics): tell a CloudWatch reporter version mismatch apart from a missing bundle [hudi]
Posted by "rangareddy (via GitHub)" <gi...@apache.org>.
rangareddy commented on code in PR #19477:
URL: https://github.com/apache/hudi/pull/19477#discussion_r3703016233
##########
hudi-common/src/test/java/org/apache/hudi/metrics/TestMetricsReporterFactory.java:
##########
@@ -137,22 +139,50 @@ 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)) {
+ 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 InvocationTargetException(new IllegalStateException("no AWS region configured"))));
+
HoodieException exception = assertThrows(HoodieException.class,
() -> MetricsReporterFactory.createReporter(metricsConfig, registry));
Review Comment:
Fixed — it was a dropped word, exactly as you read it. Now:
```java
assertEquals("Unable to instantiate class " + MetricsReporterFactory.CLOUDWATCH_REPORTER_CLASS,
exception.getMessage(),
"A failure that is neither a missing class nor a missing constructor must not be rewritten");
```
`Tests run: 12, Failures: 0, Errors: 0`, checkstyle clean.
--
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: commits-unsubscribe@hudi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix(metrics): tell a CloudWatch reporter version mismatch apart from a missing bundle [hudi]
Posted by "hudi-bot (via GitHub)" <gi...@apache.org>.
hudi-bot commented on PR #19477:
URL: https://github.com/apache/hudi/pull/19477#issuecomment-5165334377
<!--
Meta data
{
"version" : 1,
"metaDataEntries" : [ {
"hash" : "e0ed1438ac1cbe7cfaef6a90737a55343c75388f",
"status" : "PENDING",
"url" : "https://dev.azure.com/apachehudi/a1a51da7-8592-47d4-88dc-fd67bed336bb/_build/results?buildId=15981",
"triggerID" : "e0ed1438ac1cbe7cfaef6a90737a55343c75388f",
"triggerType" : "PUSH"
}, {
"hash" : "ab7da92714849543927ad4ca9aded895d18a4d53",
"status" : "PENDING",
"url" : "https://dev.azure.com/apachehudi/a1a51da7-8592-47d4-88dc-fd67bed336bb/_build/results?buildId=15987",
"triggerID" : "ab7da92714849543927ad4ca9aded895d18a4d53",
"triggerType" : "PUSH"
} ]
}-->
## CI report:
* e0ed1438ac1cbe7cfaef6a90737a55343c75388f Azure: [PENDING](https://dev.azure.com/apachehudi/a1a51da7-8592-47d4-88dc-fd67bed336bb/_build/results?buildId=15981)
* ab7da92714849543927ad4ca9aded895d18a4d53 Azure: [PENDING](https://dev.azure.com/apachehudi/a1a51da7-8592-47d4-88dc-fd67bed336bb/_build/results?buildId=15987)
<details>
<summary>Bot commands</summary>
@hudi-bot supports the following commands:
- `@hudi-bot run azure` re-run the last Azure build
</details>
--
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: commits-unsubscribe@hudi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix(metrics): route the reflection failures a CloudWatch skew actually produces [hudi]
Posted by "voonhous (via GitHub)" <gi...@apache.org>.
voonhous merged PR #19477:
URL: https://github.com/apache/hudi/pull/19477
--
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: commits-unsubscribe@hudi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix(metrics): tell a CloudWatch reporter version mismatch apart from a missing bundle [hudi]
Posted by "hudi-bot (via GitHub)" <gi...@apache.org>.
hudi-bot commented on PR #19477:
URL: https://github.com/apache/hudi/pull/19477#issuecomment-5164683594
<!--
Meta data
{
"version" : 1,
"metaDataEntries" : [ {
"hash" : "e0ed1438ac1cbe7cfaef6a90737a55343c75388f",
"status" : "UNKNOWN",
"url" : "TBD",
"triggerID" : "e0ed1438ac1cbe7cfaef6a90737a55343c75388f",
"triggerType" : "PUSH"
} ]
}-->
## CI report:
* e0ed1438ac1cbe7cfaef6a90737a55343c75388f UNKNOWN
<details>
<summary>Bot commands</summary>
@hudi-bot supports the following commands:
- `@hudi-bot run azure` re-run the last Azure build
</details>
--
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: commits-unsubscribe@hudi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix(metrics): tell a CloudWatch reporter version mismatch apart from a missing bundle [hudi]
Posted by "voonhous (via GitHub)" <gi...@apache.org>.
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: commits-unsubscribe@hudi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix(metrics): route the reflection failures a CloudWatch skew actually produces [hudi]
Posted by "voonhous (via GitHub)" <gi...@apache.org>.
voonhous closed pull request #19477: fix(metrics): route the reflection failures a CloudWatch skew actually produces
URL: https://github.com/apache/hudi/pull/19477
--
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: commits-unsubscribe@hudi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix(metrics): tell a CloudWatch reporter version mismatch apart from a missing bundle [hudi]
Posted by "hudi-bot (via GitHub)" <gi...@apache.org>.
hudi-bot commented on PR #19477:
URL: https://github.com/apache/hudi/pull/19477#issuecomment-5175672167
<!--
Meta data
{
"version" : 1,
"metaDataEntries" : [ {
"hash" : "e0ed1438ac1cbe7cfaef6a90737a55343c75388f",
"status" : "DELETED",
"url" : "https://dev.azure.com/apachehudi/a1a51da7-8592-47d4-88dc-fd67bed336bb/_build/results?buildId=15981",
"triggerID" : "e0ed1438ac1cbe7cfaef6a90737a55343c75388f",
"triggerType" : "PUSH"
}, {
"hash" : "ab7da92714849543927ad4ca9aded895d18a4d53",
"status" : "SUCCESS",
"url" : "https://dev.azure.com/apachehudi/a1a51da7-8592-47d4-88dc-fd67bed336bb/_build/results?buildId=15987",
"triggerID" : "ab7da92714849543927ad4ca9aded895d18a4d53",
"triggerType" : "PUSH"
}, {
"hash" : "1708133afa465ead0dd01e864e31c37d8a94042c",
"status" : "PENDING",
"url" : "https://dev.azure.com/apachehudi/a1a51da7-8592-47d4-88dc-fd67bed336bb/_build/results?buildId=16036",
"triggerID" : "1708133afa465ead0dd01e864e31c37d8a94042c",
"triggerType" : "PUSH"
} ]
}-->
## CI report:
* ab7da92714849543927ad4ca9aded895d18a4d53 Azure: [SUCCESS](https://dev.azure.com/apachehudi/a1a51da7-8592-47d4-88dc-fd67bed336bb/_build/results?buildId=15987)
* 1708133afa465ead0dd01e864e31c37d8a94042c Azure: [PENDING](https://dev.azure.com/apachehudi/a1a51da7-8592-47d4-88dc-fd67bed336bb/_build/results?buildId=16036)
<details>
<summary>Bot commands</summary>
@hudi-bot supports the following commands:
- `@hudi-bot run azure` re-run the last Azure build
</details>
--
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: commits-unsubscribe@hudi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix(metrics): route the reflection failures a CloudWatch skew actually produces [hudi]
Posted by "voonhous (via GitHub)" <gi...@apache.org>.
voonhous closed pull request #19477: fix(metrics): route the reflection failures a CloudWatch skew actually produces
URL: https://github.com/apache/hudi/pull/19477
--
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: commits-unsubscribe@hudi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix(metrics): tell a CloudWatch reporter version mismatch apart from a missing bundle [hudi]
Posted by "hudi-bot (via GitHub)" <gi...@apache.org>.
hudi-bot commented on PR #19477:
URL: https://github.com/apache/hudi/pull/19477#issuecomment-5172059455
<!--
Meta data
{
"version" : 1,
"metaDataEntries" : [ {
"hash" : "e0ed1438ac1cbe7cfaef6a90737a55343c75388f",
"status" : "DELETED",
"url" : "https://dev.azure.com/apachehudi/a1a51da7-8592-47d4-88dc-fd67bed336bb/_build/results?buildId=15981",
"triggerID" : "e0ed1438ac1cbe7cfaef6a90737a55343c75388f",
"triggerType" : "PUSH"
}, {
"hash" : "ab7da92714849543927ad4ca9aded895d18a4d53",
"status" : "SUCCESS",
"url" : "https://dev.azure.com/apachehudi/a1a51da7-8592-47d4-88dc-fd67bed336bb/_build/results?buildId=15987",
"triggerID" : "ab7da92714849543927ad4ca9aded895d18a4d53",
"triggerType" : "PUSH"
} ]
}-->
## CI report:
* ab7da92714849543927ad4ca9aded895d18a4d53 Azure: [SUCCESS](https://dev.azure.com/apachehudi/a1a51da7-8592-47d4-88dc-fd67bed336bb/_build/results?buildId=15987)
<details>
<summary>Bot commands</summary>
@hudi-bot supports the following commands:
- `@hudi-bot run azure` re-run the last Azure build
</details>
--
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: commits-unsubscribe@hudi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org