From ed3010d439f7807554992e37d99c0fae8e942225 Mon Sep 17 00:00:00 2001 From: Nathan VanBenschoten Date: Tue, 13 Oct 2020 00:38:02 -0400 Subject: [PATCH] kv: remove obsolete TODO in maybeAcquireSnapshotMergeLock 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. --- pkg/kv/kvserver/replica_raft.go | 11 ----------- 1 file changed, 11 deletions(-) diff --git a/pkg/kv/kvserver/replica_raft.go b/pkg/kv/kvserver/replica_raft.go index 696ecb003df9..b374840e3482 100644 --- a/pkg/kv/kvserver/replica_raft.go +++ b/pkg/kv/kvserver/replica_raft.go @@ -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()