VReplication: Move ENUM and SET mappings from vplayer to vstreamer #15723
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
force-pushed
the
vstream_enum_strings
branch
2 times, most recently
from
640fec5
to
8385a0c
Compare
Signed-off-by: Matt Lord <mattalord@gmail.com>
mattlord
force-pushed
the
vstream_enum_strings
branch
from
8385a0c
to
2200fcc
Compare
Signed-off-by: Matt Lord <mattalord@gmail.com>
mattlord
force-pushed
the
vstream_enum_strings
branch
from
6b521d4
to
0fee7d4
Compare
|
Cross reference for convenience: Online DDL/VReplication test suite: support ENUM as part of PRIMARY KEY |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #15723 +/- ##
==========================================
- Coverage 68.44% 68.42% -0.02%
==========================================
Files 1558 1559 +1
Lines 195822 196825 +1003
==========================================
+ Hits 134025 134685 +660
- Misses 61797 62140 +343 ☔ View full report in Codecov by Sentry. |
Signed-off-by: Matt Lord <mattalord@gmail.com>
Signed-off-by: Matt Lord <mattalord@gmail.com>
mattlord
force-pushed
the
vstream_enum_strings
branch
from
ee27c2a
to
c12a878
Compare
Signed-off-by: Matt Lord <mattalord@gmail.com>
Signed-off-by: Matt Lord <mattalord@gmail.com>
Signed-off-by: Matt Lord <mattalord@gmail.com>
And remove some noisy debug logging. And remove any OnlineDDL changes for now. Those changes were premature. Signed-off-by: Matt Lord <mattalord@gmail.com>
mattlord
force-pushed
the
vstream_enum_strings
branch
from
2133974
to
1f10e2a
Compare
mattlord
added
Component: VReplication
Type: Enhancement
Signed-off-by: Matt Lord <mattalord@gmail.com>
mattlord
force-pushed
the
vstream_enum_strings
branch
from
57932f8
to
d2fed3c
Compare
Signed-off-by: Matt Lord <mattalord@gmail.com>
It was never failing before others pushed commits 🙂 |
|
Yes, it does look to have been d2bfdae ! I reverted and pushed. |
Signed-off-by: Matt Lord <mattalord@gmail.com>
Signed-off-by: Matt Lord <mattalord@gmail.com>
Signed-off-by: Matt Lord <mattalord@gmail.com>
|
Thank you for the reviews, @shlomi-noach and @rohit-nayak-ps ! I believe that I have addressed all of your comments now. |
Signed-off-by: Matt Lord <mattalord@gmail.com>
Signed-off-by: Matt Lord <mattalord@gmail.com>
Signed-off-by: Matt Lord <mattalord@gmail.com>
shlomi-noach
approved these changes
May 2, 2024
Signed-off-by: Matt Lord <mattalord@gmail.com>
Signed-off-by: Matt Lord <mattalord@gmail.com>
Signed-off-by: Matt Lord <mattalord@gmail.com>
Signed-off-by: Matt Lord <mattalord@gmail.com>
Signed-off-by: Matt Lord <mattalord@gmail.com>
rohit-nayak-ps
approved these changes
May 2, 2024
Signed-off-by: Matt Lord <mattalord@gmail.com>
mattlord
force-pushed
the
vstream_enum_strings
branch
from
fc19138
to
02b6642
Compare
Signed-off-by: Matt Lord <mattalord@gmail.com>
twthorn
reviewed
May 8, 2024
Signed-off-by: Matt Lord <mattalord@gmail.com>
twthorn
approved these changes
May 8, 2024
mattlord
removed
the
release notes (needs details)
shlomi-noach
approved these changes
May 9, 2024
Signed-off-by: Matt Lord <mattalord@gmail.com>
…markdown viewer -- going back to text Signed-off-by: Matt Lord <mattalord@gmail.com>
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 addresses the issues noted #15750. You can see how to perform a manual test there as well.
It does this by removing the existing
ENUMandSETfield integer to string value mapping from thevplayer(consumer — where it was relying on OnlineDDL specific code that I think can be at least partially removed after this) and moving this work to thevstreamer(producer) so that Vitess provides uniform and intuitive vstream vevent behavior forENUMandSETfields regardless of what phase the stream or workflow may be in.In discussing this issue with the Debezium Vitess connector community they had asked if we could add something to the stream which would indicate that they do NOT need to do the string mappings in order to make it easier for them to transition from <= v19 to v20+ of Vitess. To support this I added a new flag to the
FieldEventtype which will indicate when theENUMandSETvalues are already strings. We always set that inFieldEvents during the copy phase as 1) we've always provided those values as strings in the copy phase and 2) we have one field event per row event batch (vstream_packet) so that extra byte is negligible and is cheaper than always searching the columns to see if there are anySETorENUMcolumns. During the running phase, however, we know when we're managing the string mappings for a table and we only set this field when the table we're streaming events for contains aSETorENUMcolumn.This PR also fixes #15598 after the evalengine portion of it was fixed in #15783.
Related Issue(s)
Checklist