jiangML commented on PR #39028:
URL: https://github.com/apache/shardingsphere/pull/39028#issuecomment-4901010501
• ### Summary
**Review Result: Mergeable**
**Reason:** This is a code-scope Code Correctness Review result, not a
final CI or repository merge-gate decision; CI not reviewed by request. The
latest PR head now covers the previously missing non-empty MySQL connection
attributes behavior, and I found no code-level blocker in the reviewed scope.
### Evidence
- `CLIENT_CONNECT_ATTRS` is advertised in
`database/protocol/dialect/mysql/src/main/java/org/apache/shardingsphere/database/protocol/mysql/constant/MySQLCapabilityFlag.java:99-100`.
- `MySQLHandshakeResponse41Packet` parses the length-encoded connection
attributes key/value block in
`database/protocol/dialect/mysql/src/main/java/org/apache/shardingsphere/database/protocol/mysql/packet/handshake/MySQLHandshakeResponse41Packet.java:97-112`.
- The new packet test builds a real `MySQLPacketPayload` with non-empty
attributes and asserts the parsed result in
`database/protocol/dialect/mysql/src/test/java/org/apache/shardingsphere/database/protocol/mysql/packet/handshake/MySQLHandshakeResponse41PacketTest.java:133-145`.
- MySQL authentication preserves parsed attributes in
`AuthenticationResult`; the test covers this at
`proxy/frontend/dialect/mysql/src/test/java/org/apache/shardingsphere/proxy/frontend/mysql/authentication/MySQLAuthenticationEngineTest.java:176-189`.
- `FrontendChannelInboundHandler` stores finished authentication
attributes on `ConnectionSession`; the test covers this at
`proxy/frontend/core/src/test/java/org/apache/shardingsphere/proxy/frontend/netty/FrontendChannelInboundHandlerTest.java:131-139`.
- Cross-protocol authentication implementations were updated for the new
`AuthenticationResultBuilder` signature and no stale internal call site was
found.
### Review Details
**Review Focus:** Code Correctness Review. CI not reviewed by request.
**Reviewed Scope:** Latest PR head
`b81e4c5c34b58adfdcbe1fdd0ac748b333ee21ae`; base `master` at
`c46e5a9de895875b346b6249bc2e0acba7d90121`; local merge-base
`c46e5a9de895875b346b6249bc2e0acba7d90121`. GitHub `/pulls/39028/files` listed
17 files, and the local triple-dot file list matched exactly. Reviewed MySQL
protocol, authentication result, proxy session, frontend authentication,
Firebird/openGauss/PostgreSQL adapter changes, and related tests.
**Not Reviewed Scope:** GitHub Actions/check-runs/Actions logs were not
reviewed. Proxy runtime smoke, native MySQL client smoke, and E2E scenarios
were not run.
**Verification:**
- `git fetch apache refs/pull/39028/head:pr-39028-latest`: exit 0.
- `./mvnw -pl
database/protocol/dialect/mysql,proxy/frontend/core,proxy/frontend/dialect/mysql
-DskipTests -DskipITs -Dspotless.skip=true test-compile`: exit 0, `BUILD
SUCCESS`.
- `./mvnw -pl
database/protocol/dialect/mysql,proxy/frontend/core,proxy/frontend/dialect/mysql
spotless:check -Pcheck -T1C`: exit 0, `BUILD SUCCESS`.
- `./mvnw -pl
database/protocol/dialect/mysql,proxy/frontend/core,proxy/frontend/dialect/mysql
checkstyle:check -Pcheck -T1C`: exit 0, `BUILD SUCCESS`.
- Focused test command exited 1 because the local environment failed to
initialize Mockito/Byte Buddy `MockMaker` self-attach before executing
`FrontendChannelInboundHandlerTest`; this did not indicate a PR assertion or
behavior failure.
**Release Note / User Docs:** Not required. This PR captures existing
MySQL protocol connection attributes into internal session state and does not
introduce user configuration, migration, rollback, or user-visible diagnostic
changes.
--
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]
