RohanExploit commented on PR #11823: URL: https://github.com/apache/seatunnel/pull/11823#issuecomment-5343391546
Thanks @DanielLeens, and no need to frame Issue 1 as your miss. You were right that the narrow two type catch was a coverage gap, and you are right now that `LinkageError` is broader than "version mismatch". Both things are true, the second only shows up once the first is fixed. All three items are addressed at `a0f6014`. Issue 1. I took Option A rather than Option B, and I want to explain why since you offered both. Narrowing back to the four mismatch types would let `UnsatisfiedLinkError` and friends escape unwrapped, which puts those users back in exactly the position #10172 describes, a bare linkage error out of Hadoop internals with nothing pointing anywhere. The problem was never that the catch was too wide, it was that the message asserted one cause with more confidence than the catch could justify. So the catch still covers the whole family and the message now says "may indicate", names the other realistic possibilities, and points at the cause. Combining that with Issue 3 does most of the work, because the concrete error is now in the message itself: ``` Failed to initialize Hadoop FileSystem (UnsatisfiedLinkError: no hadoop in java.library.path). This may indicate a Hadoop client version mismatch between SeaTunnel and the Hadoop jars resolved at runtime (...), or another classpath or linkage problem such as a missing native library or an unreadable class file. This connector is built and tested against Hadoop 3.1.4 (see seatunnel-hadoop3-3.1.4-uber in the reactor pom); check that the Hadoop client jars on the classpath are compatible with that version, and see the cause below for the specific failure. ``` A native library failure now reads as a native library failure while still getting the classpath framing, which neither option gave on its own. If you would still rather have the narrow catch I will switch it, it is a small change. Issue 2. The version moved into `TARGET_HADOOP_VERSION`, and the uber artifact name is derived from it as `"seatunnel-hadoop3-" + TARGET_HADOOP_VERSION + "-uber"` so the two cannot drift apart. Grepping the file for `3.1.4` now returns exactly one hit, the constant. Issue 3. Done via a small `describeLinkageError` helper that renders `Type: message`, falling back to just the type when the error carries no message, so the text never contains a stray "null". Tests are up to 13, two new ones: * `testWrapClasspathMismatchNamesTheConcreteLinkageFailure` throws an `UnsatisfiedLinkError` and asserts both the type and its message appear in the rewritten text. This is the one that covers the non mismatch subtypes you raised. * `testWrapClasspathMismatchHandlesLinkageErrorWithoutMessage` throws a `NoClassDefFoundError` with no message and asserts the type is still named and the text does not contain "null". The shared `assertRewrittenAsDiagnosticIOException` helper now also asserts the concrete type is named, so all four original rewrite tests cover the inlining too. I checked the new tests actually bite rather than assuming it. Dropping the null guard from `describeLinkageError` fails `testWrapClasspathMismatchHandlesLinkageErrorWithoutMessage` with `expected: <false> but was: <true>`, then passes again once restored. `spotless:check` is clean and all 13 tests pass locally on JDK 11, with the one pre-existing Windows only skip. -- 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]
