Fix infra-mysql57-binlog and infra-mysql84-binlog#5591
Open
wazir-ahmed wants to merge 11 commits intosysown:v3.0from
Open
Fix infra-mysql57-binlog and infra-mysql84-binlog#5591wazir-ahmed wants to merge 11 commits intosysown:v3.0from
infra-mysql57-binlog and infra-mysql84-binlog#5591wazir-ahmed wants to merge 11 commits intosysown:v3.0from
Conversation
- Replace the external binlog reader with a helper function which
uses mariadb replication functions.
- The helper opens replication sessions, sends `COM_REGISTER_SLAVE`,
and fetches heartbeats to verify the stream is active.
- It runs the same two-session flow required for TAP tests:
- `test_com_register_slave_enables_fast_forward-t`
- `test_binlog_reader_uses_previous_hostgroup-t`
Signed-off-by: Wazir Ahmed <wazir@proxysql.com>
wazir-ahmed
changed the title
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThe PR consolidates binlog reader functionality by removing the external Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request replaces the external test_binlog_reader-t dependency with a native C++ implementation within the test suite. By introducing binlog_rpl.h, which utilizes the MariaDB client replication API, the infrastructure no longer needs to manage external binaries or symlinks in the test environment. Review feedback identifies a memory leak where strdup is used without a corresponding free and suggests using mysql_real_connect flags rather than directly modifying internal library structures to improve compatibility.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (3)
test/tap/tests/binlog_rpl.h (2)
66-67: Direct access to internal MariaDB client structure.Accessing
mysql->options.client_flagdirectly relies on MariaDB client library internals. This works but is fragile if the internal structure changes. Consider usingmysql_options()withMYSQL_OPT_CONNECT_FLAGSif available, though this may not support adding flags post-init.Given this is test code and the MariaDB client is pinned in the build system, this is acceptable.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/tap/tests/binlog_rpl.h` around lines 66 - 67, The test currently mutates the internal client struct via mysql->options.client_flag |= CLIENT_DEPRECATE_EOF which is fragile; replace this direct access with a call to mysql_options(mysql, MYSQL_OPT_CONNECT_FLAGS, &flags) if available (build-time guard) to set CLIENT_DEPRECATE_EOF, otherwise keep the existing line but wrap it with a clear comment explaining this is intentional for pinned test builds and add an `#ifdef` guard around the direct access to limit it to known client ABI versions; reference mysql->options.client_flag, CLIENT_DEPRECATE_EOF, mysql_options() and MYSQL_OPT_CONNECT_FLAGS to locate where to change.
103-106: Memory allocated forrpl->hostis not explicitly freed.
strdup("127.0.0.1")allocates memory that is not explicitly deallocated. Whilemariadb_rpl_close()is called at line 150, whether it freesrpl->hostinternally cannot be verified without access to the MariaDB library source. This represents a minor leak (8 bytes × 2 sessions). However, per project conventions, resource leaks in short-lived test processes are accepted practice across the test suite.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/tap/tests/binlog_rpl.h` around lines 103 - 106, The strdup call assigning rpl->host ("127.0.0.1") allocates memory that isn't freed; ensure you free that allocation if mariadb_rpl_close() does not own it by calling free(rpl->host) (or set rpl->host = NULL after freeing) at the end of the test, or confirm and document that mariadb_rpl_close() takes ownership and frees rpl->host (referencing rpl->host, strdup and mariadb_rpl_close).test/tap/tests/test_binlog_reader_uses_previous_hostgroup-t.cpp (1)
131-133: Stale comment references removed tool.The comment mentions "test_binlog_reader-t tool" but this PR removes that tool in favor of
run_binlog_rpl(). Consider updating the comment.📝 Suggested update
- // test_binlog_reader-t tool make two fast_forward connections, so we - // should expect two more closed connections after its execution. + // run_binlog_rpl() makes two fast_forward connections, so we + // should expect two more closed connections after its execution.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/tap/tests/test_binlog_reader_uses_previous_hostgroup-t.cpp` around lines 131 - 133, Update the stale comment that references "test_binlog_reader-t tool" to reflect the new implementation: mention run_binlog_rpl() (or the function that performs the fast_forward connections) instead of the removed tool, so the comment above expected_increment (const int expected_increment = 2) accurately states that run_binlog_rpl() makes two fast_forward connections and therefore two additional closed connections are expected.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@test/tap/tests/binlog_rpl.h`:
- Around line 66-67: The test currently mutates the internal client struct via
mysql->options.client_flag |= CLIENT_DEPRECATE_EOF which is fragile; replace
this direct access with a call to mysql_options(mysql, MYSQL_OPT_CONNECT_FLAGS,
&flags) if available (build-time guard) to set CLIENT_DEPRECATE_EOF, otherwise
keep the existing line but wrap it with a clear comment explaining this is
intentional for pinned test builds and add an `#ifdef` guard around the direct
access to limit it to known client ABI versions; reference
mysql->options.client_flag, CLIENT_DEPRECATE_EOF, mysql_options() and
MYSQL_OPT_CONNECT_FLAGS to locate where to change.
- Around line 103-106: The strdup call assigning rpl->host ("127.0.0.1")
allocates memory that isn't freed; ensure you free that allocation if
mariadb_rpl_close() does not own it by calling free(rpl->host) (or set rpl->host
= NULL after freeing) at the end of the test, or confirm and document that
mariadb_rpl_close() takes ownership and frees rpl->host (referencing rpl->host,
strdup and mariadb_rpl_close).
In `@test/tap/tests/test_binlog_reader_uses_previous_hostgroup-t.cpp`:
- Around line 131-133: Update the stale comment that references
"test_binlog_reader-t tool" to reflect the new implementation: mention
run_binlog_rpl() (or the function that performs the fast_forward connections)
instead of the removed tool, so the comment above expected_increment (const int
expected_increment = 2) accurately states that run_binlog_rpl() makes two
fast_forward connections and therefore two additional closed connections are
expected.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: b9a4fee4-102b-4476-95cd-89cc1947f5b4
📒 Files selected for processing (5)
test/infra/control/run-tests-isolated.bashtest/tap/tests/Makefiletest/tap/tests/binlog_rpl.htest/tap/tests/test_binlog_reader_uses_previous_hostgroup-t.cpptest/tap/tests/test_com_register_slave_enables_fast_forward-t.cpp
💤 Files with no reviewable changes (1)
- test/infra/control/run-tests-isolated.bash
📜 Review details
🧰 Additional context used
📓 Path-based instructions (3)
**/*.{cpp,h,hpp}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{cpp,h,hpp}: Class names must use PascalCase with protocol prefixes (MySQL_, PgSQL_, ProxySQL_)
Member variables must use snake_case
Constants and macros must use UPPER_SNAKE_CASE
C++17 is required; use conditional compilation via#ifdefPROXYSQLGENAI,#ifdefPROXYSQL31, etc. for feature flags
Use pthread mutexes for synchronization and std::atomic<> for counters
Files:
test/tap/tests/test_binlog_reader_uses_previous_hostgroup-t.cpptest/tap/tests/test_com_register_slave_enables_fast_forward-t.cpptest/tap/tests/binlog_rpl.h
**/*.cpp
📄 CodeRabbit inference engine (CLAUDE.md)
Use RAII for resource management and jemalloc for memory allocation
Files:
test/tap/tests/test_binlog_reader_uses_previous_hostgroup-t.cpptest/tap/tests/test_com_register_slave_enables_fast_forward-t.cpp
test/tap/tests/{test_*,*-t}.cpp
📄 CodeRabbit inference engine (CLAUDE.md)
Test files in test/tap/tests/ must follow the naming pattern test_*.cpp or *-t.cpp
Files:
test/tap/tests/test_binlog_reader_uses_previous_hostgroup-t.cpptest/tap/tests/test_com_register_slave_enables_fast_forward-t.cpp
🧠 Learnings (8)
📓 Common learnings
Learnt from: wazir-ahmed
Repo: sysown/proxysql PR: 5557
File: test/tap/tests/unit/gtid_set_unit-t.cpp:14-17
Timestamp: 2026-04-01T21:27:00.297Z
Learning: In ProxySQL's unit test directory (test/tap/tests/unit/), test_globals.h and test_init.h are only required for tests that depend on the ProxySQL runtime globals/initialization (i.e., tests that exercise components linked against libproxysql.a). Pure data-structure or utility tests (e.g., ezoption_parser_unit-t.cpp, gtid_set_unit-t.cpp, gtid_trxid_interval_unit-t.cpp) only need tap.h and the relevant project header — omitting test_globals.h and test_init.h is correct and intentional in these cases.
Learnt from: yuji-hatakeyama
Repo: sysown/proxysql PR: 5307
File: test/tap/tests/reg_test_5306-show_warnings_with_comment-t.cpp:39-48
Timestamp: 2026-01-20T09:34:27.165Z
Learning: In ProxySQL test files (test/tap/tests/), resource leaks (such as not calling `mysql_close()` on early return paths) are not typically fixed because test processes are short-lived and the OS frees resources on process exit. This is a common pattern across the test suite.
Learnt from: CR
Repo: sysown/proxysql PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-22T14:38:16.093Z
Learning: Applies to test/tap/tests/unit/**/*.cpp : Unit tests in test/tap/tests/unit/ must use test_globals.h and test_init.h and link against libproxysql.a via the custom test harness
📚 Learning: 2026-03-22T14:38:16.093Z
Learnt from: CR
Repo: sysown/proxysql PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-22T14:38:16.093Z
Learning: Applies to test/tap/tests/unit/**/*.cpp : Unit tests in test/tap/tests/unit/ must use test_globals.h and test_init.h and link against libproxysql.a via the custom test harness
Applied to files:
test/tap/tests/Makefiletest/tap/tests/test_binlog_reader_uses_previous_hostgroup-t.cpptest/tap/tests/test_com_register_slave_enables_fast_forward-t.cpptest/tap/tests/binlog_rpl.h
📚 Learning: 2026-03-22T14:38:16.093Z
Learnt from: CR
Repo: sysown/proxysql PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-22T14:38:16.093Z
Learning: Applies to test/tap/tests/{test_*,*-t}.cpp : Test files in test/tap/tests/ must follow the naming pattern test_*.cpp or *-t.cpp
Applied to files:
test/tap/tests/Makefile
📚 Learning: 2026-04-01T21:27:00.297Z
Learnt from: wazir-ahmed
Repo: sysown/proxysql PR: 5557
File: test/tap/tests/unit/gtid_set_unit-t.cpp:14-17
Timestamp: 2026-04-01T21:27:00.297Z
Learning: In ProxySQL's unit test directory (test/tap/tests/unit/), test_globals.h and test_init.h are only required for tests that depend on the ProxySQL runtime globals/initialization (i.e., tests that exercise components linked against libproxysql.a). Pure data-structure or utility tests (e.g., ezoption_parser_unit-t.cpp, gtid_set_unit-t.cpp, gtid_trxid_interval_unit-t.cpp) only need tap.h and the relevant project header — omitting test_globals.h and test_init.h is correct and intentional in these cases.
Applied to files:
test/tap/tests/Makefiletest/tap/tests/test_binlog_reader_uses_previous_hostgroup-t.cpptest/tap/tests/test_com_register_slave_enables_fast_forward-t.cpptest/tap/tests/binlog_rpl.h
📚 Learning: 2026-01-20T09:34:19.124Z
Learnt from: yuji-hatakeyama
Repo: sysown/proxysql PR: 5307
File: test/tap/tests/reg_test_5306-show_warnings_with_comment-t.cpp:39-48
Timestamp: 2026-01-20T09:34:19.124Z
Learning: In ProxySQL's TAP test suite, resource leaks (e.g., not calling mysql_close() on early return paths) are commonly tolerated because test processes are short-lived and OS frees resources on exit. This pattern applies to all C++ test files under test/tap/tests. When reviewing, recognize this as a project-wide test convention and focus on test correctness and isolation rather than insisting on fixing such leaks in these test files.
Applied to files:
test/tap/tests/test_binlog_reader_uses_previous_hostgroup-t.cpptest/tap/tests/test_com_register_slave_enables_fast_forward-t.cpp
📚 Learning: 2026-01-20T07:40:34.938Z
Learnt from: yuji-hatakeyama
Repo: sysown/proxysql PR: 5307
File: test/tap/tests/reg_test_5306-show_warnings_with_comment-t.cpp:24-28
Timestamp: 2026-01-20T07:40:34.938Z
Learning: In ProxySQL test files, calling `mysql_error(NULL)` after `mysql_init()` failure is safe because the MariaDB client library implementation returns an empty string for NULL handles (not undefined behavior).
Applied to files:
test/tap/tests/test_binlog_reader_uses_previous_hostgroup-t.cpptest/tap/tests/test_com_register_slave_enables_fast_forward-t.cpptest/tap/tests/binlog_rpl.h
📚 Learning: 2026-01-20T09:34:27.165Z
Learnt from: yuji-hatakeyama
Repo: sysown/proxysql PR: 5307
File: test/tap/tests/reg_test_5306-show_warnings_with_comment-t.cpp:39-48
Timestamp: 2026-01-20T09:34:27.165Z
Learning: In ProxySQL test files (test/tap/tests/), resource leaks (such as not calling `mysql_close()` on early return paths) are not typically fixed because test processes are short-lived and the OS frees resources on process exit. This is a common pattern across the test suite.
Applied to files:
test/tap/tests/binlog_rpl.h
📚 Learning: 2026-03-22T14:38:16.093Z
Learnt from: CR
Repo: sysown/proxysql PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-22T14:38:16.093Z
Learning: Applies to **/*.{cpp,h,hpp} : C++17 is required; use conditional compilation via `#ifdef` PROXYSQLGENAI, `#ifdef` PROXYSQL31, etc. for feature flags
Applied to files:
test/tap/tests/binlog_rpl.h
🪛 Clang (14.0.6)
test/tap/tests/test_com_register_slave_enables_fast_forward-t.cpp
[error] 9-9: 'cstdlib' file not found
(clang-diagnostic-error)
test/tap/tests/binlog_rpl.h
[error] 22-22: 'cstdlib' file not found
(clang-diagnostic-error)
🔇 Additional comments (8)
test/tap/tests/Makefile (1)
188-190: LGTM!The prerequisite-only rules correctly ensure both test executables rebuild when
binlog_rpl.hchanges, while the pattern rule at line 191 handles actual compilation. This is the idiomatic way to add header dependencies in Make.test/tap/tests/test_binlog_reader_uses_previous_hostgroup-t.cpp (2)
21-21: LGTM!Clean integration of the new header to replace the external binary dependency.
109-114: LGTM!Good refactoring from subprocess execution to direct function call. The error handling with diagnostic output is appropriate.
test/tap/tests/test_com_register_slave_enables_fast_forward-t.cpp (2)
2-6: LGTM!Clear documentation explaining the test purpose and the MARIADB_RPL mechanism being tested.
14-37: LGTM!Clean refactoring that:
- Properly initializes CommandLine and validates environment
- Replaces subprocess with direct
run_binlog_rpl()call- Maintains appropriate diagnostic output on failure
- Correctly asserts success with
res == 0The
#include <cstdlib>at line 9 is valid C++ and the static analysis warning is a false positive (likely due to missing include paths in the analyzer's environment).test/tap/tests/binlog_rpl.h (3)
1-31: LGTM!Well-documented header with clear explanation of the two-session flow and COM_REGISTER_SLAVE mechanism. Include guards and includes are appropriate.
48-156: Well-structured replication session helper.The function follows good practices:
- Clear diagnostic output with session identifiers
- Proper error handling with goto-based cleanup
- Correct use of MariaDB replication API
- Validates stream activity by counting heartbeats
The cleanup section properly closes resources in reverse order of acquisition.
170-190: LGTM!Clean orchestration of two replication sessions with distinct server IDs. Sequential execution with early failure return is appropriate for this test scenario.
…ecars Issue --- - Currently binlog-reader is deployed as a sidecar in `infra-mysql57-binlog` alongside with MySQL container, but the sidecar is not configured to share the same IP. - This results in unreliable DNS resolution and causes ProxySQL to connect with the binlog-reader for new backend connections. - In `infra-mysql57-binlog`, MySQL containers already have an binlog-reader installed, but they are misconfigured, making the sidecar approach redundant. Fix --- - Remove binlog-reader sidecar, use the binlog-reader installed in MySQL container. - Pass correct environment variables to the binlog-reader process. - Introduce a configurable delay in the CI to ensure that binlog-readers are up and running before running the test. Signed-off-by: Wazir Ahmed <wazir@proxysql.com>
Signed-off-by: Wazir Ahmed <wazir@proxysql.com>
wazir-ahmed
changed the title
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
test/infra/control/ensure-infras.bash (1)
67-77: Extract the binlog-delay calculation into one shared helper.This exact
BINLOG_READER_START_DELAYdefaulting and*-binlogdetection logic now exists here and intest/infra/control/run-tests-isolated.bashon Line 78-Line 88. Keeping both copies in sync will be easy to miss the next time the startup policy changes.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/infra/control/ensure-infras.bash` around lines 67 - 77, Extract the BINLOG_READER_START_DELAY defaulting logic into a single reusable helper function (e.g., compute_binlog_reader_start_delay) that inspects the INFRAS array for any name matching "*-binlog", sets BINLOG_READER_START_DELAY to its default (30) if unset, and forces it to 0 when no "*-binlog" is found; place this helper in a shared script that both callers can source, replace the duplicated block in both ensure-infras.bash and run-tests-isolated.bash with a call to compute_binlog_reader_start_delay, and ensure the helper uses the same exported variable name (BINLOG_READER_START_DELAY) and preserves existing behavior around BINLOG_INFRA_FOUND detection.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@test/infra/infra-mysql57-binlog/docker-compose.yml`:
- Around line 19-25: The current startup loop waits BINLOG_READER_START_DELAY
but then sleeps an extra 5 seconds before the first proxysql_binlog_reader
attempt, so readers may not be listening when tests start; update the loop(s)
that run "proxysql_binlog_reader -h ... -l \"$${LISTEN_PORT:-6020}\"" to attempt
the reader immediately and only sleep 5 seconds after a failed/terminated reader
(i.e., move the `sleep 5` to after the proxysql_binlog_reader invocation or
restructure the while-true loop to run the reader first then sleep), and apply
this change to all three occurrences using the same pattern (the blocks that
start with `bash -c "(docker-entrypoint.sh mysqld &); sleep
${BINLOG_READER_START_DELAY}; while true; do ... proxysql_binlog_reader ...;
done"`).
- Around line 19-25: The startup currently backgrounds docker-entrypoint.sh
mysqld so PID 1 is the reader loop; change the command so mysqld runs in the
foreground (PID 1) and the proxysql_binlog_reader loop runs as a background job
instead. Concretely, for the command that invokes docker-entrypoint.sh mysqld
and proxysql_binlog_reader, start or background proxysql_binlog_reader (or spawn
it in the background via &) before exec'ing or running docker-entrypoint.sh
mysqld in the foreground (use exec/docker-entrypoint.sh mysqld as the final
foreground process) so container health reflects MySQL health and
docker-compose-init.bash container checks behave correctly.
In `@test/infra/SKILL.md`:
- Around line 149-152: The troubleshooting step uses the environment variable
COMPOSE_PROJECT but does not document it; update the docs to either instruct
users to source the .env created by docker-compose-init.bash (e.g., "source .env
to set COMPOSE_PROJECT") or replace the hardcoded container reference with a
self-contained lookup that resolves the container name dynamically (for example
use docker ps/grep with INFRA_ID to find the mysql container ID/name and run the
timeout test against that container); ensure references to COMPOSE_PROJECT,
docker-compose-init.bash, and INFRA_ID are mentioned so readers know which
variables/scripts provide the value.
---
Nitpick comments:
In `@test/infra/control/ensure-infras.bash`:
- Around line 67-77: Extract the BINLOG_READER_START_DELAY defaulting logic into
a single reusable helper function (e.g., compute_binlog_reader_start_delay) that
inspects the INFRAS array for any name matching "*-binlog", sets
BINLOG_READER_START_DELAY to its default (30) if unset, and forces it to 0 when
no "*-binlog" is found; place this helper in a shared script that both callers
can source, replace the duplicated block in both ensure-infras.bash and
run-tests-isolated.bash with a call to compute_binlog_reader_start_delay, and
ensure the helper uses the same exported variable name
(BINLOG_READER_START_DELAY) and preserves existing behavior around
BINLOG_INFRA_FOUND detection.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 9fb6468b-83cd-4682-9b8c-fb19475b9d91
📒 Files selected for processing (10)
test/infra/SKILL.mdtest/infra/control/docker-fs-helper.bashtest/infra/control/ensure-infras.bashtest/infra/control/run-tests-isolated.bashtest/infra/control/start-proxysql-isolated.bashtest/infra/control/stop-proxysql-isolated.bashtest/infra/infra-mysql57-binlog/.envtest/infra/infra-mysql57-binlog/conf/proxysql/infra-config.sqltest/infra/infra-mysql57-binlog/docker-compose-init.bashtest/infra/infra-mysql57-binlog/docker-compose.yml
✅ Files skipped from review due to trivial changes (1)
- test/infra/infra-mysql57-binlog/conf/proxysql/infra-config.sql
📜 Review details
🧰 Additional context used
🧠 Learnings (4)
📓 Common learnings
Learnt from: yuji-hatakeyama
Repo: sysown/proxysql PR: 5307
File: test/tap/tests/reg_test_5306-show_warnings_with_comment-t.cpp:39-48
Timestamp: 2026-01-20T09:34:27.165Z
Learning: In ProxySQL test files (test/tap/tests/), resource leaks (such as not calling `mysql_close()` on early return paths) are not typically fixed because test processes are short-lived and the OS frees resources on process exit. This is a common pattern across the test suite.
Learnt from: wazir-ahmed
Repo: sysown/proxysql PR: 5557
File: test/tap/tests/unit/gtid_set_unit-t.cpp:14-17
Timestamp: 2026-04-01T21:27:03.216Z
Learning: In ProxySQL's unit test directory (test/tap/tests/unit/), test_globals.h and test_init.h are only required for tests that depend on the ProxySQL runtime globals/initialization (i.e., tests that exercise components linked against libproxysql.a). Pure data-structure or utility tests (e.g., ezoption_parser_unit-t.cpp, gtid_set_unit-t.cpp, gtid_trxid_interval_unit-t.cpp) only need tap.h and the relevant project header — omitting test_globals.h and test_init.h is correct and intentional in these cases.
📚 Learning: 2026-01-20T09:34:27.165Z
Learnt from: yuji-hatakeyama
Repo: sysown/proxysql PR: 5307
File: test/tap/tests/reg_test_5306-show_warnings_with_comment-t.cpp:39-48
Timestamp: 2026-01-20T09:34:27.165Z
Learning: In ProxySQL test files (test/tap/tests/), resource leaks (such as not calling `mysql_close()` on early return paths) are not typically fixed because test processes are short-lived and the OS frees resources on process exit. This is a common pattern across the test suite.
Applied to files:
test/infra/SKILL.mdtest/infra/control/start-proxysql-isolated.bashtest/infra/control/run-tests-isolated.bash
📚 Learning: 2026-01-20T07:40:34.938Z
Learnt from: yuji-hatakeyama
Repo: sysown/proxysql PR: 5307
File: test/tap/tests/reg_test_5306-show_warnings_with_comment-t.cpp:24-28
Timestamp: 2026-01-20T07:40:34.938Z
Learning: In ProxySQL test files, calling `mysql_error(NULL)` after `mysql_init()` failure is safe because the MariaDB client library implementation returns an empty string for NULL handles (not undefined behavior).
Applied to files:
test/infra/SKILL.mdtest/infra/control/run-tests-isolated.bash
📚 Learning: 2026-04-01T21:27:03.216Z
Learnt from: wazir-ahmed
Repo: sysown/proxysql PR: 5557
File: test/tap/tests/unit/gtid_set_unit-t.cpp:14-17
Timestamp: 2026-04-01T21:27:03.216Z
Learning: In ProxySQL's unit test directory (test/tap/tests/unit/), test_globals.h and test_init.h are only required for tests that depend on the ProxySQL runtime globals/initialization (i.e., tests that exercise components linked against libproxysql.a). Pure data-structure or utility tests (e.g., ezoption_parser_unit-t.cpp, gtid_set_unit-t.cpp, gtid_trxid_interval_unit-t.cpp) only need tap.h and the relevant project header — omitting test_globals.h and test_init.h is correct and intentional in these cases.
Applied to files:
test/infra/control/start-proxysql-isolated.bashtest/infra/control/run-tests-isolated.bash
🪛 dotenv-linter (4.0.0)
test/infra/infra-mysql57-binlog/.env
[warning] 27-27: [SubstitutionKey] The BINLOG_READER_START_DELAY key is not assigned properly
(SubstitutionKey)
[warning] 27-27: [UnorderedKey] The BINLOG_READER_START_DELAY key should go before the ROOT_PASSWORD key
(UnorderedKey)
🪛 GitHub Check: SonarCloud Code Analysis
test/infra/control/ensure-infras.bash
[failure] 75-75: Use '[[' instead of '[' for conditional tests. The '[[' construct is safer and more feature-rich.
test/infra/control/docker-fs-helper.bash
[failure] 12-12: Use '[[' instead of '[' for conditional tests. The '[[' construct is safer and more feature-rich.
[failure] 12-12: Use '[[' instead of '[' for conditional tests. The '[[' construct is safer and more feature-rich.
test/infra/control/run-tests-isolated.bash
[failure] 86-86: Use '[[' instead of '[' for conditional tests. The '[[' construct is safer and more feature-rich.
🪛 Shellcheck (0.11.0)
test/infra/control/stop-proxysql-isolated.bash
[info] 10-10: Not following: ./docker-fs-helper.bash was not specified as input (see shellcheck -x).
(SC1091)
test/infra/control/start-proxysql-isolated.bash
[info] 10-10: Not following: ./docker-fs-helper.bash was not specified as input (see shellcheck -x).
(SC1091)
test/infra/infra-mysql57-binlog/docker-compose-init.bash
[info] 16-16: Not following: ./test/infra/control/docker-fs-helper.bash was not specified as input (see shellcheck -x).
(SC1091)
test/infra/control/run-tests-isolated.bash
[warning] 405-405: This { is literal. Check expression (missing ;/\n?) or quote it.
(SC1083)
[warning] 405-405: This } is literal. Check expression (missing ;/\n?) or quote it.
(SC1083)
Signed-off-by: Wazir Ahmed <wazir@proxysql.com>
There was a problem hiding this comment.
♻️ Duplicate comments (2)
test/infra/infra-mysql57-binlog/docker-compose.yml (2)
20-25:⚠️ Potential issue | 🟠 MajorStart
proxysql_binlog_readerimmediately after the configured delay.The current loop sleeps an extra 5 seconds before the first reader attempt, so readiness drifts beyond
BINLOG_READER_START_DELAYand can race test startup.Suggested adjustment
- while true; do - sleep 5; - proxysql_binlog_reader -h \"$${MYSQL_HOST:-127.0.0.1}\" -u \"$${BINLOG_READER_USER:=root}\" -p \"$${BINLOG_READER_PASSWORD:-root}\" -P \"$${MYSQL_PORT:-3306}\" -l \"$${LISTEN_PORT:-6020}\" -f 2>&1 | tee -a /var/log/mysql/binlogreader-error.log; - done" + while true; do + proxysql_binlog_reader -h \"$${MYSQL_HOST:-127.0.0.1}\" -u \"$${BINLOG_READER_USER:=root}\" -p \"$${BINLOG_READER_PASSWORD:-root}\" -P \"$${MYSQL_PORT:-3306}\" -l \"$${LISTEN_PORT:-6020}\" -f 2>&1 | tee -a /var/log/mysql/binlogreader-error.log; + sleep 5; + done"Also applies to: 55-60, 92-97
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/infra/infra-mysql57-binlog/docker-compose.yml` around lines 20 - 25, The startup wrapper currently does "sleep ${BINLOG_READER_START_DELAY}; while true; do sleep 5; proxysql_binlog_reader ...; done" which waits an extra 5s before the first proxysql_binlog_reader invocation; change the flow so you perform the configured BINLOG_READER_START_DELAY, then start proxysql_binlog_reader immediately and only sleep between restart attempts (e.g., sleep 5 after the reader exits), updating the bash block that invokes proxysql_binlog_reader (the proxysql_binlog_reader invocation and the BINLOG_READER_START_DELAY usage) accordingly; apply the same change to the other identical loops at the other occurrences mentioned (the blocks at 55-60 and 92-97).
20-25:⚠️ Potential issue | 🟠 MajorRun
mysqldin the foreground so container health tracks database health.With
docker-entrypoint.sh mysqld &, PID 1 is the shell loop. If MySQL exits, the container can stayRunning, masking backend failures during infra checks.Suggested process model
- command: > - bash -c "(docker-entrypoint.sh mysqld &); - sleep ${BINLOG_READER_START_DELAY}; - while true; do - sleep 5; - proxysql_binlog_reader -h \"$${MYSQL_HOST:-127.0.0.1}\" -u \"$${BINLOG_READER_USER:=root}\" -p \"$${BINLOG_READER_PASSWORD:-root}\" -P \"$${MYSQL_PORT:-3306}\" -l \"$${LISTEN_PORT:-6020}\" -f 2>&1 | tee -a /var/log/mysql/binlogreader-error.log; - done" + command: > + bash -c "( + sleep ${BINLOG_READER_START_DELAY}; + while true; do + proxysql_binlog_reader -h \"$${MYSQL_HOST:-127.0.0.1}\" -u \"$${BINLOG_READER_USER:=root}\" -p \"$${BINLOG_READER_PASSWORD:-root}\" -P \"$${MYSQL_PORT:-3306}\" -l \"$${LISTEN_PORT:-6020}\" -f 2>&1 | tee -a /var/log/mysql/binlogreader-error.log; + sleep 5; + done + ) & exec docker-entrypoint.sh mysqld"Also applies to: 55-60, 92-97
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/infra/infra-mysql57-binlog/docker-compose.yml` around lines 20 - 25, The container currently backgrounds MySQL (docker-entrypoint.sh mysqld &) so the shell loop is PID 1; change the startup so mysqld runs in the foreground as PID 1 and the proxysql_binlog_reader runs in the background. Concretely: start the proxysql_binlog_reader via a backgrounded loop (using sleep ${BINLOG_READER_START_DELAY} then the while loop ending with &), then exec docker-entrypoint.sh mysqld (no &) so mysqld becomes PID 1 and container health reflects MySQL exit; apply the same change for the other identical blocks that run docker-entrypoint.sh mysqld &.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@test/infra/infra-mysql57-binlog/docker-compose.yml`:
- Around line 20-25: The startup wrapper currently does "sleep
${BINLOG_READER_START_DELAY}; while true; do sleep 5; proxysql_binlog_reader
...; done" which waits an extra 5s before the first proxysql_binlog_reader
invocation; change the flow so you perform the configured
BINLOG_READER_START_DELAY, then start proxysql_binlog_reader immediately and
only sleep between restart attempts (e.g., sleep 5 after the reader exits),
updating the bash block that invokes proxysql_binlog_reader (the
proxysql_binlog_reader invocation and the BINLOG_READER_START_DELAY usage)
accordingly; apply the same change to the other identical loops at the other
occurrences mentioned (the blocks at 55-60 and 92-97).
- Around line 20-25: The container currently backgrounds MySQL
(docker-entrypoint.sh mysqld &) so the shell loop is PID 1; change the startup
so mysqld runs in the foreground as PID 1 and the proxysql_binlog_reader runs in
the background. Concretely: start the proxysql_binlog_reader via a backgrounded
loop (using sleep ${BINLOG_READER_START_DELAY} then the while loop ending with
&), then exec docker-entrypoint.sh mysqld (no &) so mysqld becomes PID 1 and
container health reflects MySQL exit; apply the same change for the other
identical blocks that run docker-entrypoint.sh mysqld &.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: f0084917-79c1-46b7-8e11-edf3630a0226
📒 Files selected for processing (2)
test/infra/infra-mysql57-binlog/bin/docker-mysql-post.bashtest/infra/infra-mysql57-binlog/docker-compose.yml
📜 Review details
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: yuji-hatakeyama
Repo: sysown/proxysql PR: 5307
File: test/tap/tests/reg_test_5306-show_warnings_with_comment-t.cpp:39-48
Timestamp: 2026-01-20T09:34:27.165Z
Learning: In ProxySQL test files (test/tap/tests/), resource leaks (such as not calling `mysql_close()` on early return paths) are not typically fixed because test processes are short-lived and the OS frees resources on process exit. This is a common pattern across the test suite.
Learnt from: wazir-ahmed
Repo: sysown/proxysql PR: 5557
File: test/tap/tests/unit/gtid_set_unit-t.cpp:14-17
Timestamp: 2026-04-01T21:27:03.216Z
Learning: In ProxySQL's unit test directory (test/tap/tests/unit/), test_globals.h and test_init.h are only required for tests that depend on the ProxySQL runtime globals/initialization (i.e., tests that exercise components linked against libproxysql.a). Pure data-structure or utility tests (e.g., ezoption_parser_unit-t.cpp, gtid_set_unit-t.cpp, gtid_trxid_interval_unit-t.cpp) only need tap.h and the relevant project header — omitting test_globals.h and test_init.h is correct and intentional in these cases.
📚 Learning: 2026-01-20T09:34:27.165Z
Learnt from: yuji-hatakeyama
Repo: sysown/proxysql PR: 5307
File: test/tap/tests/reg_test_5306-show_warnings_with_comment-t.cpp:39-48
Timestamp: 2026-01-20T09:34:27.165Z
Learning: In ProxySQL test files (test/tap/tests/), resource leaks (such as not calling `mysql_close()` on early return paths) are not typically fixed because test processes are short-lived and the OS frees resources on process exit. This is a common pattern across the test suite.
Applied to files:
test/infra/infra-mysql57-binlog/docker-compose.yml
🔇 Additional comments (1)
test/infra/infra-mysql57-binlog/bin/docker-mysql-post.bash (1)
83-84: Good pre-reconfiguration hardening on replicas.Disabling session binlogging and explicitly stopping replication before reconfiguring the replica is a solid change for this flow.
|
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@test/infra/control/run-tests-isolated.bash`:
- Around line 444-446: The sleep command inside the bash -c payload uses
unescaped double quotes which will terminate the outer double-quoted string;
update the call that contains sleep "${BINLOG_READER_START_DELAY}" so the inner
quotes are escaped (\"${BINLOG_READER_START_DELAY}\") to match the existing
escaping pattern and prevent breaking the /bin/bash -c payload.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 02d19cc3-d5e4-413c-8e81-340e85786e21
📒 Files selected for processing (4)
test/infra/SKILL.mdtest/infra/control/run-tests-isolated.bashtest/infra/infra-mysql57-binlog/.envtest/tap/tests/Makefile
✅ Files skipped from review due to trivial changes (1)
- test/tap/tests/Makefile
🚧 Files skipped from review as they are similar to previous changes (1)
- test/infra/SKILL.md
📜 Review details
🧰 Additional context used
🧠 Learnings (4)
📓 Common learnings
Learnt from: yuji-hatakeyama
Repo: sysown/proxysql PR: 5307
File: test/tap/tests/reg_test_5306-show_warnings_with_comment-t.cpp:39-48
Timestamp: 2026-01-20T09:34:27.165Z
Learning: In ProxySQL test files (test/tap/tests/), resource leaks (such as not calling `mysql_close()` on early return paths) are not typically fixed because test processes are short-lived and the OS frees resources on process exit. This is a common pattern across the test suite.
Learnt from: wazir-ahmed
Repo: sysown/proxysql PR: 5557
File: test/tap/tests/unit/gtid_set_unit-t.cpp:14-17
Timestamp: 2026-04-01T21:27:03.216Z
Learning: In ProxySQL's unit test directory (test/tap/tests/unit/), test_globals.h and test_init.h are only required for tests that depend on the ProxySQL runtime globals/initialization (i.e., tests that exercise components linked against libproxysql.a). Pure data-structure or utility tests (e.g., ezoption_parser_unit-t.cpp, gtid_set_unit-t.cpp, gtid_trxid_interval_unit-t.cpp) only need tap.h and the relevant project header — omitting test_globals.h and test_init.h is correct and intentional in these cases.
Learnt from: CR
Repo: sysown/proxysql PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-04-12T11:27:41.142Z
Learning: Refer to `doc/agents/project-conventions.md` for ProxySQL-specific rules on directories, build, test harness, and git workflow
📚 Learning: 2026-04-01T21:27:03.216Z
Learnt from: wazir-ahmed
Repo: sysown/proxysql PR: 5557
File: test/tap/tests/unit/gtid_set_unit-t.cpp:14-17
Timestamp: 2026-04-01T21:27:03.216Z
Learning: In ProxySQL's unit test directory (test/tap/tests/unit/), test_globals.h and test_init.h are only required for tests that depend on the ProxySQL runtime globals/initialization (i.e., tests that exercise components linked against libproxysql.a). Pure data-structure or utility tests (e.g., ezoption_parser_unit-t.cpp, gtid_set_unit-t.cpp, gtid_trxid_interval_unit-t.cpp) only need tap.h and the relevant project header — omitting test_globals.h and test_init.h is correct and intentional in these cases.
Applied to files:
test/infra/control/run-tests-isolated.bashtest/infra/infra-mysql57-binlog/.env
📚 Learning: 2026-01-20T09:34:27.165Z
Learnt from: yuji-hatakeyama
Repo: sysown/proxysql PR: 5307
File: test/tap/tests/reg_test_5306-show_warnings_with_comment-t.cpp:39-48
Timestamp: 2026-01-20T09:34:27.165Z
Learning: In ProxySQL test files (test/tap/tests/), resource leaks (such as not calling `mysql_close()` on early return paths) are not typically fixed because test processes are short-lived and the OS frees resources on process exit. This is a common pattern across the test suite.
Applied to files:
test/infra/control/run-tests-isolated.bash
📚 Learning: 2026-01-20T07:40:34.938Z
Learnt from: yuji-hatakeyama
Repo: sysown/proxysql PR: 5307
File: test/tap/tests/reg_test_5306-show_warnings_with_comment-t.cpp:24-28
Timestamp: 2026-01-20T07:40:34.938Z
Learning: In ProxySQL test files, calling `mysql_error(NULL)` after `mysql_init()` failure is safe because the MariaDB client library implementation returns an empty string for NULL handles (not undefined behavior).
Applied to files:
test/infra/control/run-tests-isolated.bash
🪛 dotenv-linter (4.0.0)
test/infra/infra-mysql57-binlog/.env
[warning] 31-31: [SubstitutionKey] The BINLOG_READER_START_DELAY key is not assigned properly
(SubstitutionKey)
🪛 Shellcheck (0.11.0)
test/infra/control/run-tests-isolated.bash
[warning] 445-445: This { is literal. Check expression (missing ;/\n?) or quote it.
(SC1083)
[warning] 445-445: This } is literal. Check expression (missing ;/\n?) or quote it.
(SC1083)
🔇 Additional comments (3)
test/infra/infra-mysql57-binlog/.env (1)
31-31: Default delay declaration is consistent with the infra flow.Line 31 matches the existing shell-style default pattern in this
.envand aligns with how the value is propagated downstream.test/infra/control/run-tests-isolated.bash (2)
78-88: Infra-aware delay gating looks good.Line 78–Line 88 correctly avoids the startup wait when no
*-binloginfra is selected.
312-312: Container env propagation is correct.Line 312 cleanly passes
BINLOG_READER_START_DELAYinto the test-runner container where it is consumed.
- Align infra-mysql84-binlog with the mysql57-binlog infra behavior - Fix mysql84-binlog-reader wiring to use shared mysql service networking on port 6020 - Add binlog-reader startup delay plumbing for mysql84 infra - Update mysql84 replica bootstrap flow and ProxySQL test user setup Signed-off-by: Wazir Ahmed <wazir@proxysql.com>
- Take a different approach from 235b08a, which introduced `BINLOG_READER_START_DELAY` - Remove start-delay plumbing from ensure/run-test/binlog infra paths - Wait for each binlog reader to open port 6020 during infra startup - Keep reader retry loops simple with a fixed 5-second pre-start backoff - Fail ensure-infras early with logs if a reader never comes up Signed-off-by: Wazir Ahmed <wazir@proxysql.com>
wazir-ahmed
changed the title
infra-mysql57-binlog and infra-mysql84-binlog
There was a problem hiding this comment.
🧹 Nitpick comments (1)
test/infra/infra-mysql84-binlog/docker-compose.yml (1)
70-87: Minor: Inconsistent parameter expansion syntax.The command uses
:=forBINLOG_READER_USERbut:-for other parameters. While both work in bash,:-(default value) is the intended pattern here since you're not trying to modify the variable.Suggested fix for consistency
- proxysql_binlog_reader -h \"$${MYSQL_HOST:-127.0.0.1}\" -u \"$${BINLOG_READER_USER:=root}\" -p \"$${BINLOG_READER_PASSWORD:-root}\" -P \"$${MYSQL_PORT:-3306}\" -l \"$${LISTEN_PORT:-6020}\" -f 2>&1 | tee -a /var/log/mysqlbinlog/error.log; + proxysql_binlog_reader -h \"$${MYSQL_HOST:-127.0.0.1}\" -u \"$${BINLOG_READER_USER:-root}\" -p \"$${BINLOG_READER_PASSWORD:-root}\" -P \"$${MYSQL_PORT:-3306}\" -l \"$${LISTEN_PORT:-6020}\" -f 2>&1 | tee -a /var/log/mysqlbinlog/error.log;The same applies to
reader2(line 100) andreader3(line 119).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/infra/infra-mysql84-binlog/docker-compose.yml` around lines 70 - 87, The command for the reader services uses mixed parameter expansion (uses := for BINLOG_READER_USER) which is inconsistent with the intended default-only pattern; update the proxysql_binlog_reader command in the reader services (reader1, reader2, reader3) to use the default-value expansion form (:-) for BINLOG_READER_USER (and ensure BINLOG_READER_PASSWORD and other variables also use :- consistently) so all variables use the same default-only parameter expansion.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@test/infra/infra-mysql84-binlog/docker-compose.yml`:
- Around line 70-87: The command for the reader services uses mixed parameter
expansion (uses := for BINLOG_READER_USER) which is inconsistent with the
intended default-only pattern; update the proxysql_binlog_reader command in the
reader services (reader1, reader2, reader3) to use the default-value expansion
form (:-) for BINLOG_READER_USER (and ensure BINLOG_READER_PASSWORD and other
variables also use :- consistently) so all variables use the same default-only
parameter expansion.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 6e3e981f-23bc-4422-a37e-28f669e706ec
📒 Files selected for processing (10)
test/infra/control/ensure-infras.bashtest/infra/control/run-tests-isolated.bashtest/infra/infra-mysql57-binlog/docker-compose-destroy.bashtest/infra/infra-mysql57-binlog/docker-compose-init.bashtest/infra/infra-mysql57-binlog/docker-compose.ymltest/infra/infra-mysql84-binlog/bin/docker-mysql-post.bashtest/infra/infra-mysql84-binlog/conf/proxysql/infra-config.sqltest/infra/infra-mysql84-binlog/docker-compose-init.bashtest/infra/infra-mysql84-binlog/docker-compose.ymltest/tap/groups/groups.json
💤 Files with no reviewable changes (2)
- test/infra/control/run-tests-isolated.bash
- test/infra/infra-mysql57-binlog/docker-compose-destroy.bash
✅ Files skipped from review due to trivial changes (1)
- test/infra/control/ensure-infras.bash
🚧 Files skipped from review as they are similar to previous changes (1)
- test/infra/infra-mysql57-binlog/docker-compose.yml
📜 Review details
🧰 Additional context used
🧠 Learnings (8)
📓 Common learnings
Learnt from: wazir-ahmed
Repo: sysown/proxysql PR: 5557
File: test/tap/tests/unit/gtid_set_unit-t.cpp:14-17
Timestamp: 2026-04-01T21:27:03.216Z
Learning: In ProxySQL's unit test directory (test/tap/tests/unit/), test_globals.h and test_init.h are only required for tests that depend on the ProxySQL runtime globals/initialization (i.e., tests that exercise components linked against libproxysql.a). Pure data-structure or utility tests (e.g., ezoption_parser_unit-t.cpp, gtid_set_unit-t.cpp, gtid_trxid_interval_unit-t.cpp) only need tap.h and the relevant project header — omitting test_globals.h and test_init.h is correct and intentional in these cases.
Learnt from: yuji-hatakeyama
Repo: sysown/proxysql PR: 5307
File: test/tap/tests/reg_test_5306-show_warnings_with_comment-t.cpp:39-48
Timestamp: 2026-01-20T09:34:27.165Z
Learning: In ProxySQL test files (test/tap/tests/), resource leaks (such as not calling `mysql_close()` on early return paths) are not typically fixed because test processes are short-lived and the OS frees resources on process exit. This is a common pattern across the test suite.
📚 Learning: 2026-04-12T11:27:41.142Z
Learnt from: CR
Repo: sysown/proxysql PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-04-12T11:27:41.142Z
Learning: Refer to `doc/agents/project-conventions.md` for ProxySQL-specific rules on directories, build, test harness, and git workflow
Applied to files:
test/infra/infra-mysql84-binlog/conf/proxysql/infra-config.sql
📚 Learning: 2026-04-01T21:27:03.216Z
Learnt from: wazir-ahmed
Repo: sysown/proxysql PR: 5557
File: test/tap/tests/unit/gtid_set_unit-t.cpp:14-17
Timestamp: 2026-04-01T21:27:03.216Z
Learning: In ProxySQL's unit test directory (test/tap/tests/unit/), test_globals.h and test_init.h are only required for tests that depend on the ProxySQL runtime globals/initialization (i.e., tests that exercise components linked against libproxysql.a). Pure data-structure or utility tests (e.g., ezoption_parser_unit-t.cpp, gtid_set_unit-t.cpp, gtid_trxid_interval_unit-t.cpp) only need tap.h and the relevant project header — omitting test_globals.h and test_init.h is correct and intentional in these cases.
Applied to files:
test/infra/infra-mysql84-binlog/conf/proxysql/infra-config.sqltest/tap/groups/groups.json
📚 Learning: 2026-01-20T09:34:27.165Z
Learnt from: yuji-hatakeyama
Repo: sysown/proxysql PR: 5307
File: test/tap/tests/reg_test_5306-show_warnings_with_comment-t.cpp:39-48
Timestamp: 2026-01-20T09:34:27.165Z
Learning: In ProxySQL test files (test/tap/tests/), resource leaks (such as not calling `mysql_close()` on early return paths) are not typically fixed because test processes are short-lived and the OS frees resources on process exit. This is a common pattern across the test suite.
Applied to files:
test/infra/infra-mysql84-binlog/conf/proxysql/infra-config.sql
📚 Learning: 2026-04-12T11:27:41.142Z
Learnt from: CR
Repo: sysown/proxysql PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-04-12T11:27:41.142Z
Learning: Admin Interface implementation resides in `ProxySQL_Admin.cpp` and `Admin_Handler.cpp`, with schema versions tracked in `ProxySQL_Admin_Tables_Definitions.h`. Supports runtime config changes without restart via SQL-based configuration with SQLite3 backend
Applied to files:
test/infra/infra-mysql84-binlog/conf/proxysql/infra-config.sql
📚 Learning: 2026-03-26T16:39:02.446Z
Learnt from: yuji-hatakeyama
Repo: sysown/proxysql PR: 5548
File: lib/mysql_connection.cpp:1837-1843
Timestamp: 2026-03-26T16:39:02.446Z
Learning: In ProxySQL's lib/mysql_connection.cpp, `SHOW WARNINGS` detection for both `update_warning_count_from_connection()` and the `add_eof()` call in `ASYNC_USE_RESULT_CONT` intentionally uses `myds->sess->CurrentQuery.QueryParserArgs.digest_text` (comment-stripped digest text). This means the fix/feature does not work when `mysql-query_digests_keep_comment=1` (digest_text contains comments) or `mysql-query_digests=0` (digest_text is unavailable) — these configurations are explicitly excluded from the regression test for `reg_test_5306-show_warnings_with_comment-t`. This design is consistent across the codebase and is an accepted, documented limitation.
Applied to files:
test/infra/infra-mysql84-binlog/conf/proxysql/infra-config.sql
📚 Learning: 2026-01-20T07:40:34.938Z
Learnt from: yuji-hatakeyama
Repo: sysown/proxysql PR: 5307
File: test/tap/tests/reg_test_5306-show_warnings_with_comment-t.cpp:24-28
Timestamp: 2026-01-20T07:40:34.938Z
Learning: In ProxySQL test files, calling `mysql_error(NULL)` after `mysql_init()` failure is safe because the MariaDB client library implementation returns an empty string for NULL handles (not undefined behavior).
Applied to files:
test/infra/infra-mysql84-binlog/conf/proxysql/infra-config.sql
📚 Learning: 2026-04-12T11:27:41.142Z
Learnt from: CR
Repo: sysown/proxysql PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-04-12T11:27:41.142Z
Learning: Applies to test/tap/tests/unit/**/*.cpp : Unit tests must use `test_globals.h` and `test_init.h` header files and link against `libproxysql.a` via custom test harness
Applied to files:
test/tap/groups/groups.json
🪛 Shellcheck (0.11.0)
test/infra/infra-mysql84-binlog/docker-compose-init.bash
[info] 16-16: Not following: ./test/infra/control/docker-fs-helper.bash was not specified as input (see shellcheck -x).
(SC1091)
test/infra/infra-mysql57-binlog/docker-compose-init.bash
[info] 16-16: Not following: ./test/infra/control/docker-fs-helper.bash was not specified as input (see shellcheck -x).
(SC1091)
🔇 Additional comments (11)
test/infra/infra-mysql84-binlog/bin/docker-mysql-post.bash (1)
83-91: LGTM! Proper replica initialization sequence.The changes correctly prepare replicas by:
- Disabling binary logging (
SET SQL_LOG_BIN=0) to prevent local config from being logged- Stopping the replica before resetting binary logs (required ordering)
- Then configuring and starting replication
This is the correct sequence for MySQL 8.4 replica setup.
test/tap/groups/groups.json (1)
262-265: LGTM! Test group mappings updated for new binlog infrastructure.The binlog-related tests are now correctly mapped to include
mysql84-binlog-g1, aligning with the infrastructure changes that embed binlog readers within MySQL containers and expose port 6020.test/infra/infra-mysql84-binlog/docker-compose-init.bash (3)
16-17: LGTM! Helper sourcing is correct.The
docker-fs-helper.bashis sourced afterREPO_ROOTis established, providing thedocker_fs_execfunction for filesystem operations on container-owned paths.
81-82: LGTM! Filesystem permission handling improved.Using
docker_fs_execinstead of$SUDO chmodis more reliable for paths that may be owned by container users, consistent with the pattern used elsewhere in the codebase (e.g.,stop-proxysql-isolated.bash).
130-155: LGTM! Robust binlog reader readiness check.The readiness probe correctly:
- Uses TCP socket check via
/dev/tcpto verify port 6020 is listening- Implements reasonable timeout (60s) with progress indicators
- Provides comprehensive debugging output on failure (both MySQL and reader container logs)
test/infra/infra-mysql57-binlog/docker-compose-init.bash (3)
16-17: LGTM!Helper sourcing consistent with the mysql84-binlog infrastructure.
81-82: LGTM!Filesystem permission handling consistent with the mysql84-binlog infrastructure.
130-155: LGTM!Binlog reader readiness check consistent with the mysql84-binlog infrastructure.
test/infra/infra-mysql84-binlog/conf/proxysql/infra-config.sql (2)
6-6: LGTM! Comment accurately reflects the new topology.The comment correctly describes that binlog readers now share the
mysqlNnetwork namespace and listen ongtid_port 6020, matching the docker-compose configuration.
23-23: LGTM! Required user for binlog tests.The
testuserentry is needed by tests liketest_binlog_reader_uses_previous_hostgroup-twhich reference this user in their test logic.test/infra/infra-mysql84-binlog/docker-compose.yml (1)
85-85: LGTM! Network namespace sharing is correct.Using
network_mode: service:mysql1allows the reader to connect to MySQL via127.0.0.1:3306and expose port 6020 on the MySQL container's network interface, which is exactly what the readiness checks and ProxySQLgtid_portconfiguration expect.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Replace the external binlog reader with a helper function which uses mariadb replication functions.
COM_REGISTER_SLAVE, and fetches heartbeats to verify the stream is active.test_com_register_slave_enables_fast_forward-ttest_binlog_reader_uses_previous_hostgroup-tFixed following issues in
*-binloginfrasinfra-mysql57-binlogavoid sidecar and use binlog-reader installed in the MySQL containersudoin bash scriptsSummary by CodeRabbit
Tests
Chores