Skip to content

Commit

Permalink
mvcc: fix hash mismatch
Browse files Browse the repository at this point in the history
This pr adds a check in compaction to ensure that setting keep to nil only if this is the latest compaction in queue and
merges the keep from prev ongoing compaction to this scheduled compaction allows HashKV to computes Hash from previous keep.

Scenario #1

Suppose that HashKV() starts after compact(A) and compact(B) has scheduled and after
completion of compact(A) but before the completion of compact(B).

Before this PR:
Completion of compact(A) sets keep to nil to indicate that there are no ongoing compactions.
This assumption causes HashKV to hash all mvcc keys including keys that
hasn't been deleted by compact(B) up to a rev. Hence, HashKV again after compact(B) returns
a different Hash that during compact(B).

After this PR:
Completion of compact(A) sets keep to nil if current compactRev == compactRev(compact(A)).
Since calling compact(B) changes current compactRev, then current compactRev != compactRev(compact(A)).
Then HashKV will base on the keep set during compact(B) instead of nil which return the correct hash
during compact(B).

Scenario etcd-io#2

Suppose that HashKV() starts after compact(A) and compact(B) has scheduled and before the
completion of compact(A) and compact(B).

Before this PR:
The start of compact(B) sets keep to compact(B) and overrides the keep from compact(A).
Since compact(A) hasn't finished yet, HashKV needs to hash revision from keep of compact(A) but it
is overridden. Hence HashKV misses revision from keep of compact(A) would result hash difference.

After this PR:
The start of compact(B) merges keep from compact(B) and compact(A). Even though compact(A) hasn't finished,
HashKV can retrieve from keep of compact(A). Hence HashKV doesn't miss any revisions and will compute Hash correctly.
  • Loading branch information
fanminshi committed Jul 27, 2017
1 parent 2a348fb commit 60be89f
Showing 1 changed file with 17 additions and 4 deletions.
21 changes: 17 additions & 4 deletions mvcc/kvstore.go
Original file line number Diff line number Diff line change
Expand Up @@ -258,7 +258,13 @@ func (s *store) Compact(rev int64) (<-chan struct{}, error) {

keep := s.kvindex.Compact(rev)
s.keepMu.Lock()
s.keep = keep
// merge keep from the previous ongoing compaction
// allows HashKV to include revisions from previous keep.
if len(s.keep) > 0 {
for k, v := range keep {
s.keep[k] = v
}
}
s.keepMu.Unlock()
ch := make(chan struct{})
var j = func(ctx context.Context) {
Expand All @@ -271,9 +277,16 @@ func (s *store) Compact(rev int64) (<-chan struct{}, error) {
return
}
close(ch)
s.keepMu.Lock()
s.keep = nil
s.keepMu.Unlock()
s.revMu.RLock()
// set the keep to nil if this is the last compaction
// to indicate there aren't any ongoing compactions.
// correctness of HashKV bases on state of keep.
if rev == s.compactMainRev {
s.keepMu.Lock()
s.keep = nil
s.keepMu.Unlock()
}
s.revMu.RUnlock()
}

s.fifoSched.Schedule(j)
Expand Down

0 comments on commit 60be89f

Please sign in to comment.