From b53f9896d3e3e901681b8f85dbef77998096ec6b Mon Sep 17 00:00:00 2001 From: Bill Thorp Date: Wed, 13 Feb 2019 12:27:03 -0500 Subject: [PATCH] Removed ReverseList from KeyValueStore interfaces (#1306) Removed ReverseList from KeyValueStore interfaces --- pkg/kademlia/routing_helpers.go | 20 +++++++++++--------- pkg/storj/node.go | 5 ++++- storage/boltdb/client.go | 7 ------- storage/common.go | 2 -- storage/postgreskv/client.go | 6 ------ storage/redis/client.go | 6 ------ storage/storelogger/logger.go | 7 ------- storage/teststore/store.go | 12 ------------ storage/testsuite/test.go | 9 --------- storage/testsuite/test_list.go | 24 +++++------------------- 10 files changed, 20 insertions(+), 78 deletions(-) diff --git a/pkg/kademlia/routing_helpers.go b/pkg/kademlia/routing_helpers.go index 47824f28c..cb8378ea8 100644 --- a/pkg/kademlia/routing_helpers.go +++ b/pkg/kademlia/routing_helpers.go @@ -299,18 +299,20 @@ func (rt *RoutingTable) getUnmarshaledNodesFromBucket(bID bucketID) ([]*pb.Node, // getKBucketRange: helper, returns the left and right endpoints of the range of node ids contained within the bucket func (rt *RoutingTable) getKBucketRange(bID bucketID) ([]bucketID, error) { - kadIDs, err := rt.kadBucketDB.ReverseList(bID[:], 2) + kadBucketIDs, err := rt.kadBucketDB.List(nil, 0) if err != nil { - return nil, RoutingErr.New("could not reverse list k bucket ids %s", err) + return nil, RoutingErr.New("could not list all k bucket ids: %s", err) } - coords := make([]bucketID, 2) - if len(kadIDs) < 2 { - coords[0] = bucketID{} - } else { - copy(coords[0][:], kadIDs[1]) + previousBucket := bucketID{} + for _, k := range kadBucketIDs { + thisBucket := keyToBucketID(k) + if thisBucket == bID { + return []bucketID{previousBucket, bID}, nil + } + previousBucket = thisBucket } - copy(coords[1][:], kadIDs[0]) - return coords, nil + // shouldn't happen BUT return error if no matching kbucket... + return nil, RoutingErr.New("could not find k bucket") } // determineLeafDepth determines the level of the bucket id in question. diff --git a/pkg/storj/node.go b/pkg/storj/node.go index a6c755c36..d8521fe24 100644 --- a/pkg/storj/node.go +++ b/pkg/storj/node.go @@ -21,8 +21,11 @@ var ( ErrNodeID = errs.Class("node ID error") ) +//NodeIDSize is the byte length of a NodeID +const NodeIDSize = sha256.Size + // NodeID is a unique node identifier -type NodeID [sha256.Size]byte +type NodeID [NodeIDSize]byte // NodeIDList is a slice of NodeIDs (implements sort) type NodeIDList []NodeID diff --git a/storage/boltdb/client.go b/storage/boltdb/client.go index 5f2c9e896..9d2b77845 100644 --- a/storage/boltdb/client.go +++ b/storage/boltdb/client.go @@ -155,13 +155,6 @@ func (client *Client) List(first storage.Key, limit int) (storage.Keys, error) { return rv, Error.Wrap(err) } -// ReverseList returns either a list of keys for which boltdb has values or an error. -// Starts from first and iterates backwards -func (client *Client) ReverseList(first storage.Key, limit int) (storage.Keys, error) { - rv, err := storage.ReverseListKeys(client, first, limit) - return rv, Error.Wrap(err) -} - // Close closes a BoltDB client func (client *Client) Close() error { if atomic.AddInt32(client.referenceCount, -1) == 0 { diff --git a/storage/common.go b/storage/common.go index ee6fea18d..16215997a 100644 --- a/storage/common.go +++ b/storage/common.go @@ -62,8 +62,6 @@ type KeyValueStore interface { Delete(Key) error // List lists all keys starting from start and upto limit items List(start Key, limit int) (Keys, error) - // ReverseList lists all keys in revers order - ReverseList(Key, int) (Keys, error) // Iterate iterates over items based on opts Iterate(opts IterateOptions, fn func(Iterator) error) error // Close closes the store diff --git a/storage/postgreskv/client.go b/storage/postgreskv/client.go index 66e2f058f..058b8107a 100644 --- a/storage/postgreskv/client.go +++ b/storage/postgreskv/client.go @@ -115,12 +115,6 @@ func (client *Client) List(first storage.Key, limit int) (storage.Keys, error) { return storage.ListKeys(client, first, limit) } -// ReverseList returns either a list of known keys, in reverse order, or an error. -// Starts from first and iterates backwards -func (client *Client) ReverseList(first storage.Key, limit int) (storage.Keys, error) { - return storage.ReverseListKeys(client, first, limit) -} - // Close closes the client func (client *Client) Close() error { return client.pgConn.Close() diff --git a/storage/redis/client.go b/storage/redis/client.go index 17281dfe2..306da7d1f 100644 --- a/storage/redis/client.go +++ b/storage/redis/client.go @@ -104,12 +104,6 @@ func (client *Client) List(first storage.Key, limit int) (storage.Keys, error) { return storage.ListKeys(client, first, limit) } -// ReverseList returns either a list of keys for which redis has values or an error. -// Starts from first and iterates backwards -func (client *Client) ReverseList(first storage.Key, limit int) (storage.Keys, error) { - return storage.ReverseListKeys(client, first, limit) -} - // Delete deletes a key/value pair from redis, for a given the key func (client *Client) Delete(key storage.Key) error { if key.IsZero() { diff --git a/storage/storelogger/logger.go b/storage/storelogger/logger.go index 7b1f987ac..2e5f889ce 100644 --- a/storage/storelogger/logger.go +++ b/storage/storelogger/logger.go @@ -58,13 +58,6 @@ func (store *Logger) List(first storage.Key, limit int) (storage.Keys, error) { return keys, err } -// ReverseList lists all keys in reverse order, starting from first -func (store *Logger) ReverseList(first storage.Key, limit int) (storage.Keys, error) { - keys, err := store.store.ReverseList(first, limit) - store.log.Debug("ReverseList", zap.String("first", string(first)), zap.Int("limit", limit), zap.Any("keys", keys.Strings())) - return keys, err -} - // Iterate iterates over items based on opts func (store *Logger) Iterate(opts storage.IterateOptions, fn func(storage.Iterator) error) error { store.log.Debug("Iterate", diff --git a/storage/teststore/store.go b/storage/teststore/store.go index 8c81125e3..3c723e13c 100644 --- a/storage/teststore/store.go +++ b/storage/teststore/store.go @@ -178,18 +178,6 @@ func (store *Client) List(first storage.Key, limit int) (storage.Keys, error) { return storage.ListKeys(store, first, limit) } -// ReverseList lists all keys in revers order -func (store *Client) ReverseList(first storage.Key, limit int) (storage.Keys, error) { - store.mu.Lock() - store.CallCount.ReverseList++ - if store.forcedError() { - store.mu.Unlock() - return nil, errors.New("internal error") - } - store.mu.Unlock() - return storage.ReverseListKeys(store, first, limit) -} - // Close closes the store func (store *Client) Close() error { defer store.locked()() diff --git a/storage/testsuite/test.go b/storage/testsuite/test.go index 247800142..38eb639dd 100644 --- a/storage/testsuite/test.go +++ b/storage/testsuite/test.go @@ -70,18 +70,9 @@ func testConstraints(t *testing.T, store storage.KeyValueStore) { if err != nil || len(keys) != storage.LookupLimit { t.Fatalf("List LookupLimit should succeed: %v / got %d", err, len(keys)) } - keys, err = store.ReverseList(nil, storage.LookupLimit) - if err != nil || len(keys) != storage.LookupLimit { - t.Fatalf("ReverseList LookupLimit should succeed: %v / got %d", err, len(keys)) - } - _, err = store.List(nil, storage.LookupLimit+1) if err != nil || len(keys) != storage.LookupLimit { t.Fatalf("List LookupLimit+1 shouldn't fail: %v / got %d", err, len(keys)) } - _, err = store.ReverseList(nil, storage.LookupLimit+1) - if err != nil || len(keys) != storage.LookupLimit { - t.Fatalf("ReverseList LookupLimit+1 shouldn't fail: %v / got %d", err, len(keys)) - } }) } diff --git a/storage/testsuite/test_list.go b/storage/testsuite/test_list.go index ed1eae361..8ff786a34 100644 --- a/storage/testsuite/test_list.go +++ b/storage/testsuite/test_list.go @@ -31,7 +31,6 @@ func testList(t *testing.T, store storage.KeyValueStore) { type Test struct { Name string - Reverse bool First storage.Key Limit int Expected storage.Keys @@ -46,37 +45,24 @@ func testList(t *testing.T, store storage.KeyValueStore) { } tests := []Test{ - {"without key", false, + {"without key", nil, 3, newKeys("path/0", "path/1", "path/2")}, - {"without key, limit 0", false, + {"without key, limit 0", nil, 0, newKeys("path/0", "path/1", "path/2", "path/3", "path/4", "path/5")}, - {"with key", false, + {"with key", storage.Key("path/2"), 3, newKeys("path/2", "path/3", "path/4")}, - {"without key 100", false, + {"without key 100", nil, 100, newKeys("path/0", "path/1", "path/2", "path/3", "path/4", "path/5")}, - {"reverse without key", true, - nil, 3, - newKeys("path/5", "path/4", "path/3")}, - {"reverse with key", true, - storage.Key("path/2"), 3, - newKeys("path/2", "path/1", "path/0")}, - {"reverse without key 100", true, - nil, 100, - newKeys("path/5", "path/4", "path/3", "path/2", "path/1", "path/0")}, } for _, test := range tests { var keys storage.Keys var err error - if !test.Reverse { - keys, err = store.List(test.First, test.Limit) - } else { - keys, err = store.ReverseList(test.First, test.Limit) - } + keys, err = store.List(test.First, test.Limit) if err != nil { t.Errorf("%s: %s", test.Name, err) continue