From 1246c52d04de221f30435eeb5d5c21d8cd926934 Mon Sep 17 00:00:00 2001 From: Marek Siarkowicz Date: Mon, 31 May 2021 17:25:57 +0200 Subject: [PATCH] etcdserver: Fix invalid count returned on Range with Limit (cherry picked from commit 182aef6e6b28d2f62cd7281f38c12899d8006a15) Signed-off-by: Wei Fu --- integration/v3_grpc_test.go | 31 ++++++++++++++++++++++++------- mvcc/index.go | 23 ++++++++++------------- mvcc/kv_test.go | 19 ++++++++++--------- mvcc/kvstore_test.go | 9 ++++++--- mvcc/kvstore_txn.go | 8 ++++---- 5 files changed, 54 insertions(+), 36 deletions(-) diff --git a/integration/v3_grpc_test.go b/integration/v3_grpc_test.go index 331f2bc60..ce5dc379c 100644 --- a/integration/v3_grpc_test.go +++ b/integration/v3_grpc_test.go @@ -1285,8 +1285,9 @@ func TestV3RangeRequest(t *testing.T) { putKeys []string reqs []pb.RangeRequest - wresps [][]string - wmores []bool + wresps [][]string + wmores []bool + wcounts []int64 }{ // single key { @@ -1303,6 +1304,7 @@ func TestV3RangeRequest(t *testing.T) { {}, }, []bool{false, false}, + []int64{1, 0}, }, // multi-key { @@ -1331,6 +1333,7 @@ func TestV3RangeRequest(t *testing.T) { {"a", "b", "c", "d", "e"}, }, []bool{false, false, false, false, false, false}, + []int64{5, 2, 0, 0, 0, 5}, }, // revision { @@ -1349,22 +1352,30 @@ func TestV3RangeRequest(t *testing.T) { {"a", "b"}, }, []bool{false, false, false, false}, + []int64{5, 0, 1, 2}, }, // limit { - []string{"foo", "bar"}, + []string{"a", "b", "c"}, []pb.RangeRequest{ // more {Key: []byte("a"), RangeEnd: []byte("z"), Limit: 1}, - // no more + // half {Key: []byte("a"), RangeEnd: []byte("z"), Limit: 2}, + // no more + {Key: []byte("a"), RangeEnd: []byte("z"), Limit: 3}, + // limit over + {Key: []byte("a"), RangeEnd: []byte("z"), Limit: 4}, }, [][]string{ - {"bar"}, - {"bar", "foo"}, + {"a"}, + {"a", "b"}, + {"a", "b", "c"}, + {"a", "b", "c"}, }, - []bool{true, false}, + []bool{true, true, false, false}, + []int64{3, 3, 3, 3}, }, // sort { @@ -1417,6 +1428,7 @@ func TestV3RangeRequest(t *testing.T) { {"b", "a", "c", "d"}, }, []bool{true, true, true, true, false, false}, + []int64{4, 4, 4, 4, 0, 4}, }, // min/max mod rev { @@ -1448,6 +1460,7 @@ func TestV3RangeRequest(t *testing.T) { {"rev2", "rev3", "rev4", "rev5", "rev6"}, }, []bool{false, false, false, false}, + []int64{5, 5, 5, 5}, }, // min/max create rev { @@ -1479,6 +1492,7 @@ func TestV3RangeRequest(t *testing.T) { {"rev2", "rev3", "rev6"}, }, []bool{false, false, false, false}, + []int64{3, 3, 3, 3}, }, } @@ -1512,6 +1526,9 @@ func TestV3RangeRequest(t *testing.T) { if resp.More != tt.wmores[j] { t.Errorf("#%d.%d: bad more. got = %v, want = %v, ", i, j, resp.More, tt.wmores[j]) } + if resp.GetCount() != tt.wcounts[j] { + t.Errorf("#%d.%d: bad count. got = %v, want = %v, ", i, j, resp.GetCount(), tt.wcounts[j]) + } wrev := int64(len(tt.putKeys) + 1) if resp.Header.Revision != wrev { t.Errorf("#%d.%d: bad header revision. got = %d. want = %d", i, j, resp.Header.Revision, wrev) diff --git a/mvcc/index.go b/mvcc/index.go index 5de0fdf9e..6caabd60d 100644 --- a/mvcc/index.go +++ b/mvcc/index.go @@ -25,8 +25,8 @@ import ( type index interface { Get(key []byte, atRev int64) (rev, created revision, ver int64, err error) Range(key, end []byte, atRev int64) ([][]byte, []revision) - Revisions(key, end []byte, atRev int64, limit int) []revision - CountRevisions(key, end []byte, atRev int64, limit int) int + Revisions(key, end []byte, atRev int64, limit int) ([]revision, int) + CountRevisions(key, end []byte, atRev int64) int Put(key []byte, rev revision) Tombstone(key []byte, rev revision) error RangeSince(key, end []byte, rev int64) []revision @@ -106,27 +106,27 @@ func (ti *treeIndex) visit(key, end []byte, f func(ki *keyIndex) bool) { }) } -func (ti *treeIndex) Revisions(key, end []byte, atRev int64, limit int) (revs []revision) { +func (ti *treeIndex) Revisions(key, end []byte, atRev int64, limit int) (revs []revision, total int) { if end == nil { rev, _, _, err := ti.Get(key, atRev) if err != nil { - return nil + return nil, 0 } - return []revision{rev} + return []revision{rev}, 1 } ti.visit(key, end, func(ki *keyIndex) bool { if rev, _, _, err := ki.get(ti.lg, atRev); err == nil { - revs = append(revs, rev) - if len(revs) == limit { - return false + if limit <= 0 || len(revs) < limit { + revs = append(revs, rev) } + total++ } return true }) - return revs + return revs, total } -func (ti *treeIndex) CountRevisions(key, end []byte, atRev int64, limit int) int { +func (ti *treeIndex) CountRevisions(key, end []byte, atRev int64) int { if end == nil { _, _, _, err := ti.Get(key, atRev) if err != nil { @@ -138,9 +138,6 @@ func (ti *treeIndex) CountRevisions(key, end []byte, atRev int64, limit int) int ti.visit(key, end, func(ki *keyIndex) bool { if _, _, _, err := ki.get(ti.lg, atRev); err == nil { total++ - if total == limit { - return false - } } return true }) diff --git a/mvcc/kv_test.go b/mvcc/kv_test.go index a652a45d7..adb71c113 100644 --- a/mvcc/kv_test.go +++ b/mvcc/kv_test.go @@ -219,17 +219,18 @@ func testKVRangeLimit(t *testing.T, f rangeFunc) { wrev := int64(4) tests := []struct { - limit int64 - wkvs []mvccpb.KeyValue + limit int64 + wcounts int64 + wkvs []mvccpb.KeyValue }{ // no limit - {-1, kvs}, + {-1, 3, kvs}, // no limit - {0, kvs}, - {1, kvs[:1]}, - {2, kvs[:2]}, - {3, kvs}, - {100, kvs}, + {0, 3, kvs}, + {1, 3, kvs[:1]}, + {2, 3, kvs[:2]}, + {3, 3, kvs}, + {100, 3, kvs}, } for i, tt := range tests { r, err := f(s, []byte("foo"), []byte("foo3"), RangeOptions{Limit: tt.limit}) @@ -246,7 +247,7 @@ func testKVRangeLimit(t *testing.T, f rangeFunc) { if r.Count != len(kvs) { t.Errorf("#%d: count = %d, want %d", i, r.Count, len(kvs)) } - } else if r.Count != int(tt.limit) { + } else if r.Count != int(tt.wcounts) { t.Errorf("#%d: count = %d, want %d", i, r.Count, tt.limit) } } diff --git a/mvcc/kvstore_test.go b/mvcc/kvstore_test.go index 3e8ae89bf..672878fa6 100644 --- a/mvcc/kvstore_test.go +++ b/mvcc/kvstore_test.go @@ -936,12 +936,15 @@ type fakeIndex struct { indexCompactRespc chan map[revision]struct{} } -func (i *fakeIndex) Revisions(key, end []byte, atRev int64, limit int) []revision { +func (i *fakeIndex) Revisions(key, end []byte, atRev int64, limit int) ([]revision, int) { _, rev := i.Range(key, end, atRev) - return rev + if len(rev) >= limit { + rev = rev[:limit] + } + return rev, len(rev) } -func (i *fakeIndex) CountRevisions(key, end []byte, atRev int64, limit int) int { +func (i *fakeIndex) CountRevisions(key, end []byte, atRev int64) int { _, rev := i.Range(key, end, atRev) return len(rev) } diff --git a/mvcc/kvstore_txn.go b/mvcc/kvstore_txn.go index 5c2c7eb77..c7b1f7c54 100644 --- a/mvcc/kvstore_txn.go +++ b/mvcc/kvstore_txn.go @@ -126,14 +126,14 @@ func (tr *storeTxnRead) rangeKeys(key, end []byte, curRev int64, ro RangeOptions return &RangeResult{KVs: nil, Count: -1, Rev: 0}, ErrCompacted } if ro.Count { - total := tr.s.kvindex.CountRevisions(key, end, rev, int(ro.Limit)) + total := tr.s.kvindex.CountRevisions(key, end, rev) tr.trace.Step("count revisions from in-memory index tree") return &RangeResult{KVs: nil, Count: total, Rev: curRev}, nil } - revpairs := tr.s.kvindex.Revisions(key, end, rev, int(ro.Limit)) + revpairs, total := tr.s.kvindex.Revisions(key, end, rev, int(ro.Limit)) tr.trace.Step("range keys from in-memory index tree") if len(revpairs) == 0 { - return &RangeResult{KVs: nil, Count: 0, Rev: curRev}, nil + return &RangeResult{KVs: nil, Count: total, Rev: curRev}, nil } limit := int(ro.Limit) @@ -176,7 +176,7 @@ func (tr *storeTxnRead) rangeKeys(key, end []byte, curRev int64, ro RangeOptions } } tr.trace.Step("range keys from bolt db") - return &RangeResult{KVs: kvs, Count: len(revpairs), Rev: curRev}, nil + return &RangeResult{KVs: kvs, Count: total, Rev: curRev}, nil } func (tw *storeTxnWrite) put(key, value []byte, leaseID lease.LeaseID) {