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