From 4afe3eb224d6f26ac768aaccbca68c69035d57d4 Mon Sep 17 00:00:00 2001 From: Itamar Turner-Trauring Date: Wed, 11 May 2022 10:58:13 -0400 Subject: [PATCH 1/5] Clarify sets vs lists some more. --- docs/proposed/http-storage-node-protocol.rst | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/docs/proposed/http-storage-node-protocol.rst b/docs/proposed/http-storage-node-protocol.rst index 693ce9290..7e0b4a542 100644 --- a/docs/proposed/http-storage-node-protocol.rst +++ b/docs/proposed/http-storage-node-protocol.rst @@ -350,8 +350,10 @@ Because of the simple types used throughout and the equivalence described in `RFC 7049`_ these examples should be representative regardless of which of these two encodings is chosen. +The one exception is sets. For CBOR messages, any sequence that is semantically a set (i.e. no repeated values allowed, order doesn't matter, and elements are hashable in Python) should be sent as a set. Tag 6.258 is used to indicate sets in CBOR; see `the CBOR registry `_ for more details. +Sets will be represented as JSON lists in examples because JSON doesn't support sets. HTTP Design ~~~~~~~~~~~ @@ -739,7 +741,7 @@ Reading !!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!! Retrieve a set indicating all shares available for the indicated storage index. -For example:: +For example (this is shown as list, since it will be list for JSON, but will be set for CBOR):: [1, 5] From 07e16b80b5df22a5620d390cb34032a55e62e795 Mon Sep 17 00:00:00 2001 From: Itamar Turner-Trauring Date: Wed, 11 May 2022 11:00:05 -0400 Subject: [PATCH 2/5] Better name. --- src/allmydata/storage/http_server.py | 4 ++-- src/allmydata/storage/server.py | 2 +- src/allmydata/test/test_storage.py | 12 ++++++------ 3 files changed, 9 insertions(+), 9 deletions(-) diff --git a/src/allmydata/storage/http_server.py b/src/allmydata/storage/http_server.py index 748790a72..96e906e43 100644 --- a/src/allmydata/storage/http_server.py +++ b/src/allmydata/storage/http_server.py @@ -658,10 +658,10 @@ class HTTPServer(object): "/v1/mutable//shares", methods=["GET"], ) - def list_mutable_shares(self, request, authorization, storage_index): + def enumerate_mutable_shares(self, request, authorization, storage_index): """List mutable shares for a storage index.""" try: - shares = self._storage_server.list_mutable_shares(storage_index) + shares = self._storage_server.enumerate_mutable_shares(storage_index) except KeyError: raise _HTTPError(http.NOT_FOUND) return self._send_encoded(request, shares) diff --git a/src/allmydata/storage/server.py b/src/allmydata/storage/server.py index b46303cd8..ab7947bf9 100644 --- a/src/allmydata/storage/server.py +++ b/src/allmydata/storage/server.py @@ -690,7 +690,7 @@ class StorageServer(service.MultiService): self) return share - def list_mutable_shares(self, storage_index) -> set[int]: + def enumerate_mutable_shares(self, storage_index) -> set[int]: """List all share numbers for the given mutable. Raises ``KeyError`` if the storage index is not known. diff --git a/src/allmydata/test/test_storage.py b/src/allmydata/test/test_storage.py index 8f1ece401..9bc218fa6 100644 --- a/src/allmydata/test/test_storage.py +++ b/src/allmydata/test/test_storage.py @@ -1315,26 +1315,26 @@ class MutableServer(unittest.TestCase): self.failUnless(isinstance(readv_data, dict)) self.failUnlessEqual(len(readv_data), 0) - def test_list_mutable_shares(self): + def test_enumerate_mutable_shares(self): """ - ``StorageServer.list_mutable_shares()`` returns a set of share numbers + ``StorageServer.enumerate_mutable_shares()`` returns a set of share numbers for the given storage index, or raises ``KeyError`` if it does not exist at all. """ - ss = self.create("test_list_mutable_shares") + ss = self.create("test_enumerate_mutable_shares") # Initially, nothing exists: with self.assertRaises(KeyError): - ss.list_mutable_shares(b"si1") + ss.enumerate_mutable_shares(b"si1") self.allocate(ss, b"si1", b"we1", b"le1", [0, 1, 4, 2], 12) - shares0_1_2_4 = ss.list_mutable_shares(b"si1") + shares0_1_2_4 = ss.enumerate_mutable_shares(b"si1") # Remove share 2, by setting size to 0: secrets = (self.write_enabler(b"we1"), self.renew_secret(b"le1"), self.cancel_secret(b"le1")) ss.slot_testv_and_readv_and_writev(b"si1", secrets, {2: ([], [], 0)}, []) - shares0_1_4 = ss.list_mutable_shares(b"si1") + shares0_1_4 = ss.enumerate_mutable_shares(b"si1") self.assertEqual( (shares0_1_2_4, shares0_1_4), ({0, 1, 2, 4}, {0, 1, 4}) From 6d412a017c34fc6f6528c89b1443d9bdf7caee64 Mon Sep 17 00:00:00 2001 From: Itamar Turner-Trauring Date: Wed, 11 May 2022 11:00:46 -0400 Subject: [PATCH 3/5] Type annotation. --- src/allmydata/storage/server.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/allmydata/storage/server.py b/src/allmydata/storage/server.py index ab7947bf9..f1b835780 100644 --- a/src/allmydata/storage/server.py +++ b/src/allmydata/storage/server.py @@ -690,7 +690,7 @@ class StorageServer(service.MultiService): self) return share - def enumerate_mutable_shares(self, storage_index) -> set[int]: + def enumerate_mutable_shares(self, storage_index: bytes) -> set[int]: """List all share numbers for the given mutable. Raises ``KeyError`` if the storage index is not known. From 4b62ec082bed7ecc33612741dfbffc16868ca3ff Mon Sep 17 00:00:00 2001 From: Itamar Turner-Trauring Date: Wed, 11 May 2022 11:11:24 -0400 Subject: [PATCH 4/5] Match Foolscap behavior for slot_readv of unknown storage index. --- src/allmydata/storage_client.py | 8 +++++++- src/allmydata/test/test_istorageserver.py | 16 ++++++++++++++++ 2 files changed, 23 insertions(+), 1 deletion(-) diff --git a/src/allmydata/storage_client.py b/src/allmydata/storage_client.py index cd489a307..83a1233f5 100644 --- a/src/allmydata/storage_client.py +++ b/src/allmydata/storage_client.py @@ -1200,7 +1200,13 @@ class _HTTPStorageServer(object): # If shares list is empty, that means list all shares, so we need # to do a query to get that. if not shares: - shares = yield mutable_client.list_shares(storage_index) + try: + shares = yield mutable_client.list_shares(storage_index) + except ClientException as e: + if e.code == http.NOT_FOUND: + shares = set() + else: + raise # Start all the queries in parallel: for share_number in shares: diff --git a/src/allmydata/test/test_istorageserver.py b/src/allmydata/test/test_istorageserver.py index a3e75bbac..66535ddda 100644 --- a/src/allmydata/test/test_istorageserver.py +++ b/src/allmydata/test/test_istorageserver.py @@ -854,6 +854,22 @@ class IStorageServerMutableAPIsTestsMixin(object): {0: [b"abcdefg"], 1: [b"0123456"], 2: [b"9876543"]}, ) + @inlineCallbacks + def test_slot_readv_unknown_storage_index(self): + """ + With unknown storage index, ``IStorageServer.slot_readv()`` TODO. + """ + storage_index = new_storage_index() + reads = yield self.storage_client.slot_readv( + storage_index, + shares=[], + readv=[(0, 7)], + ) + self.assertEqual( + reads, + {}, + ) + @inlineCallbacks def create_slot(self): """Create a slot with sharenum 0.""" From 457db8f992c62e8f5c5bc4cdcb6e99f097062f08 Mon Sep 17 00:00:00 2001 From: Itamar Turner-Trauring Date: Wed, 11 May 2022 11:17:57 -0400 Subject: [PATCH 5/5] Get rid of the "no such storage index" edge case, since it's not really necessary. --- src/allmydata/storage/http_server.py | 5 +---- src/allmydata/storage/server.py | 7 ++----- src/allmydata/storage_client.py | 8 +------- src/allmydata/test/test_storage.py | 12 ++++++------ 4 files changed, 10 insertions(+), 22 deletions(-) diff --git a/src/allmydata/storage/http_server.py b/src/allmydata/storage/http_server.py index 96e906e43..a1641f563 100644 --- a/src/allmydata/storage/http_server.py +++ b/src/allmydata/storage/http_server.py @@ -660,10 +660,7 @@ class HTTPServer(object): ) def enumerate_mutable_shares(self, request, authorization, storage_index): """List mutable shares for a storage index.""" - try: - shares = self._storage_server.enumerate_mutable_shares(storage_index) - except KeyError: - raise _HTTPError(http.NOT_FOUND) + shares = self._storage_server.enumerate_mutable_shares(storage_index) return self._send_encoded(request, shares) diff --git a/src/allmydata/storage/server.py b/src/allmydata/storage/server.py index f1b835780..bcf44dc30 100644 --- a/src/allmydata/storage/server.py +++ b/src/allmydata/storage/server.py @@ -691,15 +691,12 @@ class StorageServer(service.MultiService): return share def enumerate_mutable_shares(self, storage_index: bytes) -> set[int]: - """List all share numbers for the given mutable. - - Raises ``KeyError`` if the storage index is not known. - """ + """Return all share numbers for the given mutable.""" si_dir = storage_index_to_dir(storage_index) # shares exist if there is a file for them bucketdir = os.path.join(self.sharedir, si_dir) if not os.path.isdir(bucketdir): - raise KeyError("Not found") + return set() result = set() for sharenum_s in os.listdir(bucketdir): try: diff --git a/src/allmydata/storage_client.py b/src/allmydata/storage_client.py index 83a1233f5..cd489a307 100644 --- a/src/allmydata/storage_client.py +++ b/src/allmydata/storage_client.py @@ -1200,13 +1200,7 @@ class _HTTPStorageServer(object): # If shares list is empty, that means list all shares, so we need # to do a query to get that. if not shares: - try: - shares = yield mutable_client.list_shares(storage_index) - except ClientException as e: - if e.code == http.NOT_FOUND: - shares = set() - else: - raise + shares = yield mutable_client.list_shares(storage_index) # Start all the queries in parallel: for share_number in shares: diff --git a/src/allmydata/test/test_storage.py b/src/allmydata/test/test_storage.py index 9bc218fa6..65d09de25 100644 --- a/src/allmydata/test/test_storage.py +++ b/src/allmydata/test/test_storage.py @@ -1317,14 +1317,14 @@ class MutableServer(unittest.TestCase): def test_enumerate_mutable_shares(self): """ - ``StorageServer.enumerate_mutable_shares()`` returns a set of share numbers - for the given storage index, or raises ``KeyError`` if it does not exist at all. + ``StorageServer.enumerate_mutable_shares()`` returns a set of share + numbers for the given storage index, or an empty set if it does not + exist at all. """ ss = self.create("test_enumerate_mutable_shares") # Initially, nothing exists: - with self.assertRaises(KeyError): - ss.enumerate_mutable_shares(b"si1") + empty = ss.enumerate_mutable_shares(b"si1") self.allocate(ss, b"si1", b"we1", b"le1", [0, 1, 4, 2], 12) shares0_1_2_4 = ss.enumerate_mutable_shares(b"si1") @@ -1336,8 +1336,8 @@ class MutableServer(unittest.TestCase): ss.slot_testv_and_readv_and_writev(b"si1", secrets, {2: ([], [], 0)}, []) shares0_1_4 = ss.enumerate_mutable_shares(b"si1") self.assertEqual( - (shares0_1_2_4, shares0_1_4), - ({0, 1, 2, 4}, {0, 1, 4}) + (empty, shares0_1_2_4, shares0_1_4), + (set(), {0, 1, 2, 4}, {0, 1, 4}) ) def test_bad_magic(self):