-
Notifications
You must be signed in to change notification settings - Fork 2k
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
Relax singleton-context constraint for pending non-singleton-context reverts #10114
Relax singleton-context constraint for pending non-singleton-context reverts #10114
Conversation
Signed-off-by: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com>
Signed-off-by: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com>
Signed-off-by: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com>
Call for review |
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.
A couple of nits, but otherwise LGTM. We should at least change the single dashes to double for the new flag everywhere. Let me know what you think.
go/test/endtoend/onlineddl/singleton/onlineddl_singleton_test.go
Outdated
Show resolved
Hide resolved
go/vt/vttablet/onlineddl/executor.go
Outdated
revertedUUID, _ := pendingOnlineDDL.GetRevertUUID() | ||
if revertedUUID == "" { |
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 feels like an opportunity for undefined behavior in the future. Any reason not to do this instead?
if _, err := pendingOnlineDDL.GetRevertUUID(); errors.Is(err, vtrpcpb.Code_INVALID_ARGUMENT) {
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.
changed to if _, err := pendingOnlineDDL.GetRevertUUID(); err != nil
; I prefer not to rely on a specific error code.
Signed-off-by: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com>
Signed-off-by: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com>
Signed-off-by: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com>
Signed-off-by: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com>
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.
Thanks!
no no, thank you |
Description
When a migration runs with -singleton-context ddl strategy flag, and with context
A
, then another migration with-singleton-context
can only be submitted if it has same contextA
, and cannot be submitted if it has contextB
. Note, the submission itself returns with an error.We want to have better user experience when it comes to reverts. Reverts may be long running and are allowed to run concurrently to other migrations.
A REVERT can run without a
-singleton-context
flag. In fact, this is the canonical way of running it. So we do not fail submitting a REVERT even if there are pending migrations.However, what happens when a REVERT is already running, and a new migration comes in?
Starting this PR, the following holds true:
-singleton-context
-singleton-context
and with some arbitrarymigration_context
.To clarify:
-singleton-context
flag with some-migration_context=A
-singleton-context
flag may only be submitted it it also has-migration_context=A
, and it will be rejected if it has-migration_context=B
The reason we add this relaxation is that we see an operational pattern in usage, and this behavior makes sense operationally.
Documentation to follow.
Related Issue(s)
-singleton-context
flag #7946Checklist