github-actions[bot] commented on PR #61050: URL: https://github.com/apache/doris/pull/61050#issuecomment-4001545434
## Code Review Summary: CLIENT_DEPRECATE_EOF Implementation **Overall Assessment: No blocking issues found.** The implementation is correct, well-structured, and covers all necessary code paths. ### Critical Checkpoint Conclusions **1. Goal and Correctness:** The PR correctly implements the MySQL `CLIENT_DEPRECATE_EOF` capability to fix empty result sets with MySQL Connector/J 9.5.0+. The fix addresses all four protocol-level changes: (a) advertising the capability in the handshake, (b) suppressing intermediate EOF packets after column definitions, (c) replacing the end-of-result-set EOF with a ResultSet OK packet (0xFE with payload > 5 bytes), and (d) always writing the info field in OK packets to prevent `ArrayIndexOutOfBoundsException`. **2. Modification Scope (Small, Clear, Focused):** Yes. Changes are limited to MySQL protocol handling across 10 files (6 source, 3 test, 1 thrift). Each change serves a clear purpose in the protocol implementation. **3. Concurrency:** Not applicable. The `clientDeprecatedEOF` flag is set once during connection handshake (single-threaded per connection) and then read-only during the connection lifetime. No concurrency concerns. **4. Lifecycle Management:** Not applicable. No complex lifecycle management introduced. **5. Configuration Items:** Not applicable. No new configuration items added — this is a protocol-level capability negotiation. **6. Incompatible Changes / Upgrade Compatibility:** **Safe.** The `CLIENT_DEPRECATE_EOF` capability is only activated when the client also advertises it during the handshake negotiation. Older clients that do not send this flag will continue to receive the traditional EOF packets. The `MysqlOkPacket` change to always write the info field adds a trailing `0x00` byte (empty string) which is safe per the MySQL protocol spec (`string<EOF>` format — the rest of the packet). The Thrift change uses an optional field (ID 1003), which is backward-compatible during rolling upgrades. **7. Parallel Code Paths:** All 4 intermediate EOF sending points in `StmtExecutor` (`sendMetaData`, `sendStmtPrepareOK` x2, `sendFields`) are correctly guarded. The end-of-result-set path in `ConnectProcessor.getResultPacket()` is the sole path for sending the final EOF/ResultSet OK packet and is correctly guarded. `MysqlProto.sendResponsePacket()` is only used during the authentication phase (state is always OK or ERR, never EOF) and does not need modification. No `ConnectProcessor` subclasses override `getResultPacket()`. **8. Test Coverage:** Unit tests cover: (a) `MysqlCapabilityTest` verifies the flag is in default capabilities, (b) `MysqlOkPacketTest` verifies the empty info field is written and payload > 5, (c) `MysqlResultSetEndPacketTest` verifies header, all fields, payload > 5, and distinguishability from traditional EOF. The PR references manual testing with MySQL Connector/J 9.5.0+ via a dedicated test tool. A regression test would strengthen confidence but is not strictly required given the protocol-level nature of the fix. **9. Observability:** Not applicable. Protocol-level change; existing connection logging is sufficient. **10. FE-BE Protocol Compatibility:** The Thrift change adds an optional bool field to `TMasterOpRequest` for proxy forwarding. This is correctly propagated in `FEOpExecutor.buildStmtForwardParams()` and consumed in `ConnectProcessor.proxyExecute()`. The field is optional, so old FE nodes will simply not set it (defaulting to false, which means traditional EOF behavior — correct). **11. Performance:** No performance concern. The only change is conditional checks on a boolean field (negligible cost) and slightly different packet encoding in the end-of-result-set marker. **12. Other Observations (Informational, Non-blocking):** - Both `MysqlEofPacket` and the new `MysqlResultSetEndPacket` hardcode warnings to 0 rather than propagating `QueryState.getWarningRows()`. This is pre-existing behavior and not a regression introduced by this PR. - The implementation correctly follows the MySQL protocol specification: intermediate EOF suppression between column defs and data rows, and 0xFE OK packet (payload > 5 bytes) as the end-of-result-set marker. -- 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] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
