-
Notifications
You must be signed in to change notification settings - Fork 3.8k
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
storage: fix replica removal when learners not active #39697
Conversation
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.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @danhhz and @tbg)
pkg/server/version_cluster_test.go, line 403 at r1 (raw file):
k := tc.ScratchRange(t) tc.AddReplicasOrFatal(t, k, tc.Target(2)) log.Infof(ctx, "TBG====")
intentional?
pkg/storage/replica_command.go, line 981 at r1 (raw file):
target := chgs[0].Target return r.addReplicaLegacyPreemptiveSnapshot(ctx, target, desc, priority, reason, details) }
this !useLearners
branch works now by falling through to the learners codepath, but happening to not do any of the learner stuff because there aren't any replica additions. this is pretty subtle. i'd much prefer to have something more direct here (like an else that calls finalizeChangeReplicas
, though the name of that is a little confusing in this context.)
90ea1cd
to
2a4b5f6
Compare
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.
PTAL (ignore the first commit, it's #39690)
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @danhhz)
pkg/server/version_cluster_test.go, line 403 at r1 (raw file):
Previously, danhhz (Daniel Harrison) wrote…
intentional?
🙈 just checking that you're paying attention
Removed.
pkg/storage/replica_command.go, line 981 at r1 (raw file):
Previously, danhhz (Daniel Harrison) wrote…
this
!useLearners
branch works now by falling through to the learners codepath, but happening to not do any of the learner stuff because there aren't any replica additions. this is pretty subtle. i'd much prefer to have something more direct here (like an else that callsfinalizeChangeReplicas
, though the name of that is a little confusing in this context.)
Done.
2a4b5f6
to
5b60303
Compare
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.
Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @danhhz)
pkg/storage/replica_command.go, line 981 at r1 (raw file):
Previously, tbg (Tobias Grieger) wrote…
Done.
much better thank you
I introduced a bug in cockroachdb#39640 which would fail removal of replicas whenever preemptive snapshots were used, since we'd accidentally send a preemptive snapshot which would end up with a "node already in descriptor" error. See: cockroachdb#39640 (review) Release note: None
5b60303
to
492daa4
Compare
TFTR! Fixed the lint error so this should be green now bors r=danhhz |
39697: storage: fix replica removal when learners not active r=danhhz a=tbg I introduced a bug in #39640 which would fail removal of replicas whenever preemptive snapshots were used, since we'd accidentally send a preemptive snapshot which would end up with a "node already in descriptor" error. See: #39640 (review) Release note: None Co-authored-by: Tobias Schottdorf <[email protected]>
Build succeeded |
I introduced a bug in #39640 which would fail removal of replicas
whenever preemptive snapshots were used, since we'd accidentally
send a preemptive snapshot which would end up with a "node already
in descriptor" error.
See:
#39640 (review)
Release note: None