rajvarun77 commented on PR #3330: URL: https://github.com/apache/brpc/pull/3330#issuecomment-4664906986
@wwbmmm @chenBright — pushed a revision addressing the review feedback and fixing the integration-test setup. Summary: **Review comments** - Stored the mysql-transaction `BindSockAction` in two `Controller::_flags` bits instead of a dedicated member. - Made `MysqlResponse` explicitly non-copyable and turned the previously no-op `MergeFrom` into a hard `CHECK`-failure (so an accidental copy is caught instead of silently dropping replies). - Renamed `get_tx()`/`get_stmt()` to `tx()`/`stmt()`. - Added a clearly named `ControllerPrivateAccessor::set_mysql_statement_type()` alias over the `pipelined_count` slot the mysql protocol reuses. - The `MysqlCollations` ODR note, the gflag spelling, and the two doc/comment mismatches were already addressed in the current revision. - On `_fd_version`: I left a reply explaining why it needs to stay atomic — it is written in `ResetFileDescriptor` (reconnect path, no lock) and read lock-free from another thread via `fd_version()` in the prepared-statement path, so a non-atomic field would be a data race. It mirrors how `_fd` itself is handled. Happy to discuss further. **Integration tests now run against a real MySQL server** - The suites previously did not exercise a live server in CI: a legacy test pointed at an external host and hard-failed, and the make/bazel wiring built it as a single globbed target. I removed the legacy test, switched the `test/mysql/*` suites to the redis-style self-spawning + skip-if-absent pattern, build one cc_test per file, and merged master so the CI image installs `mysql-server`. The suites now spawn a local throwaway mysqld and run. **Local verification** I built and ran all seven mysql suites locally against a live MySQL 9.6 server: 110 tests, 0 skipped, 0 failed — covering `caching_sha2_password` auth, the text and binary (prepared-statement) protocols, transactions, and connection pooling. (MySQL 9.x removed `mysql_native_password`, so this also exercises the caching_sha2 path end-to-end.) CI is running on the latest revision. Could you take another look when you have a chance? Thanks for the reviews. -- 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]
