Support building Proxy Native via GraalVM CE for JDK 25#38682
Support building Proxy Native via GraalVM CE for JDK 25#38682linghengqian wants to merge 2 commits into
Conversation
c7606e0 to
540a6f5
Compare
4aa7c04 to
5874874
Compare
terrymanu
left a comment
There was a problem hiding this comment.
Decision
Merge Verdict: Not Mergeable
Reviewed Scope: latest PR head 5874874; .github/workflows, distribution/proxy-native, pom.xml, proxy/backend/core, test/native, docs, and reachability metadata. No SQL parser changes were detected. Need Expert Review: GraalVM/native-image and dependency compatibility.
Not Reviewed Scope: I did not run the full native-image suite locally; I relied on GitHub CI/API evidence and local diff inspection.
Positive Feedback
The direction is reasonable: the PR targets the real native-image path, updates the GraalVM build/test version, and the latest GraalVM rerun shows Ubuntu and Windows success for JDK 25.0.2.
Major Issues
[P0] Style gate is currently failing.
git diff --check fails with trailing whitespace in ProxyTestingServer.java. This violates the project gate in CODE_OF_CONDUCT.md (line 17), which requires coding standards and build checks to pass. Please run Spotless and recheck.
[P1] Shared config lookup precedence changed without a counterexample test.
ProxyConfigurationLoader#getResourceFile now prefers an existing filesystem path before classpath lookup. That is likely the right root-cause direction for #33206, but it changes shared Proxy startup/config-loading semantics used by normal Proxy bootstrap, e2e embedded Proxy, and native tests. I did not find a new test covering same-name/shadowing or fallback behavior. Please add direct validation for filesystem-first behavior, classpath fallback, and an adjacent counterexample where a classpath resource remains selected when no filesystem path exists.
Newly Introduced Issues
[P1] ZookeeperTest can leak the data source on failure paths.
The PR moves ResourceUtils.closeJdbcDataSource(logicDataSource) from @AfterEach into the happy path inside the TestingServer block in ZookeeperTest.java. If initEnvironment, processSuccess, or cleanEnvironment throws, the datasource is not closed before ZooKeeper is closed, which can reproduce the same shutdown-order risk this change is trying to avoid. Please use try/finally so close always happens before the TestingServer is closed.
Unrelated Changes
Please roll back or split the HiveServer2 doc change unless it is tied to this PR with evidence. The PR removes the commons-text exclusion in the user docs, but test/native/pom.xml still excludes commons-text; the PR body discusses commons-compress, not commons-text.
Verification
Commands run:
git fetch apache +5874874... +579de83...: exit 0
git diff --name-status apache/pr-38682-base..apache/pr-38682: exit 0
git diff --check apache/pr-38682-base..apache/pr-38682: exit 2, trailing whitespace found
git diff -G 'computeIfAbsent|ConcurrentHashMap' ...: exit 0, no hot-path computeIfAbsent change found
GitHub API checks: latest GraalVM rerun success on Ubuntu/Windows; earlier Windows run failed but was superseded by a later success
Next Step
Please fix the whitespace, add lookup-precedence/counterexample tests for ProxyConfigurationLoader, make ZookeeperTest cleanup exception-safe, and roll back or justify the unrelated HiveServer2 documentation change. Then this PR is worth another focused review.
linghengqian
left a comment
There was a problem hiding this comment.
Well, from my personal perspective, I can separate some things out.
[P0] Style gate is currently failing.
This seems to be an illusion caused by the agent you're using; neither CI nor I noticed the problem. As far as I remember, all files should have a blank line at the bottom.
[P1] Shared config lookup precedence changed without a counterexample test.
Adding unit tests to the current PR isn't very convenient for viewing, given GitHub's less-than-ergonomic web UI. I've split it into #38721. #38721 is the previous PR of this PR.
[P1] ZookeeperTest can leak the data source on failure paths.
This was intentional. If this native test fails while other native tests succeed, GraalVM Reachability Metadata should not be generated or the native test should be considered successful. GraalVM Native Build Tools simply did not consider this scenario of allowing failed unit tests in the unit test suite.
Please roll back or split the HiveServer2 doc change unless it is tied to this PR with evidence. The PR removes the commons-text exclusion in the user docs, but test/native/pom.xml still excludes commons-text; the PR body discusses commons-compress, not commons-text.
I split it at #38722 . This is just a dependency issue I'm verifying while working on the current PR.
terrymanu
left a comment
There was a problem hiding this comment.
Decision
- Merge Verdict: Not Mergeable
- Reviewed Scope: Latest PR head
587487419d1ca85903318004f43d0b2ccb377fb1againstapache/master; workflows, proxy-native Dockerfiles,pom.xml, GraalVM filter/metadata files at diff level,ProxyConfigurationLoader, native test utility/tests, and changed docs. - Not Reviewed Scope: Full line-by-line validation of every generated reachability metadata deletion; local GraalVM native-image build; full dependency vulnerability/license scan. No SQL parser files were touched.
- Need Expert Review: Native image/GraalVM metadata maintainer review is still needed; dependency/supply-chain review is recommended for the ClickHouse JDBC version bump.
Positive Feedback
- The direction of avoiding classpath lookup for existing filesystem paths is aligned with the reported native-image resource-matching problem.
- Latest GitHub check-run scan for the head commit showed 83 latest unique check names with no non-success result.
Major Issues
-
[Required] Make ZooKeeper datasource cleanup exception-safe (
test/native/src/test/java/org/apache/shardingsphere/test/natived/jdbc/modes/cluster/ZookeeperTest.java:64)- Symptom: The datasource is closed only after
initEnvironment(),processSuccess(), andcleanEnvironment()all succeed;afterEach()now only clears the system property. - Risk: If any statement before the close fails,
TestingServeris closed whilelogicDataSource/ ContextManager may remain open, so the original "requests to ZooKeeper after server shutdown" path is still reachable on the failure path. - Action: Please close
logicDataSourcein afinallyblock inside theTestingServerscope so it always closes before the ZooKeeper server, and keep or expand validation for the failure path.
- Symptom: The datasource is closed only after
-
[Required] Add counterexample coverage for filesystem-first config lookup (
proxy/backend/core/src/main/java/org/apache/shardingsphere/proxy/backend/config/ProxyConfigurationLoader.java:99)- Symptom:
getResourceFilenow returns an existing filesystem path before checking classpath and changes URL conversion tonew File(url.toURI()), but this PR does not changeProxyConfigurationLoaderTest. - Risk: This is a lookup-precedence change shared by Proxy startup and native tests; without a same-name/shadowing or classpath-fallback counterexample, regressions in classpath config loading or encoded filesystem paths can slip through.
- Action: Please add tests covering filesystem path wins when it exists, classpath fallback when no local file exists, and compatible
server.yaml/config-*.yamlfallback under the new order.
- Symptom:
-
[Required] Fix formatting gate failure (
test/native/src/test/java/org/apache/shardingsphere/test/natived/commons/util/ProxyTestingServer.java:78)- Symptom:
git diff --check $(git merge-base apache/master apache/pr/38682) apache/pr/38682fails with trailing whitespace at this line. - Risk: This violates the style/Spotless gate and can block the required quality checks even though functional checks pass.
- Action: Please run
./mvnw spotless:apply -Pcheck -T1C, then recheck style.
- Symptom:
Unrelated Changes
- HiveServer2 optional-plugin doc cleanup belongs outside this PR (
docs/document/content/user-manual/shardingsphere-jdbc/optional-plugins/hiveserver2/_index.en.md:55)- Symptom: The PR removes the
commons-textexclusion from HiveServer2 docs, while the main goal is Proxy Native on JDK 25 and the PR description says this was split to #38722. - Risk: Merging the current head brings a separate documentation/dependency-guidance change together with the native-image upgrade, blurring review and rollback scope.
- Action: Please roll this back from this PR, or rebase this PR after the dedicated doc PR is merged so the latest diff contains only the intended JDK 25/native-image changes.
- Symptom: The PR removes the
Next Steps
- Make the ZooKeeper native test cleanup exception-safe.
- Add direct
ProxyConfigurationLoaderregression tests for the new lookup precedence and fallback paths. - Remove or rebase out the HiveServer2 doc-only change.
- Fix the trailing whitespace and rerun Spotless/checkstyle plus the relevant native/GraalVM CI path.
linghengqian
left a comment
There was a problem hiding this comment.
Waiting for #38721 to be reviewed.
4b6a16c to
855b6a3
Compare
Summary
Positive Feedback
Issues
Multi-Round Comparison
Review Details
|
There was a problem hiding this comment.
[P2] Add release note for user-visible native-image and ClickHouse changes (pom.xml:134)
- Is it really necessary to add a release note? The clickhouse jdbc driver is just a test dependency in the master branch. There are no changes to the sql parser in the integration test and unit test. And we do not prevent the use of old versions of graalvm ce to compile shardingsphere proxy native, refer to https://shardingsphere.apache.org/document/current/en/user-manual/shardingsphere-jdbc/graalvm-native-image/
Users can still use old versions of Oracle GraalVM such as
21.0.8-graalon SDKMAN! to build ShardingSphere’s GraalVM Native Image product. But this will cause the failure of building GraalVM Native Image when integrating some third-party dependencies.
[P2] Make ZooKeeper native-test cleanup failure-path safe (test/native/src/test/java/org/apache/shardingsphere/test/natived/jdbc/modes/cluster/ZookeeperTest.java:64)
Closing the logical data source of shardingsphere after closing the zookeeper server will destroy the unit test. This conflicts with the processing of shardingsphere jdbc, because closing the logical data source of shardingsphere requires connecting to the zookeeper server.
Java's finally keyword has no meaning here.
For #29052 .
Changes proposed in this pull request:
Unsafeon JDK 25; consequently, ShardingSphere Proxy has slowed down, exposing issues that had previously gone unnoticed.org.apache.shardingsphere.test.natived.commons.util.ProxyTestingServerto use fix from Reset proxy backend executor across repeated ShardingSphereProxy lifecycles #38664 . Also fixes Completely prevent GraalVM Tracing Agent from collecting unreasonable GRM for unit tests related toorg.apache.shardingsphere.test.natived.proxy.features.**#33206 .test/native/native-image-filter/extra-filter.jsonfrom grabbing the GraalVM Reachability Metadata ofjava.lang.**. Compared with GraalVM CE For JDK 24.0.2, GraalVM CE For JDK 25.0.2 has constraints on more JDK own classes. If our grab part of the GRM ofjava.lang.**, the native image will fail to build.org.apache.commons:commons-compressdependency conflict withorg.apache.shardingsphere:shardingsphere-test-native. This is related to [repo] Dependencies version update ClickHouse/clickhouse-java#2696 .org.apache.shardingsphere.test.natived.jdbc.modes.cluster.ZookeeperTestwould still make requests to the ZooKeeper server via the ContextManager after the ZooKeeper server was shut down. This was actually an effect of a recent change introduced in the ShardingSphere master branch, which now attempts to access the ZooKeeper server when the ContextManager is shut down."-DjvmArgs=-XX:MaxRAMPercentage=85.0"from the CI.Before committing this PR, I'm sure that I have checked the following options:
./mvnw clean install -B -T1C -Dmaven.javadoc.skip -Dmaven.jacoco.skip -e.