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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
