From b6cfd9a36430d608ea5d49e9f787caedcd1649b7 Mon Sep 17 00:00:00 2001 From: Ethan Buchman Date: Wed, 13 Dec 2017 01:17:20 -0500 Subject: [PATCH] wip: tests and fixes for kvstore iteration --- glide.lock | 2 +- store/cachekvstore_test.go | 174 ++++++++++++++++++++++++++++++------ store/cachemergeiterator.go | 22 ++++- store/memiterator.go | 10 ++- 4 files changed, 179 insertions(+), 29 deletions(-) diff --git a/glide.lock b/glide.lock index 314de2e58360..a80574674802 100644 --- a/glide.lock +++ b/glide.lock @@ -174,7 +174,7 @@ imports: - types - version - name: github.com/tendermint/tmlibs - version: 5636a02d035258701974da39c62d13c1d76f8ae8 + version: 318982c0babe627c7dda57e23a1eae2bf0d2c1bf subpackages: - autofile - cli diff --git a/store/cachekvstore_test.go b/store/cachekvstore_test.go index 7b55e55a1382..4c0e796e8bda 100644 --- a/store/cachekvstore_test.go +++ b/store/cachekvstore_test.go @@ -3,63 +3,187 @@ package store import ( "testing" + "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" + cmn "github.com/tendermint/tmlibs/common" dbm "github.com/tendermint/tmlibs/db" ) +func keyFmt(i int) []byte { return bz(cmn.Fmt("key%d", i)) } +func valFmt(i int) []byte { return bz(cmn.Fmt("value%d", i)) } + func TestCacheKVStore(t *testing.T) { mem := dbm.NewMemDB() st := NewCacheKVStore(mem) - require.Empty(t, st.Get(bz("key1")), "Expected `key1` to be empty") + require.Empty(t, st.Get(keyFmt(1)), "Expected `key1` to be empty") - mem.Set(bz("key1"), bz("value1")) - st.Set(bz("key1"), bz("value1")) - require.Equal(t, bz("value1"), st.Get(bz("key1"))) + // put something in mem and in cache + mem.Set(keyFmt(1), valFmt(1)) + st.Set(keyFmt(1), valFmt(1)) + require.Equal(t, valFmt(1), st.Get(keyFmt(1))) - st.Set(bz("key1"), bz("value2")) - require.Equal(t, bz("value2"), st.Get(bz("key1"))) - require.Equal(t, bz("value1"), mem.Get(bz("key1"))) + // update it in cache, shoudn't change mem + st.Set(keyFmt(1), valFmt(2)) + require.Equal(t, valFmt(2), st.Get(keyFmt(1))) + require.Equal(t, valFmt(1), mem.Get(keyFmt(1))) + // write it. should change mem st.Write() - require.Equal(t, bz("value2"), mem.Get(bz("key1"))) + require.Equal(t, valFmt(2), mem.Get(keyFmt(1))) + require.Equal(t, valFmt(2), st.Get(keyFmt(1))) + // more writes and checks st.Write() st.Write() - require.Equal(t, bz("value2"), mem.Get(bz("key1"))) + require.Equal(t, valFmt(2), mem.Get(keyFmt(1))) + require.Equal(t, valFmt(2), st.Get(keyFmt(1))) + + // make a new one, check it + st = NewCacheKVStore(mem) + require.Equal(t, valFmt(2), st.Get(keyFmt(1))) + // make a new one and delete - should not be removed from mem st = NewCacheKVStore(mem) - st.Delete(bz("key1")) - require.Empty(t, st.Get(bz("key1"))) - require.Equal(t, mem.Get(bz("key1")), bz("value2")) + st.Delete(keyFmt(1)) + require.Empty(t, st.Get(keyFmt(1))) + require.Equal(t, mem.Get(keyFmt(1)), valFmt(2)) + // Write. should now be removed from both st.Write() - require.Empty(t, st.Get(bz("key1")), "Expected `key1` to be empty") - require.Empty(t, mem.Get(bz("key1")), "Expected `key1` to be empty") + require.Empty(t, st.Get(keyFmt(1)), "Expected `key1` to be empty") + require.Empty(t, mem.Get(keyFmt(1)), "Expected `key1` to be empty") } func TestCacheKVStoreNested(t *testing.T) { mem := dbm.NewMemDB() st := NewCacheKVStore(mem) - st.Set(bz("key1"), bz("value1")) - require.Empty(t, mem.Get(bz("key1"))) - require.Equal(t, bz("value1"), st.Get(bz("key1"))) + // set. check its there on st and not on mem. + st.Set(keyFmt(1), valFmt(1)) + require.Empty(t, mem.Get(keyFmt(1))) + require.Equal(t, valFmt(1), st.Get(keyFmt(1))) + + // make a new from st and check st2 := NewCacheKVStore(st) - require.Equal(t, bz("value1"), st2.Get(bz("key1"))) + require.Equal(t, valFmt(1), st2.Get(keyFmt(1))) - st2.Set(bz("key1"), bz("VALUE2")) - require.Equal(t, []byte(nil), mem.Get(bz("key1"))) - require.Equal(t, bz("value1"), st.Get(bz("key1"))) - require.Equal(t, bz("VALUE2"), st2.Get(bz("key1"))) + // update the value on st2, check it only effects st2 + st2.Set(keyFmt(1), valFmt(3)) + require.Equal(t, []byte(nil), mem.Get(keyFmt(1))) + require.Equal(t, valFmt(1), st.Get(keyFmt(1))) + require.Equal(t, valFmt(3), st2.Get(keyFmt(1))) + // st2 writes to its parent, st. doesnt effect mem st2.Write() - require.Equal(t, []byte(nil), mem.Get(bz("key1"))) - require.Equal(t, bz("VALUE2"), st.Get(bz("key1"))) + require.Equal(t, []byte(nil), mem.Get(keyFmt(1))) + require.Equal(t, valFmt(3), st.Get(keyFmt(1))) + // updates mem st.Write() - require.Equal(t, bz("VALUE2"), mem.Get(bz("key1"))) + require.Equal(t, valFmt(3), mem.Get(keyFmt(1))) +} + +func TestCacheKVIteratorBounds(t *testing.T) { + mem := dbm.NewMemDB() + st := NewCacheKVStore(mem) + + // set some items + nItems := 5 + for i := 0; i < nItems; i++ { + st.Set(keyFmt(i), valFmt(i)) + } + + // iterate over all of them + itr := st.Iterator(dbm.BeginningKey(), dbm.EndingKey()) + var i = 0 + for ; itr.Valid(); itr.Next() { + k, v := itr.Key(), itr.Value() + assert.Equal(t, keyFmt(i), k) + assert.Equal(t, valFmt(i), v) + i += 1 + } + assert.Equal(t, nItems, i) + + // iterate over none + itr = st.Iterator(bz("money"), dbm.EndingKey()) + i = 0 + for ; itr.Valid(); itr.Next() { + i += 1 + } + assert.Equal(t, 0, i) + return + + // iterate over lower + itr = st.Iterator(keyFmt(0), keyFmt(3)) + i = 0 + for ; itr.Valid(); itr.Next() { + k, v := itr.Key(), itr.Value() + assert.Equal(t, keyFmt(i), k) + assert.Equal(t, valFmt(i), v) + i += 1 + } + assert.Equal(t, 2, i) + + // iterate over upper + itr = st.Iterator(keyFmt(2), keyFmt(4)) + i = 2 + for ; itr.Valid(); itr.Next() { + k, v := itr.Key(), itr.Value() + assert.Equal(t, keyFmt(i), k) + assert.Equal(t, valFmt(i), v) + i += 1 + } + assert.Equal(t, 4, i) +} + +func TestCacheKVMergeIterator(t *testing.T) { + mem := dbm.NewMemDB() + st := NewCacheKVStore(mem) + + // set some items and write them + nItems := 5 + for i := 0; i < nItems; i++ { + st.Set(keyFmt(i), valFmt(i)) + } + st.Write() + + // set some items and leave dirty + for i := nItems; i < nItems*2; i++ { + st.Set(keyFmt(i), valFmt(i)) + } + + // iterate over all of them + assertIterateDomain(t, st, nItems*2) + + // set the last dirty item to delete + last := nItems*2 - 1 + st.Delete(keyFmt(last)) + + // iterate over all of them, ensure we dont see the last one + assertIterateDomain(t, st, nItems*2-1) + + // write and check again + st.Write() + assertIterateDomain(t, st, nItems*2-1) + + // delete the next last one + last = nItems*2 - 2 + st.Delete(keyFmt(last)) + assertIterateDomain(t, st, nItems*2-2) +} +// iterate over whole domain +func assertIterateDomain(t *testing.T, st KVStore, expectedN int) { + itr := st.Iterator(dbm.BeginningKey(), dbm.EndingKey()) + var i = 0 + for ; itr.Valid(); itr.Next() { + k, v := itr.Key(), itr.Value() + assert.Equal(t, keyFmt(i), k) + assert.Equal(t, valFmt(i), v) + i += 1 + } + assert.Equal(t, expectedN, i) } func bz(s string) []byte { return []byte(s) } diff --git a/store/cachemergeiterator.go b/store/cachemergeiterator.go index 68f63a9b5303..fc5259f6f74a 100644 --- a/store/cachemergeiterator.go +++ b/store/cachemergeiterator.go @@ -1,6 +1,8 @@ package store -import "bytes" +import ( + "bytes" +) // cacheMergeIterator merges a parent Iterator and a cache Iterator. // The cache iterator may return nil keys to signal that an item @@ -169,6 +171,9 @@ func (iter *cacheMergeIterator) compare(a, b []byte) int { // If `until` is nil, there is no limit. // CONTRACT: cache is valid. func (iter *cacheMergeIterator) skipCacheDeletes(until []byte) { + if !iter.cache.Valid() { + return + } for (until == nil || iter.compare(iter.cache.Key(), until) < 0) && iter.cache.Value() == nil { @@ -189,8 +194,21 @@ func (iter *cacheMergeIterator) skipUntilExistsOrInvalid() { return } + // If cache not valid but parent is, return + if iter.parent.Valid() && + !iter.cache.Valid() { + return + } + // Parent and Cache items exist. - keyP, keyC := iter.parent.Key(), iter.cache.Key() + var keyP, keyC []byte + if iter.parent.Valid() { + keyP = iter.parent.Key() + } + if iter.cache.Valid() { + keyC = iter.cache.Key() + } + cmp := iter.compare(keyP, keyC) switch cmp { diff --git a/store/memiterator.go b/store/memiterator.go index ab5d9b621b01..933e268a0059 100644 --- a/store/memiterator.go +++ b/store/memiterator.go @@ -1,5 +1,7 @@ package store +import dbm "github.com/tendermint/tmlibs/db" + // Iterates over iterKVCache items. // if key is nil, means it was deleted. // Implements Iterator. @@ -9,10 +11,16 @@ type memIterator struct { } func newMemIterator(start, end []byte, items []KVPair) *memIterator { + itemsInDomain := make([]KVPair, 0) + for _, item := range items { + if dbm.IsKeyInDomain(string(item.Key), start, end) { + itemsInDomain = append(itemsInDomain, item) + } + } return &memIterator{ start: start, end: end, - items: items, + items: itemsInDomain, } }