mvcc: fix TestHashKVWhenCompacting hash mismatch #8333

fanminshi · 2017-07-28T22:12:27Z

fanminshi · 2017-07-28T22:13:42Z

this pr allows HashKV to get compaction Keep without modifying the treeIndex.

heyitsanthony

nothing calls Keep() outside of tests; please update hashkv in this patchset if that's why this is being introduced

heyitsanthony · 2017-07-28T22:19:14Z

mvcc/index.go

+// Keep finds all revisions to be kept for a Compaction at the given rev.
+func (ti *treeIndex) Keep(rev int64) map[revision]struct{} {
+	available := make(map[revision]struct{})
+	ti.Lock()


why isn't this RLock?

heyitsanthony · 2017-07-28T22:24:40Z

mvcc/key_index.go

 		// remove any tombstone
-		if len(g.revs) == 1 && i != len(ki.generations)-1 {
+		if len(g.revs)-1 == n && i != len(ki.generations)-1 {


swapping the order of the if's doesn't look like it preserves the meaning; if n > 0 then delete(available, g.revs[0]) is not the same as the current code

yeah, the code should be delete(available, g.revs[n]) instead of delete(available, g.revs[0]).

why wasn't this caught by unit tests?

The unit tests calls Compact before Keep where Compaction on tombstone clears out the generation. Then the subsequent Keep on the same tombstone rev will not even hit if len(g.revs)-1 == n && i != len(ki.generations)-1 { since if !g.isEmpty() { will be empty.

e.g

// Init Tree. // key: "foo" // rev: 16 // generations: // {empty} // {{14, 0}[1], {14, 1}[2], {16, 0}(t)[3]} // {{8, 0}[1], {10, 0}[2], {12, 0}(t)[3]} // {{2, 0}[1], {4, 0}[2], {6, 0}(t)[3]} // Call Compact(12) keep = {} // generations: // {empty} // {{14, 0}[1], {14, 1}[2], {16, 0}(t)[3]} // Call Keep(12) keep = {} since there is no generation that contains rev 12.

I'll make unit test more strict.

heyitsanthony · 2017-07-28T22:28:53Z

mvcc/key_index.go

+	ki.doCompact(atRev, available, false)
+}
+
+func (ki *keyIndex) doCompact(atRev int64, available map[revision]struct{}, doModify bool) {


it's not good practice to have boolean arguments; possibly have
func (ki *keyIndex) doCompact(atRev int64, available map[revision]struct{}) (genIdx int, revIndex int) and handle the modification after calling doCompact

heyitsanthony · 2017-07-28T22:29:10Z

mvcc/kvstore_test.go

@@ -767,6 +767,10 @@ func (i *fakeIndex) Compact(rev int64) map[revision]struct{} {
 	i.Recorder.Record(testutil.Action{Name: "compact", Params: []interface{}{rev}})
 	return <-i.indexCompactRespc
 }
+func (i *fakeIndex) Keep(rev int64) map[revision]struct{} {


this should be in the same commit as the test update

heyitsanthony · 2017-07-31T20:42:31Z

mvcc/kvstore.go

-	}
-	keep := s.keep
-	s.keepMu.Unlock()
+	keep := s.kvindex.Keep(rev)


only need to acquire s.mu.RLock instead of s.mu.Lock now?

Keep finds all revisions to be kept for a Compaction at the given rev.

fanminshi · 2017-07-31T21:06:58Z

all fixed. PTAL.

heyitsanthony

lgtm

heyitsanthony · 2017-07-31T21:30:11Z

mvcc/key_index_test.go

+}
+
+func cloneGeneration(g *generation) *generation {
+	var tmp []revision


use go built-ins:

tmp := make([]revision, len(g.revs)) copy(tmp, g.revs)

If g.revs is nil, the above code will create a non nil tmp which causes comparison failures.

--- FAIL: TestKeyIndexCompactAndKeep (0.00s) key_index_test.go:451: #0: ki = g: created[{2 0}] ver[3], revs []mvcc.revision{mvcc.revision{main:2, sub:0}, mvcc.revision{main:4, sub:0}, mvcc.revision{main:6, sub:0}} g: created[{8 0}] ver[3], revs []mvcc.revision{mvcc.revision{main:8, sub:0}, mvcc.revision{main:10, sub:0}, mvcc.revision{main:12, sub:0}} g: created[{14 0}] ver[3], revs []mvcc.revision{mvcc.revision{main:14, sub:0}, mvcc.revision{main:14, sub:1}, mvcc.revision{main:16, sub:0}} g: created[{0 0}] ver[0], revs []mvcc.revision(nil) , want g: created[{2 0}] ver[3], revs []mvcc.revision{mvcc.revision{main:2, sub:0}, mvcc.revision{main:4, sub:0}, mvcc.revision{main:6, sub:0}} g: created[{8 0}] ver[3], revs []mvcc.revision{mvcc.revision{main:8, sub:0}, mvcc.revision{main:10, sub:0}, mvcc.revision{main:12, sub:0}} g: created[{14 0}] ver[3], revs []mvcc.revision{mvcc.revision{main:14, sub:0}, mvcc.revision{main:14, sub:1}, mvcc.revision{main:16, sub:0}} g: created[{0 0}] ver[0], revs []mvcc.revision{}

OK:

if len(g.revs) == nil { // preserve nils return nil } tmp := make([]revision, len(g.revs)) copy(tmp, g.revs) return tmp

the return type is *generation

I have the following

func cloneGeneration(g *generation) *generation { if g.revs == nil { return &generation{g.ver, g.created, nil} } tmp := make([]revision, len(g.revs)) copy(tmp, g.revs) return &generation{g.ver, g.created, tmp} }

fanminshi · 2017-08-01T16:54:04Z

via https://jenkins-etcd-public.prod.coreos.systems/job/etcd-ci-ppc64/1743/console

2017-08-01 03:15:30.669672 I | etcdserver/api/v3rpc: get error from resetTransport context canceled, transportMonitor returning
2017-08-01 03:15:30.669691 I | etcdserver/api/v3rpc: grpc: addrConn.transportMonitor exits due to: context canceled
Too many goroutines running after all test(s).
1 instances of:
google.golang.org/grpc.(*ClientConn).lbWatcher(...)
	/var/jenkins_home/workspace/etcd-ci-ppc64/gopath/src/google.golang.org/grpc/clientconn.go:499 +0x78
created by google.golang.org/grpc.DialContext.func3
	/var/jenkins_home/workspace/etcd-ci-ppc64/gopath/src/google.golang.org/grpc/clientconn.go:409 +0x194
1 instances of:
google.golang.org/grpc.(*addrConn).resetTransport(...)
	/var/jenkins_home/workspace/etcd-ci-ppc64/gopath/src/google.golang.org/grpc/clientconn.go:903 +0x754
google.golang.org/grpc.(*addrConn).transportMonitor(...)
	/var/jenkins_home/workspace/etcd-ci-ppc64/gopath/src/google.golang.org/grpc/clientconn.go:999 +0x460
google.golang.org/grpc.(*ClientConn).resetAddrConn.func1(...)
	/var/jenkins_home/workspace/etcd-ci-ppc64/gopath/src/google.golang.org/grpc/clientconn.go:640 +0x1bc
created by google.golang.org/grpc.(*ClientConn).resetAddrConn
	/var/jenkins_home/workspace/etcd-ci-ppc64/gopath/src/google.golang.org/grpc/clientconn.go:641 +0x5dc
exit status 1
FAIL	github.com/coreos/etcd/clientv3/integration	230.527s

Leaky test. I don't think it is related to this pr.

heyitsanthony · 2017-08-01T16:54:54Z

that's a grpc leak, fine to ignore

fanminshi · 2017-08-01T16:55:11Z

proxy-ci fails via #6934.
I don't believe this is cause by this pr.

heyitsanthony suggested changes Jul 28, 2017

View reviewed changes

fanminshi force-pushed the retrieve_keep_from_index branch from e0592ea to 9eaa470 Compare July 28, 2017 23:50

heyitsanthony reviewed Jul 31, 2017

View reviewed changes

fanminshi force-pushed the retrieve_keep_from_index branch from 9eaa470 to c7817cd Compare July 31, 2017 20:59

fanminshi mentioned this pull request Jul 31, 2017

mvcc: fix TestHashKVWhenCompacting hash mismatch #8314

Closed

fanminshi changed the title ~~mvcc: add Keep api to index~~ mvcc: fix TestHashKVWhenCompacting hash mismatch Jul 31, 2017

mvcc: add and implement Keep api to index

7b8fb3c

Keep finds all revisions to be kept for a Compaction at the given rev.

fanminshi force-pushed the retrieve_keep_from_index branch from c7817cd to f0f886b Compare July 31, 2017 21:04

fanminshi force-pushed the retrieve_keep_from_index branch from f0f886b to 65b6ce5 Compare July 31, 2017 21:26

heyitsanthony approved these changes Jul 31, 2017

View reviewed changes

fanminshi force-pushed the retrieve_keep_from_index branch from 65b6ce5 to bd321b8 Compare July 31, 2017 21:40

fanminshi added 3 commits July 31, 2017 17:59

mvcc: add tests for Keep

4c2c5b0

mvcc: HashKV gets keep from kvindex.Keep

bb86c32

mvcc: increase rev for TestHashKVWhenCompacting

df5a3d1

fanminshi force-pushed the retrieve_keep_from_index branch from bd321b8 to df5a3d1 Compare August 1, 2017 01:00

heyitsanthony merged commit 585b1d7 into etcd-io:master Aug 1, 2017

fuweid mentioned this pull request Jul 16, 2024

HashKV should compute the hash of all MVCC keys up to a given revision #18337

Open

4 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

mvcc: fix TestHashKVWhenCompacting hash mismatch #8333

mvcc: fix TestHashKVWhenCompacting hash mismatch #8333

fanminshi commented Jul 28, 2017 •

edited

Loading

fanminshi commented Jul 28, 2017

heyitsanthony left a comment

heyitsanthony Jul 28, 2017

heyitsanthony Jul 28, 2017

fanminshi Jul 28, 2017

heyitsanthony Jul 28, 2017

fanminshi Jul 28, 2017 •

edited

Loading

heyitsanthony Jul 28, 2017

heyitsanthony Jul 28, 2017

heyitsanthony Jul 31, 2017

fanminshi commented Jul 31, 2017

heyitsanthony left a comment

heyitsanthony Jul 31, 2017

fanminshi Jul 31, 2017

heyitsanthony Jul 31, 2017

fanminshi Aug 1, 2017

heyitsanthony Aug 1, 2017

fanminshi commented Aug 1, 2017

heyitsanthony commented Aug 1, 2017

fanminshi commented Aug 1, 2017

mvcc: fix TestHashKVWhenCompacting hash mismatch #8333

mvcc: fix TestHashKVWhenCompacting hash mismatch #8333

Conversation

fanminshi commented Jul 28, 2017 • edited Loading

fanminshi commented Jul 28, 2017

heyitsanthony left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fanminshi Jul 28, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fanminshi commented Jul 31, 2017

heyitsanthony left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fanminshi commented Aug 1, 2017

heyitsanthony commented Aug 1, 2017

fanminshi commented Aug 1, 2017

fanminshi commented Jul 28, 2017 •

edited

Loading

fanminshi Jul 28, 2017 •

edited

Loading