You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@shardingsphere.apache.org by "linghengqian (via GitHub)" <gi...@apache.org> on 2026/05/10 09:13:01 UTC
[PR] Support building Proxy Native via GraalVM CE for JDK 25 [shardingsphere]
linghengqian opened a new pull request, #38682:
URL: https://github.com/apache/shardingsphere/pull/38682
For #29052 .
Changes proposed in this pull request:
- Support building Proxy Native via GraalVM CE for JDK 25. These are the key steps for using https://github.com/graalvm/native-build-tools/releases/tag/1.0.0 .
- Fixes `org.apache.commons:commons-compress` dependency conflict with `org.apache.shardingsphere:shardingsphere-test-native`. This is related to https://github.com/ClickHouse/clickhouse-java/pull/2696 .
- Fixes a bug where `org.apache.shardingsphere.test.natived.jdbc.modes.cluster.ZookeeperTest` would 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.
- Increase the timeout for detecting the ShardingSphere proxy in nativetest to 5 minutes. Also see https://github.com/netty/netty/wiki/Java-24-and-sun.misc.Unsafe . Netty no longer utilizes `Unsafe` on JDK 25; consequently, ShardingSphere Proxy has slowed down, exposing issues that had previously gone unnoticed. Also see https://github.com/apache/shardingsphere/issues/38664 .
---
Before committing this PR, I'm sure that I have checked the following options:
- [x] My code follows the [code of conduct](https://shardingsphere.apache.org/community/en/involved/conduct/code/) of this project.
- [x] I have self-reviewed the commit code.
- [x] I have (or in comment I request) added corresponding labels for the pull request.
- [x] I have passed maven check locally : `./mvnw clean install -B -T1C -Dmaven.javadoc.skip -Dmaven.jacoco.skip -e`.
- [x] I have made corresponding changes to the documentation.
- [x] I have added corresponding unit tests for my changes.
- [ ] I have updated the Release Notes of the current development version. For more details, see [Update Release Note](https://shardingsphere.apache.org/community/en/involved/contribute/contributor/)
--
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: notifications-unsubscribe@shardingsphere.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Support building Proxy Native via GraalVM CE for JDK 25 [shardingsphere]
Posted by "terrymanu (via GitHub)" <gi...@apache.org>.
terrymanu closed pull request #38682: Support building Proxy Native via GraalVM CE for JDK 25
URL: https://github.com/apache/shardingsphere/pull/38682
--
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: notifications-unsubscribe@shardingsphere.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Support building Proxy Native via GraalVM CE for JDK 25 [shardingsphere]
Posted by "terrymanu (via GitHub)" <gi...@apache.org>.
terrymanu commented on PR #38682:
URL: https://github.com/apache/shardingsphere/pull/38682#issuecomment-4753089948
Thanks for the explanation. I understand the motivation of this change: nativeTest repeatedly resets / closes the transaction manager, and after `ContextManager.close()`, the resource in `TransactionRule` is cleared, while the later JDBC close / cleanup path may still touch the transaction context.
My concern is not that this nativeTest scenario should not be handled. The concern is that the fix currently lands in the shared production transaction abstraction, `ConnectionTransaction`. With the current implementation, when XA/BASE sees `rule.getResource() == null`, `distributedTransactionManager` becomes `null`, and then `begin()` / `commit()` / `rollback()` silently no-op. This models the lifecycle state “transaction resource has already been closed” as “a valid XA/BASE transaction that simply does not need transaction operations.”
So I suggest separating the problem into two possible directions:
1. If this state only happens during nativeTest cleanup / shutdown, please try to keep the fix inside the cleanup boundary, for example by preserving the close ordering, avoiding post-close connections from re-entering transaction paths, or explicitly handling this shutdown-only case in the test utility layer.
2. If this state really needs to be accepted by production paths, then please do not model it implicitly with `null manager + no-op`. Instead, introduce an explicit closed / absent transaction resource semantics, and add tests for paths such as `setAutoCommit(false)`, `begin`, `commit`, and `rollback` to prove that behavior is intentional.
In other words, I am not asking for a simple rollback. I am asking that we avoid letting a nativeTest lifecycle sentinel become shared JDBC / Proxy transaction semantics.
For the ClickHouse release note, I understand that it is an optional driver and not a mandatory default dependency. The main point here is not to describe it as a major runtime change. The issue is that the current release note says `nativeTest`, while the diff changes the root managed version in `pom.xml`, and the Proxy / Proxy Native `db-clickhouse` runtime profiles consume that managed version. The user docs are also updated to `0.9.8`.
So this can be handled in a lightweight way: either reword the release note to say that the optional ClickHouse JDBC driver is updated to `0.9.8`, without limiting it to nativeTest; or, if the intention is truly to affect only nativeTest, please constrain the version change to the nativeTest dependency scope.
I think the first transaction-resource state issue is the one that needs to be settled first. The ClickHouse point is mainly about keeping the release note description consistent with the actual diff scope.
--
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: notifications-unsubscribe@shardingsphere.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Support building Proxy Native via GraalVM CE for JDK 25 [shardingsphere]
Posted by "terrymanu (via GitHub)" <gi...@apache.org>.
terrymanu commented on PR #38682:
URL: https://github.com/apache/shardingsphere/pull/38682#issuecomment-4528555270
Decision
Merge Verdict: Not Mergeable
Reviewed Scope: latest PR head 587487419d1ca85903318004f43d0b2ccb377fb1; .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](https://github.com/apache/shardingsphere/blob/587487419d1ca85903318004f43d0b2ccb377fb1/test/native/src/test/java/org/apache/shardingsphere/test/natived/commons/util/ProxyTestingServer.java#L78). 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](https://github.com/apache/shardingsphere/blob/587487419d1ca85903318004f43d0b2ccb377fb1/proxy/backend/core/src/main/java/org/apache/shardingsphere/proxy/backend/config/ProxyConfigurationLoader.java#L98-L104) 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](https://github.com/apache/shardingsphere/blob/587487419d1ca85903318004f43d0b2ccb377fb1/test/native/src/test/java/org/apache/shardingsphere/test/natived/jdbc/modes/cluster/ZookeeperTest.java#L57-L72). 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.
--
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: notifications-unsubscribe@shardingsphere.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Support building Proxy Native via GraalVM CE for JDK 25 [shardingsphere]
Posted by "terrymanu (via GitHub)" <gi...@apache.org>.
terrymanu merged PR #38682:
URL: https://github.com/apache/shardingsphere/pull/38682
--
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: notifications-unsubscribe@shardingsphere.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Support building Proxy Native via GraalVM CE for JDK 25 [shardingsphere]
Posted by "terrymanu (via GitHub)" <gi...@apache.org>.
terrymanu commented on PR #38682:
URL: https://github.com/apache/shardingsphere/pull/38682#issuecomment-4638920655
### Summary
- **Merge Decision: Not Mergeable**
- **Reason:** The latest head still has a failure-path cleanup gap and is missing a required release note for user-visible native-image/dependency changes.
- **Expert Review Needed:** Native-image/reachability metadata review is still needed after the blockers are fixed because this PR changes large generated metadata files and JDK 25 build behavior.
### Positive Feedback
- The latest scope removed the previous `ProxyConfigurationLoader` and HiveServer2-doc blockers from this PR.
- The generated JSON files parse successfully, and no SQL parser files are touched.
### Issues
- **[P2] Add release note for user-visible native-image and ClickHouse changes** (`pom.xml:134`)
- **Problem:** The PR changes the ClickHouse JDBC version to `0.9.8`, updates GraalVM native-image user docs to `25.0.2`, changes Proxy Native platform guidance, and updates Proxy Native Docker/workflow build versions, but `RELEASE-NOTES.md` is not in the latest GitHub file list.
- **Impact:** Users and operators will not see the upgrade impact for the GraalVM CE baseline, Proxy Native platform/build behavior, and ClickHouse optional dependency version.
- **Required Change:** Please add a `RELEASE-NOTES.md` entry, likely under `Enhancements`, describing the user-visible outcome and affected areas, or explicitly delegate the release note to an accepted umbrella PR.
- **[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`)
- **Problem:** `afterEach()` calls `ResourceUtils.closeJdbcDataSource(logicDataSource)` before `testingServer.close()` without a null guard or `finally`; if data source creation fails or data source close throws, the ZooKeeper `TestingServer` close is skipped.
- **Impact:** The previous shutdown-order issue is fixed on the normal path, but setup/cleanup failure paths can still leak the ZooKeeper test server and affect later native-test or metadata-generation runs.
- **Required Change:** Please make cleanup close both resources reliably while preserving data-source-before-ZooKeeper ordering when the data source exists, for example with null-safe cleanup and a `finally` for `testingServer.close()`.
### Multi-Round Comparison
- **Fixed:** The shared `ProxyConfigurationLoader` lookup-precedence blocker is no longer in this PR diff after the preceding merged change.
- **Fixed:** The HiveServer2 unrelated doc cleanup is no longer in the latest PR diff.
- **Partially fixed:** `ZookeeperTest` now closes the data source before ZooKeeper on the normal path, but the failure path remains unresolved and is tracked above.
- **Verification note:** `git diff --check` still reports trailing whitespace in `ProxyTestingServer.java:78` and `ZookeeperTest.java:54`; I did not treat this as a standalone blocker without running the repository Spotless/Checkstyle gates.
### Review Details
- **Reviewed Scope:** Latest PR head `855b6a34315af444996ec8abdc3bbd261ee7df35`; local merge-base `21c96bb9a02f8ad2a6e65475ff4886a6aa22e676`; local triple-dot file list matched GitHub `/pulls/38682/files` exactly. Reviewed workflows, Proxy Native Dockerfiles, GraalVM native-image docs, ClickHouse docs/version, reachability metadata diffs, `ProxyTestingServer`, `ZookeeperTest`, MySQL/PostgreSQL proxy native timeout changes, and native metadata filters.
- **Not Reviewed Scope:** Full native-image build/nativeTest execution, full line-by-line semantic validation of every generated metadata deletion, full ClickHouse dependency vulnerability/license scan, and GitHub Actions/CI status.
- **Verification:** `git fetch apache master:refs/remotes/apache/master` exit 0; forced PR ref refresh exit 0; GitHub/local file-list diff exit 0; JSON syntax validation for changed JSON files exit 0; `git diff -G 'computeIfAbsent|ConcurrentHashMap'` exit 0 with no matching diff; parser-file search exit 1 with no parser files; `git diff --check` exit 2 with trailing-whitespace diagnostics only; no Maven/native build was run locally.
- **Release Note / User Docs:** User docs were reviewed at diff level; official GraalVM docs support the filter rule ordering and GraalVM 25.0.2 macOS x64 removal (`https://www.graalvm.org/latest/reference-manual/native-image/metadata/AutomaticMetadataCollection/`, `https://www.graalvm.org/release-notes/JDK_25/`). Release note is required and missing.
--
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: notifications-unsubscribe@shardingsphere.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org