-
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
Clear existing keyspace schema in vtgate before [re]loading it #9437
Clear existing keyspace schema in vtgate before [re]loading it #9437
Conversation
0663db4
to
4a3225d
Compare
6304de5
to
af7224b
Compare
2f8cb15
to
f4f876a
Compare
f4f876a
to
e227c54
Compare
// Enable schema tracking | ||
clusterInstance.VtGateExtraArgs = []string{"-schema_change_signal"} | ||
clusterInstance.VtTabletExtraArgs = []string{ | ||
"-queryserver-config-schema-change-signal", | ||
fmt.Sprintf("-queryserver-config-schema-change-signal-interval=%d", signalInterval), | ||
} | ||
|
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.
this changes the existing test, would prefer a new test for the fix.
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.
How so? The signal was getting enabled before on line 89, now it's here as it should be IMO. And the non-default interval isn't necessary, it just decreases the runtime of the tests.
Am I missing something?
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.
The earlier test was testing that
VTGate started with schema tracking
VTTablet started with no-schema tracking
on restart of VTTablet, schema tracking was enabled.
VTGate should be able to receive the schema information and update it's vschema post VTTablet restart.
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.
Regarding the restarttablet
test suite, I agree with you @mattlord, I think we can use a non-default interval as it can decrease the runtime of the tests. But, we need to keep setting the -queryserver-config-schema-change-signal
flag to true
after the tablets have already started, as @harshit-gangal said, so we can keep the old behavior.
I don't think that setting the flag after the tablet's first start and before its restart shouldn't impact your test (TestVSchemaTrackerKeyspaceReInit
). If it does we will probably need a new TestMain
.
The pull request looks great to me! thank you 👌🏻
Ah, I now see what you mean. I'll correct this now. Thanks, @harshit-gangal and @frouioui ! |
@harshit-gangal and @frouioui I believe that this simple change — where we set the tablet's schema change flags after cluster creation — gets us what we wanted here so that the existing test is unchanged (we start the tablet w/o schema tracking enabled, then restart it with it enabled). With that commit, the full relevant portion of the diff is now:
Please let me know if I missed or misunderstood something though. Thanks again! |
b52d325
to
d4f60d4
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>
This keeps the init test unchanged Signed-off-by: Matt Lord <mattalord@gmail.com>
d4f60d4
to
aeb4d93
Compare
tablet.VttabletProcess.ExtraArgs = []string{"-queryserver-config-schema-change-signal"} | ||
tablet.VttabletProcess.ExtraArgs = []string{ | ||
"-queryserver-config-schema-change-signal", | ||
fmt.Sprintf("-queryserver-config-schema-change-signal-interval=%d", signalInterval), |
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.
nit: easier way to give this is
[]string{...., "-queryserver-config-schema-change-signal-interval", strconv.Itoa(signalInterval), .... }
Description
A
vtgate
listens for schema change signals from the primary tablet in a shard. When avtgate
receives the initial signal from a new primary tablet it then initializes/loads the keyspace schema. When the primary tablet was restarted, however, we did not clear out any previous schema for that keyspace so we would end up with duplicated column names that grew over time.We also had a very confusing log message which said it loaded N tables in a keyspace when in reality the value was a count of the columns across all the tables.
You can see examples and test cases for these things here and here.
The manual test here now passes and a new e2e test was added to cover this case.
The new e2e test fails on main as expected. Click here for details.
Related Issue(s)
Fixes: #9431
Checklist