-
Notifications
You must be signed in to change notification settings - Fork 2.1k
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.
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
VReplication: Move ENUM and SET mappings from vplayer to vstreamer #15723
Conversation
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
|
640fec5
to
8385a0c
Compare
Signed-off-by: Matt Lord <[email protected]>
8385a0c
to
2200fcc
Compare
Signed-off-by: Matt Lord <[email protected]>
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 <[email protected]>
Signed-off-by: Matt Lord <[email protected]>
ee27c2a
to
c12a878
Compare
Signed-off-by: Matt Lord <[email protected]>
Signed-off-by: Matt Lord <[email protected]>
Signed-off-by: Matt Lord <[email protected]>
And remove some noisy debug logging. And remove any OnlineDDL changes for now. Those changes were premature. Signed-off-by: Matt Lord <[email protected]>
2133974
to
1f10e2a
Compare
Signed-off-by: Matt Lord <[email protected]>
57932f8
to
d2fed3c
Compare
Signed-off-by: Matt Lord <[email protected]>
Yes, it does look to have been d2bfdae ! I reverted and pushed. |
Signed-off-by: Matt Lord <[email protected]>
Signed-off-by: Matt Lord <[email protected]>
Signed-off-by: Matt Lord <[email protected]>
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 <[email protected]>
Signed-off-by: Matt Lord <[email protected]>
Signed-off-by: Matt Lord <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
minor styling comment.
Signed-off-by: Matt Lord <[email protected]>
Signed-off-by: Matt Lord <[email protected]>
Signed-off-by: Matt Lord <[email protected]>
Signed-off-by: Matt Lord <[email protected]>
Signed-off-by: Matt Lord <[email protected]>
Signed-off-by: Matt Lord <[email protected]>
fc19138
to
02b6642
Compare
Signed-off-by: Matt Lord <[email protected]>
Signed-off-by: Matt Lord <[email protected]>
Signed-off-by: Matt Lord <[email protected]>
…markdown viewer -- going back to text Signed-off-by: Matt Lord <[email protected]>
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
ENUM
andSET
field 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 forENUM
andSET
fields 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
FieldEvent
type which will indicate when theENUM
andSET
values are already strings. We always set that inFieldEvent
s 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 anySET
orENUM
columns. 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 aSET
orENUM
column.This PR also fixes #15598 after the evalengine portion of it was fixed in #15783.
Related Issue(s)
Checklist