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

release-2.1: storage: smarten GC of orphaned replicas of subsumed ranges #31685

Merged
merged 3 commits into from
Oct 22, 2018

Conversation

benesch
Copy link
Contributor

@benesch benesch commented Oct 21, 2018

Backport 3/3 commits from #31570.

/cc @cockroachdb/release


When a range is subsumed, there are two paths by which its replicas can
be cleaned up. The first path is that the subsuming replica, when it
applies the merge trigger, removes the subsumed replica. This is the
common case, as all replicas are collocated when the merge transaction
starts.

The second path is that the subumed replica is later cleaned up by the
replica GC queue. This occurs when the subsuming range is rebalanced
away shortly after the merge and so never applies the merge trigger,
"orphaning" the subsuming replica.

The replica GC queue must be careful to never to GC a replica that could
be subsumed. If it discovers that a merge occurred, it needs to "prove"
that the replica is actually orphaned. It does so by checking whether
the left neighbor's local descriptor matches the meta2 descriptor; if it
does not, the left neighbor is out of date and could possibly still
apply a merge trigger, so the replica cannot be GC'd.

Unfortunately, the replica GC queue tried to be too clever: it assumed
such a proof was not necessary if the store was still a member of the
subsuming range. Concretely, suppose adjacent ranges A and B merge, and
store 2's replica of B is orphaned. When the replica GC queue looks
up B's descriptor in meta2, it will get the descriptor for the combined
range AB instead and correctly infer that a merge occurred. It also
assumed that, because AB is listed as having a replica on store2, that
the merge must be applying soon.

This assumption was wrong. Suppose the merged range AB immediately
splits back into A and B. The replica GC queue, considering store 2's
replica of the new B, will, again, correctly infer that a merge took
place (even though the descriptor it fetches from meta2 will have the
same start and end key as its local descriptor, it will have a new range
ID), but now its assumption that a replica of A must exist on the store
is incorrect! A may have been rebalanced away, in which case we must
GC the old copy of B, or the store will never be able to accept a
snapshot for the new copy of B.

This scenario was observed in several real clusters, and easily
reproduces when restoring TPC-C.

The fix is simple: teach the replica GC queue to always perform the
proof when a range has been merged away. Attempting to be clever just to
save one meta2 lookup was a bad idea.

Touches #31409.

Release note: None

/cc @andreimatei

If a store receives a snapshot that overlaps an existing replica, we
take it as a sign that the local replica may no longer be a member of
its range and queue it for processing in the replica GC queue.

When this code was added (cockroachdb#10426), the replica GC queue was quite
aggressive about processing replicas, and so the implementation was
careful to only queue a replica if it looked "inactive." Unfortunately,
this inactivity check rotted when epoch-based leases were introduced a
month later (cockroachdb#10305). An inactive replica with an epoch-based lease can
incorrectly be considered active, even if all of the other members of
the range have stopped sending it messages, because the epoch-based
lease will continue to be heartbeated by the node itself. (With an
expiration-based lease, the replica's local copy of the lease would
quickly expire if the other members of the range stopped sending it
messages.)

Fixing the inactivity check to work with epoch-based leases is rather
tricky. Quiescent replicas are nearly indistinguishable from
abandoned replicas. This commit just removes the inactivity check and
unconditionally queues replicas for GC if they intersect an incoming
snapshot. The replica GC queue check is relatively cheap (one or two
meta2 lookups), and overlapping snapshot situations are not expected to
last for very long.

Release note: None
When a range is subsumed, there are two paths by which its replicas can
be cleaned up. The first path is that the subsuming replica, when it
applies the merge trigger, removes the subsumed replica. This is the
common case, as all replicas are collocated when the merge transaction
starts.

The second path is that the subumed replica is later cleaned up by the
replica GC queue. This occurs when the subsuming range is rebalanced
away shortly after the merge and so never applies the merge trigger,
"orphaning" the subsuming replica.

The replica GC queue must be careful to never to GC a replica that could
be subsumed. If it discovers that a merge occurred, it needs to "prove"
that the replica is actually orphaned. It does so by checking whether
the left neighbor's local descriptor matches the meta2 descriptor; if it
does not, the left neighbor is out of date and could possibly still
apply a merge trigger, so the replica cannot be GC'd.

Unfortunately, the replica GC queue tried to be too clever: it assumed
such a proof was not necessary if the store was still a member of the
subsuming range. Concretely, suppose adjacent ranges A and B merge, and
store 2's replica of B is orphaned. When the replica GC queue looks up
B's descriptor in meta2, it will get the descriptor for the combined
range AB instead and correctly infer that a merge occurred. It also
assumed that, because AB is listed as having a replica on store2, that
the merge must be applying soon.

This assumption was wrong. Suppose the merged range AB immediately
splits back into A and B. The replica GC queue, considering store 2's
replica of the new B, will, again, correctly infer that a merge took
place (even though the descriptor it fetches from meta2 will have the
same start and end key as its local descriptor, it will have a new range
ID), but now its assumption that a replica of A must exist on the store
is incorrect! A may have been rebalanced away, in which case we *must*
GC the old copy of B, or the store will never be able to accept a
snapshot for the new copy of B.

This scenario was observed in several real clusters, and easily
reproduces when restoring TPC-C.

The fix is simple: teach the replica GC queue to always perform the
proof when a range has been merged away. Attempting to be clever just to
save one meta2 lookup was a bad idea.

Touches cockroachdb#31409.

Release note: None
Convert the log.V calls to log.VEvent calls in the replica GC queue so
that they show up in the debug enqueue page.

Release note: None
@benesch benesch requested review from bdarnell, tbg and a team October 21, 2018 21:52
@cockroach-teamcity
Copy link
Member

This change is Reviewable

@tbg
Copy link
Member

tbg commented Oct 22, 2018 via email

@benesch benesch merged commit 29d1f23 into cockroachdb:release-2.1 Oct 22, 2018
@benesch benesch deleted the backport2.1-31570 branch October 22, 2018 16:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants