Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Use uint64 for binary log file position #17472

Open
wants to merge 14 commits into
base: main
Choose a base branch
from
Open

Use uint64 for binary log file position #17472

wants to merge 14 commits into from

Conversation

mattlord
Copy link
Contributor

@mattlord mattlord commented Jan 7, 2025 •
edited
Loading

Description

The binary log file position is stored in MySQL as a ulongulong or uint64:

But as you can see in the PR changes we stored the file position internally as a uint32.

The modified tests fail as expected w/o the type changes in the PR and pass in the PR branch, for example:

❯ go test -timeout 30s -run ^TestEmergencyReparentShard$ vitess.io/vitess/go/vt/wrangler/testlib
...
--- FAIL: TestEmergencyReparentShard (0.02s)
    emergency_reparent_shard_test.go:136:
                Error Trace:    /Users/matt/git/vitess-private/go/vt/wrangler/testlib/emergency_reparent_shard_test.go:136
                Error:          Received unexpected error:
                                invalid FilePos GTID (relay-bin.003222:18321744073709551612): expecting pos to be an integer
                Test:           TestEmergencyReparentShard

I ended up stripping the PR down so I think it's a pretty safe and easy thing to backport to all GA releases.

Related Issue(s)

Checklist

  • "Backport to:" labels have been added if this change should be back-ported to release branches
  • If this change is to be back-ported to previous releases, a justification is included in the PR description
  • Tests were added or are not required
  • Did the new or modified tests pass consistently locally and on CI?
  • Documentation was added or is not required

@vitess-bot vitess-bot
Copy link
Contributor

vitess-bot bot commented Jan 7, 2025

Review Checklist

Hello reviewers! 👋 Please follow this checklist when reviewing this Pull Request.

General

  • Ensure that the Pull Request has a descriptive title.
  • Ensure there is a link to an issue (except for internal cleanup and flaky test fixes), new features should have an RFC that documents use cases and test cases.

Tests

  • Bug fixes should have at least one unit or end-to-end test, enhancement and new features should have a sufficient number of tests.

Documentation

  • Apply the release notes (needs details) label if users need to know about this change.
  • New features should be documented.
  • There should be some code comments as to why things are implemented the way they are.
  • There should be a comment at the top of each new or modified test to explain what the test does.

New flags

  • Is this flag really necessary?
  • Flag names must be clear and intuitive, use dashes (-), and have a clear help text.

If a workflow is added or modified:

  • Each item in Jobs should be named in order to mark it as required.
  • If the workflow needs to be marked as required, the maintainer team must be notified.

Backward compatibility

  • Protobuf changes should be wire-compatible.
  • Changes to _vt tables and RPCs need to be backward compatible.
  • RPC changes should be compatible with vitess-operator
  • If a flag is removed, then it should also be removed from vitess-operator and arewefastyet, if used there.
  • vtctl command output order should be stable and awk-able.

@vitess-bot vitess-bot bot added NeedsBackportReason If backport labels have been applied to a PR, a justification is required NeedsDescriptionUpdate The description is not clear or comprehensive enough, and needs work NeedsIssue A linked issue is missing for this Pull Request NeedsWebsiteDocsUpdate What it says labels Jan 7, 2025
@github-actions github-actions bot added this to the v22.0.0 milestone Jan 7, 2025
@mattlord mattlord added Component: Query Serving Component: VReplication Type: Bug and removed NeedsWebsiteDocsUpdate What it says NeedsIssue A linked issue is missing for this Pull Request NeedsDescriptionUpdate The description is not clear or comprehensive enough, and needs work NeedsBackportReason If backport labels have been applied to a PR, a justification is required labels Jan 7, 2025
@mattlord
Fix unit tests
85389bb 
Signed-off-by: Matt Lord <[email protected]>
@mattlord
Adjustments and fixes
11d0404 
Signed-off-by: Matt Lord <[email protected]>
@mattlord mattlord force-pushed the binlog_filepos_uint64 branch from 740bc3f to 11d0404 Compare January 7, 2025 18:29
@mattlord mattlord force-pushed the binlog_filepos_uint64 branch from 73618f8 to bb933e0 Compare January 7, 2025 19:32
@codecov Codecov
Copy link

codecov bot commented Jan 7, 2025 •
edited
Loading

Codecov Report

Attention: Patch coverage is 75.00000% with 6 lines in your changes missing coverage. Please review.

Project coverage is 67.67%. Comparing base (69e0224) to head (cc53c8c).
Report is 4 commits behind head on main.

Files with missing lines Patch % Lines
go/mysql/binlog_event_common.go 0.00% 3 Missing ⚠️
go/vt/vtorc/inst/binlog.go 33.33% 2 Missing ⚠️
go/mysql/binlog_event_filepos.go 75.00% 1 Missing ⚠️
Additional details and impacted files 
@@            Coverage Diff             @@
##             main   #17472      +/-   ##
==========================================
+ Coverage   67.66%   67.67%   +0.01%     
==========================================
  Files        1584     1584              
  Lines      254394   254472      +78     
==========================================
+ Hits       172139   172220      +81     
+ Misses      82255    82252       -3

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@mattlord mattlord added the Component: VTorc Vitess Orchestrator integration label Jan 7, 2025
@mattlord
Remove BinlogFilePos code
4e54f15 
In the end it wasn't worth it as we only used it for one variable
and it made backporting more risky/difficult.

Signed-off-by: Matt Lord <[email protected]>
@mattlord
Add back missing test change
6ba0308 
Signed-off-by: Matt Lord <[email protected]>
@mattlord
Changes from self review
ea23519 
Signed-off-by: Matt Lord <[email protected]>
@mattlord mattlord added Backport to: release-19.0 Needs to be back ported to release-19.0 Backport to: release-20.0 Needs to be backport to release-20.0 labels Jan 7, 2025
@mattlord mattlord added the Backport to: release-21.0 Needs to be backport to release-21.0 label Jan 7, 2025
@mattlord
Add missing test
347d371 
Signed-off-by: Matt Lord <[email protected]>
@mattlord mattlord marked this pull request as ready for review January 7, 2025 23:49
timvaillancourt
timvaillancourt reviewed Jan 8, 2025
View reviewed changes
go/mysql/flavor_filepos.go Outdated
@@ -191,21 +191,21 @@ func (flv *filePosFlavor) readBinlogEvent(c *Conn) (BinlogEvent, error) {
eDeleteRowsEventV0, eDeleteRowsEventV1, eDeleteRowsEventV2,
eUpdateRowsEventV0, eUpdateRowsEventV1, eUpdateRowsEventV2:
flv.savedEvent = event
return newFilePosGTIDEvent(flv.file, event.nextPosition(flv.format), event.Timestamp()), nil
return newFilePosGTIDEvent(flv.file, uint64(event.nextPosition(flv.format)), event.Timestamp()), nil
Copy link
Contributor

@timvaillancourt timvaillancourt Jan 8, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mattlord could we update event.nextPosition(flv.format) to return uint64 instead? Currently it returns uint32

Copy link
Contributor Author

@mattlord mattlord Jan 8, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was on the fence about that since we only ever seem to treat it as an integral and it's a uint32 in the protocol, but this does make it more uniform so I'm good with it. I did that here: 7a45ff5

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@timvaillancourt timvaillancourt timvaillancourt approved these changes

@dbussink dbussink dbussink approved these changes

@deepthi deepthi Awaiting requested review from deepthi deepthi is a code owner

@rohit-nayak-ps rohit-nayak-ps Awaiting requested review from rohit-nayak-ps rohit-nayak-ps is a code owner

@shlomi-noach shlomi-noach Awaiting requested review from shlomi-noach shlomi-noach is a code owner

@GuptaManan100 GuptaManan100 Awaiting requested review from GuptaManan100 GuptaManan100 is a code owner

@harshit-gangal harshit-gangal Awaiting requested review from harshit-gangal harshit-gangal is a code owner

@systay systay Awaiting requested review from systay systay is a code owner

Assignees
No one assigned
Labels
Backport to: release-19.0 Needs to be back ported to release-19.0 Backport to: release-20.0 Needs to be backport to release-20.0 Backport to: release-21.0 Needs to be backport to release-21.0 Component: Query Serving Component: VReplication Component: VTorc Vitess Orchestrator integration Type: Bug
Projects
None yet
Milestone
v22.0.0
Development

Successfully merging this pull request may close these issues.

Bug Report: parsed GTID out of range
3 participants
@mattlord @dbussink @timvaillancourt