terrymanu commented on PR #40162:
URL: https://github.com/apache/shardingsphere/pull/40162#issuecomment-6121362346
### Result
**Review Result: Mergeable**
The local candidate fixes SPI instantiation failure when Hive JDBC is
unavailable. No blocking issues were found. This conclusion covers code
correctness only; CI was not reviewed.
### Evidence
- `HiveConnectionPropertiesParser.java:36–60`: The specific Hive exceptions
and original parsing body were moved into a private static nested class. Public
signatures, parsing results, property merge order, and exception types remain
unchanged. The outer class bytecode no longer directly references Hive JDBC
types.
- `HiveConnectionPropertiesParserTest.java:63–93`: The single new test
rejects all classes under `org.apache.hive.jdbc.`, obtains the Hive parser
through an isolated instance of the real `TypedSPILoader`, and verifies that
the parser belongs to the isolated class loader. The TCCL is restored in
`finally`, and the class loader is closed correctly.
- The new test failed against the original production code on both JDK 21
and JDK 8 because `ZooKeeperHiveClientException` was unavailable. It passed
after the fix. The final JDK 21 run passed all 10 tests; JDK 8 (1.8.0_322)
independently passed the missing-driver scenario.
- Spotless and Checkstyle passed for both target files. Aggregated JaCoCo
coverage was 2/2 classes, 12/12 lines, and 4/4 branches.
- The independent defensive-code audit passed. No production fallback,
generalized exception handling, or swallowed errors were introduced. Driver
rejection and resource restoration in the test directly satisfy the explicit
requirements.
### Coverage
- Standalone local candidate based on
`579a4d66ef733731ebe14bee7b4fc3de0c4e1905`. Only these local changes were
reviewed:
-
`database/connector/dialect/hive/src/main/java/org/apache/shardingsphere/database/connector/hive/jdbcurl/HiveConnectionPropertiesParser.java`
-
`database/connector/dialect/hive/src/test/java/org/apache/shardingsphere/database/connector/hive/jdbcurl/HiveConnectionPropertiesParserTest.java`
- The behavior scope covers SPI loading without the driver and compatibility
of existing Hive URL parsing. Existing optional-driver behavior is preserved;
no public contract or project commitment was added.
- Review covered root cause and behavior, impact and contracts, tests and
runtime paths, and final convergence. No unresolved facts, unauthorized
changes, or necessary scope expansion remain.
- Both quality scans exited with code 1 and reported only R15-C: the scanner
supports test-only scope and cannot represent the explicitly authorized
production relocation. An audit against the original two-file baseline
confirmed that the changes remain within scope. The scans were not reported as
fully passing.
- RAT was not applicable because no files, license headers, or RAT
configuration were added or changed. CI was not reviewed. Verification evidence
and this report apply only to the local candidate.
--
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]
