Skip to content
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: Don't allow preemptive snapshots on initialized replicas #14261

Closed
wants to merge 1 commit into from

Conversation

a-robinson
Copy link
Contributor

@a-robinson a-robinson commented Mar 20, 2017

This prevents preemptive snapshots from silently succeeding, allowing a
replica GC to be intertwined with the creation of a new replica such
that the new replica has no local data.

Also add some verbose logging for replica ID changes.


This PR is honestly more questions than changes. The only real change is moving the r.mu.replicaID > replicaID check ahead of the replicaID == 0 check, and beyond that it's just an extra log statement and questions. Even that change requires some more investigation as it's caused the test to start occasionally timing out due to a failure to ever GC the removed replica (with not gc'able messages in the logs), which may or may not be an existing issue that rejecting the preemptive snapshots exposes.

I think we also need to do more than this since it's essentially just preventing preemptive snapshots from being applied to existing replicas, but I'm a little wary of making bigger modifications without fully understanding the reasoning/history behind why things are the way they are. In particular, the fact that a MsgHeartbeat can seemingly come in and create a replica that doesn't know its own key range which will then fail when it tries to process the heartbeat (as in #14231 (comment)) seems super dangerous, but there may be some reason for doing so that I haven't thought of.

Once the questions are resolved, this will:
Fix #14193
Fix #14231
Fix #12574

@cockroachdb/stability


This change is Reviewable

This prevents preemptive snapshots from silently succeeding, allowing a
replica GC to be intertwined with the creation of a new replica such
that the new replica has no local data.

Also add some verbose logging for replica ID changes.
@tamird tamird requested review from bdarnell, rjnn and petermattis March 20, 2017 14:39
@bdarnell
Copy link
Contributor

Historical note: we disallowed preemptive snapshots when there was an existing replica until #8613. There was debate about whether or not we should allow that in #7833 (I was of the opinion that it would be fine; @tschottdorf was more cautious).

@a-robinson
Copy link
Contributor Author

Consider this replaced by #14306.

@a-robinson a-robinson closed this Mar 22, 2017
@a-robinson a-robinson deleted the debug branch May 18, 2018 20:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants