Skip to content

Commit

Permalink
kv: remove obsolete TODO in maybeAcquireSnapshotMergeLock
Browse files Browse the repository at this point in the history
The TODO discussed a merge followed by a split and alluded to an
optimization that could be used to avoid a snapshot. This doesn't make
sense to me. I think that's because the comment was written back in a
time where a range merged into and then back out of its left hand
neighbor would be the same range with the same ID. This is not the case
anymore, so I don't see how we could ever (reasonably) avoid a snapshot
in such a case.
  • Loading branch information
nvanbenschoten committed Oct 14, 2020
1 parent d8a4d5f commit ed3010d
Showing 1 changed file with 0 additions and 11 deletions.
11 changes: 0 additions & 11 deletions pkg/kv/kvserver/replica_raft.go
Original file line number Diff line number Diff line change
Expand Up @@ -1567,17 +1567,6 @@ func (r *Replica) maybeAcquireSnapshotMergeLock(
subsumedRepls = append(subsumedRepls, sRepl)
endKey = sRepl.Desc().EndKey
}
// TODO(benesch): we may be unnecessarily forcing another Raft snapshot here
// by subsuming too much. Consider the case where [a, b) and [c, e) first
// merged into [a, e), then split into [a, d) and [d, e), and we're applying a
// snapshot that spans this merge and split. The bounds of this snapshot will
// be [a, d), so we'll subsume [c, e). But we're still a member of [d, e)!
// We'll currently be forced to get a Raft snapshot to catch up. Ideally, we'd
// subsume only half of [c, e) and synthesize a new RHS [d, e), effectively
// applying both the split and merge during snapshot application. This isn't a
// huge deal, though: we're probably behind enough that the RHS would need to
// get caught up with a Raft snapshot anyway, even if we synthesized it
// properly.
return subsumedRepls, func() {
for _, sr := range subsumedRepls {
sr.raftMu.Unlock()
Expand Down

0 comments on commit ed3010d

Please sign in to comment.