VReplication: Enable VPlayerBatching in unit tests #17339
Merged
Conversation
This file contains 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
Review ChecklistHello reviewers! 👋 Please follow this checklist when reviewing this Pull Request. General
Tests
Documentation
New flags
If a workflow is added or modified:
Backward compatibility
|
vitess-bot
bot
added
NeedsBackportReason
mattlord
removed
NeedsWebsiteDocsUpdate
mattlord
force-pushed
the
vplayerbatching_unit_tests
branch
from
22b1c1d to
f8a3fad
Compare
mattlord
removed
NeedsDescriptionUpdate
mattlord
commented
Dec 5, 2024
| @@ -141,7 +141,6 @@ func setup(ctx context.Context) (func(), int) { | |||
| resetBinlogClient() | |||
|
|
|||
| vttablet.InitVReplicationConfigDefaults() | |||
| vttablet.DefaultVReplicationConfig.ExperimentalFlags = 0 | |||
There was a problem hiding this comment.
This is where the flags were disabled for all of the unit tests and I did not notice it previously.
There was a problem hiding this comment.
If we want this PR to be able to stand on its own, I could change this line to vttablet.DefaultVReplicationConfig.ExperimentalFlags = 7 rather than removing it.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #17339 +/- ##
==========================================
- Coverage 67.42% 67.42% -0.01%
==========================================
Files 1575 1576 +1
Lines 253324 253429 +105
==========================================
+ Hits 170816 170873 +57
- Misses 82508 82556 +48 ☔ View full report in Codecov by Sentry. |
mattlord
force-pushed
the
vplayerbatching_unit_tests
branch
4 times, most recently
from
e552d3c to
fc4187d
Compare
Signed-off-by: Matt Lord <[email protected]>
mattlord
force-pushed
the
vplayerbatching_unit_tests
branch
from
fc4187d to
e1d2649
Compare
mattlord
commented
Dec 5, 2024
Signed-off-by: Matt Lord <[email protected]>
mattlord
commented
Dec 5, 2024
Signed-off-by: Matt Lord <[email protected]>
mattlord
commented
Dec 5, 2024
mattlord
commented
Dec 5, 2024
dbussink
approved these changes
Dec 5, 2024
| // starts with the BEGIN and ends with the COMMIT, so we | ||
| // do not send a BEGIN down the wire ahead of time. | ||
| vc.queriesPos = int64(len(vc.queries)) | ||
| vc.batchSize = 6 // begin and semicolon |
There was a problem hiding this comment.
Looks like it's not new here and moved, but should we avoid the magic constant here?
There was a problem hiding this comment.
It's not a magic constant, it's just the known length of "begin;"
Signed-off-by: Matt Lord <[email protected]>
mattlord
commented
Dec 5, 2024
Comment on lines
+511
to
+518
| // If we're batching a transaction, then include the state update | ||
| // in the current transaction batch. | ||
| if vr.dbClient.InTransaction && vr.dbClient.maxBatchSize > 0 { | ||
| vr.dbClient.AddQueryToTrxBatch(query) | ||
| } else { // Otherwise, send it down the wire | ||
| if _, err := vr.dbClient.ExecuteFetch(query, 1); err != nil { | ||
| return fmt.Errorf("could not set state: %v: %v", query, err) | ||
| } |
There was a problem hiding this comment.
This is the fix for the noted extraneous state update that surfaced in the unit tests. This caused no known issues, but was unnecessary and corrected here.
shlomi-noach
approved these changes
Dec 5, 2024
| // starts with the BEGIN and ends with the COMMIT, so we | ||
| // do not send a BEGIN down the wire ahead of time. | ||
| vc.queriesPos = int64(len(vc.queries)) | ||
| vc.batchSize = 6 // begin and semicolon |
There was a problem hiding this comment.
Let's make it clearer in the comment:
Suggested change
| vc.batchSize = 6 // begin and semicolon | |
| vc.batchSize = 6 // equal to `len("begin;")` |
There was a problem hiding this comment.
Addressed it here: 26f93d4
Signed-off-by: Matt Lord <[email protected]>
mattlord
commented
Dec 5, 2024
Comment on lines
+61
to
+72
| if vc.maxBatchSize > 0 { | ||
| // We are batching the contents of the transaction, which | ||
| // starts with the BEGIN and ends with the COMMIT, so we | ||
| // do not send a BEGIN down the wire ahead of time. | ||
| vc.queriesPos = int64(len(vc.queries)) | ||
| vc.batchSize = beginStmtLen | ||
| } else { | ||
| // We're not batching so we start the transaction here | ||
| // by sending the BEGIN down the wire. | ||
| if err := vc.DBClient.Begin(); err != nil { | ||
| return err | ||
| } |
There was a problem hiding this comment.
This is the fix for the noted extraneous BEGIN that surfaced in the unit tests. This was harmless, but also unnecessary and corrected here.
rohit-nayak-ps
approved these changes
Dec 6, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Description
This is a quick follow-up to #17166. When working on that, I did not realize that the unit test framework was explicitly disabling all experimental flags and thus we were not testing the new flag/default behavior. This PR removes that
vttablet.DefaultVReplicationConfig.ExperimentalFlags = 0line inframework_test.goand adjusts the unit test expectations accordingly.In working on this, it surfaced an issue with an extraneous BEGIN being used when in batching mode that was also corrected in
vdbclient.go. Similarly, the unit tests surfaced an extraneous state update that was corrected here invreplicator.go. Lastly, it also updates the vplayer so that batching is only done when in the running/replicating phase as intended.Related Issue(s)
Checklist