terrymanu commented on PR #39013:
URL: https://github.com/apache/shardingsphere/pull/39013#issuecomment-4886549622
### Summary
**Review Result: Mergeable**
Reason: This is a code-scope review only; CI not reviewed by request. The
latest PR head centralizes metadata-based database type fallback in
`DatabaseTypeFactory`, removes the Hive-specific branch from shared core code,
updates callers to use the metadata overload, and includes focused tests for
the new fallback and exception boundaries. I did not find code-level blockers
in the reviewed scope.
### Evidence
-
`database/connector/core/src/main/java/org/apache/shardingsphere/database/connector/core/type/DatabaseTypeFactory.java#L65-L79`
now keeps the complexity inside `get(DatabaseMetaData)`: it first resolves by
`DatabaseMetaData#getURL()`, then falls back through `DialectJdbcUrlFetcher`
only when the URL operation is unsupported.
-
`database/connector/dialect/hive/src/main/java/org/apache/shardingsphere/database/connector/hive/jdbcurl/HiveJdbcUrlFetcher.java#L31-L39`
and
`database/connector/dialect/hive/src/main/resources/META-INF/services/org.apache.shardingsphere.database.connector.core.jdbcurl.DialectJdbcUrlFetcher#L18`
provide the Hive-specific URL extraction through SPI, so shared core code no
longer needs a Hive type-name check.
-
`infra/common/src/main/java/org/apache/shardingsphere/infra/database/DatabaseTypeEngine.java#L99-L103`,
`kernel/transaction/core/src/main/java/org/apache/shardingsphere/transaction/savepoint/ConnectionSavepointManager.java#L100-L102`,
and
`mcp/support/src/main/java/org/apache/shardingsphere/mcp/support/database/metadata/jdbc/MCPJdbcDatabaseProfileLoader.java#L60-L75`
now delegate to `DatabaseTypeFactory.get(DatabaseMetaData)`, preserving the
intended boundary that callers do not need to know the dialect fallback details.
- The changed tests cover direct URL resolution, unsupported metadata URL
fallback through `DialectJdbcUrlFetcher`, no matching fetcher, unsupported
fetched URL, caller exception wrapping, and MCP invalid-configuration
conversion.
### Review Details
- Review Focus: Code Correctness Review. CI not reviewed by request.
- Reviewed Scope: PR #39013, latest head
`b000406db8d885176399e7958724f9e4d5f13485`, base `master` at
`b1508805707a53220e23290910d3699f747a8c2d`, merge-base
`b1508805707a53220e23290910d3699f747a8c2d`. Reviewed all 9 files from GitHub
`/pulls/39013/files`; the local triple-dot file list matched the GitHub file
list.
- Not Reviewed Scope: GitHub Actions/check-runs/logs, full E2E suites,
native-image/distribution packaging beyond the changed files.
- Verification:
- Public GitHub PR metadata, file list, issue comments, PR reviews, and
review comments were accessible through GitHub REST. Public comments/reviews
were empty at review time.
- `./mvnw -pl
database/connector/core,infra/common,kernel/transaction/core,mcp/support
-Dtest=DatabaseTypeFactoryTest,DatabaseTypeEngineTest,ConnectionSavepointManagerTest,MCPJdbcDatabaseProfileLoaderTest
test -Pcheck -T1C` passed with exit code 0.
- `./mvnw -pl mcp/core test -Pcheck -T1C` passed with exit code 0.
- `./mvnw checkstyle:check -Pcheck -T1C` passed with exit code 0.
- `./mvnw spotless:check -Pcheck -T1C` passed with exit code 0.
- Release Note / User Docs: Not required. The PR is an internal refactor
with no public configuration key, SQL syntax, user-facing protocol, migration,
or compatibility contract change found in the reviewed 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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
