Parse GTID ranges on proxysql_mysqlbinlog update messages.#5427
Parse GTID ranges on proxysql_mysqlbinlog update messages.#5427proton-lisandro-pin wants to merge 1 commit intosysown:feature/gtid-range-updatefrom
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughThis change refactors GTID (Global Transaction ID) handling by introducing structured classes ( Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
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 Tip CodeRabbit can use Trivy to scan for security misconfigurations and secrets in Infrastructure as Code files.Add a .trivyignore file to your project to customize which findings Trivy reports. |
Summary of ChangesHello @proton-lisandro-pin, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request enhances ProxySQL's GTID handling by adding support for GTID ranges in Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Changelog
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![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 is a significant and valuable refactoring of the GTID handling logic. Encapsulating GTID sets and UUID:GTID pairs into their own classes (Gtid_Set, Uuid_Gtid) greatly improves code organization, clarity, and maintainability. The rest of the codebase is correctly updated to use these new abstractions. My review focuses on a few areas where further improvements can be made, including fixing a potential parsing bug, enhancing performance, and adhering to modern C++ best practices.
proton-lisandro-pin
force-pushed
the
ranged_gtid_updates_2
branch
from
c8ede20 to
acb02d4
Compare
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (5)
lib/GTID_Server_Data.cpp (2)
263-265: Prefer C++ constructor syntax over C-style casts.Using explicit constructors is more idiomatic C++ and clearer in intent.
♻️ Proposed refactor
bool GTID_Server_Data::gtid_exists(char *gtid_uuid, uint64_t gtid_trxid) { - return gtid_executed.has_gtid((std::string)gtid_uuid, (gtid_t)gtid_trxid); + return gtid_executed.has_gtid(std::string(gtid_uuid), static_cast<gtid_t>(gtid_trxid)); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/GTID_Server_Data.cpp` around lines 263 - 265, The function GTID_Server_Data::gtid_exists is using C-style casts; replace them with C++ constructors/casts to be idiomatic: construct the std::string using std::string(gtid_uuid) and convert gtid_trxid to gtid_t with a static_cast<gtid_t>(gtid_trxid) when calling gtid_executed.has_gtid to preserve types and intent.
368-386: Prefer C++ constructor syntax for consistency.Same suggestion as line 264: use
std::string(uuid_server)instead of C-style cast.♻️ Proposed refactor
switch (rec_msg[1]) { case '1': a = strchr(rec_msg+3,':'); ul = a-rec_msg-3; strncpy(uuid_server,rec_msg+3,ul); uuid_server[ul] = 0; - gtid_executed.add((std::string)uuid_server, a+1); + gtid_executed.add(std::string(uuid_server), a+1); break; case '2': - gtid_executed.add((std::string)uuid_server, rec_msg+3); + gtid_executed.add(std::string(uuid_server), rec_msg+3); break;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/GTID_Server_Data.cpp` around lines 368 - 386, Replace the C-style casts to std::string in the GTID handling branch: where gtid_executed.add((std::string)uuid_server, ...) is used (inside the 'I' message handling switch cases), construct the std::string via its constructor instead (std::string(uuid_server)) before passing to gtid_executed.add so the code uses consistent C++ constructor syntax for uuid_server.lib/proxysql_gtid.cpp (3)
227-239: Markhas_gtid()asconst.This query method doesn't modify the object's state.
Suggested fix
-const bool Gtid_Set::has_gtid(const std::string& uuid, const gtid_t gtid) { +const bool Gtid_Set::has_gtid(const std::string& uuid, const gtid_t gtid) const {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/proxysql_gtid.cpp` around lines 227 - 239, The method Gtid_Set::has_gtid is a query that doesn't modify object state; change its signature to be const (i.e., make has_gtid a const member function) so it can be called on const Gtid_Set instances and to reflect its non-mutating behavior; update the declaration and definition of Gtid_Set::has_gtid accordingly so both use the const qualifier.
10-11: Pass_uuidby const reference to avoid unnecessary copy.The string parameter is copied on function call, then copied again to initialize the member. Use
const std::string&instead.Suggested fix
-Uuid_Gtid::Uuid_Gtid(const std::string _uuid, const gtid_t _gtid) : uuid(_uuid), gtid(_gtid) { +Uuid_Gtid::Uuid_Gtid(const std::string& _uuid, const gtid_t _gtid) : uuid(_uuid), gtid(_gtid) { }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/proxysql_gtid.cpp` around lines 10 - 11, The constructor Uuid_Gtid::Uuid_Gtid currently takes the string parameter as std::string _uuid which causes an extra copy; change the signature to take the UUID parameter as const std::string& (e.g., const std::string& _uuid) and initialize the member uuid(_uuid) as before so only one copy occurs; keep gtid_t _gtid as-is (or consider const reference if gtid_t is large) and update any declarations/forwarders that call Uuid_Gtid::Uuid_Gtid to match the new parameter type.
14-17: Markcopy()asconst.This method doesn't modify the object's state.
Suggested fix
-Uuid_Gtid Uuid_Gtid::copy() { +Uuid_Gtid Uuid_Gtid::copy() const { return Uuid_Gtid(uuid, gtid); - }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/proxysql_gtid.cpp` around lines 14 - 17, The Uuid_Gtid::copy() method is immutable but not declared const; update its declaration and definition to be const by adding a trailing const to the method signature (i.e., change Uuid_Gtid Uuid_Gtid::copy() to Uuid_Gtid Uuid_Gtid::copy() const) so it can be called on const instances and correctly signals it doesn't modify object state; ensure any corresponding declaration in the class header is updated the same way.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@include/proxysql_gtid.h`:
- Line 25: Fix the typo in the header comment string "Encapuslates an interval
of GTIDs." to read "Encapsulates an interval of GTIDs." so the documentation in
include/proxysql_gtid.h is correct; update the comment that currently contains
"Encapuslates" to "Encapsulates".
- Line 50: Fix the spelling of "Encapuslates" to "Encapsulates" in the comment
that describes the map of "UUID -> GTID intervals." Update both occurrences of
the misspelling in include/proxysql_gtid.h (the current comment and the earlier
duplicate in the same file) so the header comment text reads "Encapsulates a map
of UUID -> GTID intervals." and matches style with other comments.
In `@lib/proxysql_gtid.cpp`:
- Around line 242-259: Gtid_Set::to_string currently omits a separator between
groups for different UUID keys because the comma is only added between intervals
inside the same uuid loop; update the outer loop over map in Gtid_Set::to_string
so that before emitting the first interval for a new uuid you prepend a
separator if this is not the first uuid (e.g., track whether this is the first
map entry or check previous output length), ensuring you output "," between
different uuid groups while keeping the existing comma logic for itr in
it->second.
In `@test/tap/tests/unit-gtid_interval-t.cpp`:
- Around line 89-105: The TAP plan count function testGtidSetAdd_Count currently
returns 8 but testGtidSetAdd executes 9 ok() assertions; update
testGtidSetAdd_Count to return 9 so the TAP plan matches the number of
assertions in testGtidSetAdd (look for the functions testGtidSetAdd_Count and
testGtidSetAdd and adjust the integer returned by testGtidSetAdd_Count).
---
Nitpick comments:
In `@lib/GTID_Server_Data.cpp`:
- Around line 263-265: The function GTID_Server_Data::gtid_exists is using
C-style casts; replace them with C++ constructors/casts to be idiomatic:
construct the std::string using std::string(gtid_uuid) and convert gtid_trxid to
gtid_t with a static_cast<gtid_t>(gtid_trxid) when calling
gtid_executed.has_gtid to preserve types and intent.
- Around line 368-386: Replace the C-style casts to std::string in the GTID
handling branch: where gtid_executed.add((std::string)uuid_server, ...) is used
(inside the 'I' message handling switch cases), construct the std::string via
its constructor instead (std::string(uuid_server)) before passing to
gtid_executed.add so the code uses consistent C++ constructor syntax for
uuid_server.
In `@lib/proxysql_gtid.cpp`:
- Around line 227-239: The method Gtid_Set::has_gtid is a query that doesn't
modify object state; change its signature to be const (i.e., make has_gtid a
const member function) so it can be called on const Gtid_Set instances and to
reflect its non-mutating behavior; update the declaration and definition of
Gtid_Set::has_gtid accordingly so both use the const qualifier.
- Around line 10-11: The constructor Uuid_Gtid::Uuid_Gtid currently takes the
string parameter as std::string _uuid which causes an extra copy; change the
signature to take the UUID parameter as const std::string& (e.g., const
std::string& _uuid) and initialize the member uuid(_uuid) as before so only one
copy occurs; keep gtid_t _gtid as-is (or consider const reference if gtid_t is
large) and update any declarations/forwarders that call Uuid_Gtid::Uuid_Gtid to
match the new parameter type.
- Around line 14-17: The Uuid_Gtid::copy() method is immutable but not declared
const; update its declaration and definition to be const by adding a trailing
const to the method signature (i.e., change Uuid_Gtid Uuid_Gtid::copy() to
Uuid_Gtid Uuid_Gtid::copy() const) so it can be called on const instances and
correctly signals it doesn't modify object state; ensure any corresponding
declaration in the class header is updated the same way.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: f14c9617-f9d2-430f-a379-e6543fea2ebb
📒 Files selected for processing (7)
include/GTID_Server_Data.hinclude/MySQL_HostGroups_Manager.hinclude/proxysql_gtid.hlib/GTID_Server_Data.cpplib/MySQL_HostGroups_Manager.cpplib/proxysql_gtid.cpptest/tap/tests/unit-gtid_interval-t.cpp
💤 Files with no reviewable changes (2)
- include/MySQL_HostGroups_Manager.h
- include/GTID_Server_Data.h
This PR adds full support for ranges in `proxysql_mysqlbinlog` update messages, while also improving efficiency when dealing with GTID gaps for any given UUID. Single GTID updates are transparently supported, so older `proxysql_mysqlbinlog` version will continue to work with newer ProxySQL releases. See sysown/proxysql_mysqlbinlog#33 for details.
proton-lisandro-pin
force-pushed
the
ranged_gtid_updates_2
branch
from
acb02d4 to
96f1b44
Compare
|
2 participants
Following up on #5224, this PR adds full support for ranges in
proxysql_mysqlbinlogupdate messages, while also improving efficiency when dealing with GTID gaps for any given UUID.Single GTID updates are transparently supported, so older
proxysql_mysqlbinlogversion will continue to work with newer ProxySQL releases.See sysown/proxysql_mysqlbinlog#33 for details.
Once this change is merged, i'll upload the last PR to batch GTID updates as ranges in
proxysql_mysqlbinlog.Summary by CodeRabbit
New Features
Refactor
Tests