Marton Greber has posted comments on this change. ( http://gerrit.cloudera.org:8080/24293 )
Change subject: [wip][java] Add JDK25 support ...................................................................... Patch Set 2: (3 comments) http://gerrit.cloudera.org:8080/#/c/24293/2/java/kudu-client/src/test/java/org/apache/kudu/client/TestSecurity.java File java/kudu-client/src/test/java/org/apache/kudu/client/TestSecurity.java: http://gerrit.cloudera.org:8080/#/c/24293/2/java/kudu-client/src/test/java/org/apache/kudu/client/TestSecurity.java@516 PS2, Line 516: // Error text varies across JDK/Kerberos implementations, but any q: By removing the assertion on the exception message, this catch now accepts *any* NonRecoverableException -- including ones caused by unexpected failures (wrong port, protocol mismatch, timeout classified as non-recoverable). Would it be safer to assert on a broader pattern (e.g. message contains "auth" or "SASL" or "Kerberos" or "GSS") so the test still fails if the connection is rejected for an unrelated reason? http://gerrit.cloudera.org:8080/#/c/24293/2/java/kudu-spark/src/main/scala/org/apache/kudu/spark/kudu/KuduContext.scala File java/kudu-spark/src/main/scala/org/apache/kudu/spark/kudu/KuduContext.scala: http://gerrit.cloudera.org:8080/#/c/24293/2/java/kudu-spark/src/main/scala/org/apache/kudu/spark/kudu/KuduContext.scala@589 PS2, Line 589: val subject = try { q: The catch clauses handle NoSuchMethodException, UnsupportedOperationException, and InvocationTargetException wrapping UnsupportedOperationException -- but `getDeclaredMethod("current").invoke(null)` can also throw IllegalAccessException or SecurityException. On a JDK that has the method but blocks reflective access (e.g. strong encapsulation without --add-opens), this would escape unhandled and crash the caller. Would it be safer to catch ReflectiveOperationException broadly and fall back to the old path for any reflection failure? http://gerrit.cloudera.org:8080/#/c/24293/2/java/kudu-test-utils/src/main/java/org/apache/kudu/test/cluster/FakeDNS.java File java/kudu-test-utils/src/main/java/org/apache/kudu/test/cluster/FakeDNS.java: http://gerrit.cloudera.org:8080/#/c/24293/2/java/kudu-test-utils/src/main/java/org/apache/kudu/test/cluster/FakeDNS.java@111 PS2, Line 111: } catch (ReflectiveOperationException | RuntimeException modernError) { Catching RuntimeException here is quite broad -- this will swallow NPEs, ClassCastExceptions, and other programming errors in the Java 8 path, treating them as "this JDK doesn't support this approach, try the next." That could make debugging painful when a bug in the reflection logic causes a NullPointerException, which then silently falls through to the JDK25 path (which also fails), then to the Java 8 path (which also fails), producing a confusing chained error. Consider narrowing to specific RuntimeExceptions you actually expect (e.g. InaccessibleObjectException), or at least logging when falling through due to a RuntimeException. -- To view, visit http://gerrit.cloudera.org:8080/24293 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I9d3835c9289ac9817c39a86b7be22a28818c5bb0 Gerrit-Change-Number: 24293 Gerrit-PatchSet: 2 Gerrit-Owner: Zoltan Chovan <[email protected]> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Marton Greber <[email protected]> Gerrit-Comment-Date: Wed, 08 Jul 2026 12:04:20 +0000 Gerrit-HasComments: Yes
