The documentation of the "slot_testv_and_readv_and_writev" method in interfaces.py requires clarification. #1744

New Issue

zancas · 2012-05-19T08:10:04Z

zancas commented

2012-05-19 08:10:04 +00:00

There are several ambiguities, or errors, in the old version [ or my understanding thereof ;-) ].

First: It wasn't clear that the read operation occurs regardless of the outcome of the test operation.

Second: It ambiguously states that the write vectors are applied to "those shares" (in fact iiuc the write vector shares may be overlapping, or even disjoint, from the existing shares).

It is correct that the shares which may be written will only be written in the case that a call to:

SOMESHARE.check_testv(testv)

returns "True" for all shares in the test_and_write_vectors, but the SOMESHARE will be EmptyShare if the share to be written is not already on the server.

Here's how I changed it in a local git repo:

# Please enter the commit message for your changes. Lines starting
# with '#' will be ignored, and an empty message aborts the commit.
# On branch master
# Changes to be committed:
#   (use "git reset HEAD <file>..." to unstage)
#
#       modified:   interfaces.py
#
# Changes not staged for commit:
#   (use "git add <file>..." to update what will be committed)
#   (use "git checkout -- <file>..." to discard changes in working directory)
#
#       modified:   mutable/layout.py
#
diff --git a/src/allmydata/interfaces.py b/src/allmydata/interfaces.py
index a73ce86..b15863b 100644
--- a/src/allmydata/interfaces.py
+++ b/src/allmydata/interfaces.py
@@ -159,10 +159,13 @@ class RIStorageServer(RemoteInterface):
                                         tw_vectors=TestAndWriteVectorsForShares,
                                         r_vector=ReadVector,
                                         ):
-        """General-purpose test-and-set operation for mutable slots. Perform
-        a bunch of comparisons against the existing shares. If they all pass,
-        then apply a bunch of write vectors to those shares. Then use the
-        read vectors to extract data from all the shares and return the data.
+        """
+        General-purpose atomic test-read-and-set operation for mutable slots.
+        (1) For submitted shnums compare against extant-and-empty shares. 
+        (2) Use the read vectors to extract "old data" from all extant shares.
+        (3) If all tests in (1) passed, then write tw_vectors shares. 
+        (4) Return "whether-the-tests-passed" and the "old data", which does not 
+        include any modifications made by the writes.

         This method is, um, large. The goal is to allow clients to update all
         the shares associated with a mutable file in a single round trip.
@@ -232,9 +235,11 @@ class RIStorageServer(RemoteInterface):
         than the size of the data after applying all write vectors.

         The read vector is used to extract data from all known shares,
-        *before* any writes have been applied. The same vector is used for
-        all shares. This captures the state that was tested by the test
-        vector.
+        *before* any writes have been applied. The same read vector is used 
+        for all shares. This captures the state that was tested by the test
+        vector, for extant shares. Of course, the extracted data contains no
+        information about shares written to new shares that did not previously
+        exist.

         This method returns two values: a boolean and a dict. The boolean is
         True if the write vectors were applied, False if not. The dict is

There are several ambiguities, or errors, in the old version [ or my understanding thereof ;-) ]. First: It wasn't clear that the read operation occurs regardless of the outcome of the test operation. Second: It ambiguously states that the write vectors are applied to "those shares" (in fact iiuc the write vector shares may be overlapping, or even disjoint, from the existing shares). It _is_ correct that the shares which may be written will only be written in the case that a call to: ``` SOMESHARE.check_testv(testv) ``` returns "True" for all shares in the test_and_write_vectors, but the SOMESHARE will be EmptyShare if the share to be written is not already on the server. Here's how I changed it in a local git repo: ``` # Please enter the commit message for your changes. Lines starting # with '#' will be ignored, and an empty message aborts the commit. # On branch master # Changes to be committed: # (use "git reset HEAD <file>..." to unstage) # # modified: interfaces.py # # Changes not staged for commit: # (use "git add <file>..." to update what will be committed) # (use "git checkout -- <file>..." to discard changes in working directory) # # modified: mutable/layout.py # diff --git a/src/allmydata/interfaces.py b/src/allmydata/interfaces.py index a73ce86..b15863b 100644 --- a/src/allmydata/interfaces.py +++ b/src/allmydata/interfaces.py @@ -159,10 +159,13 @@ class RIStorageServer(RemoteInterface): tw_vectors=TestAndWriteVectorsForShares, r_vector=ReadVector, ): - """General-purpose test-and-set operation for mutable slots. Perform - a bunch of comparisons against the existing shares. If they all pass, - then apply a bunch of write vectors to those shares. Then use the - read vectors to extract data from all the shares and return the data. + """ + General-purpose atomic test-read-and-set operation for mutable slots. + (1) For submitted shnums compare against extant-and-empty shares. + (2) Use the read vectors to extract "old data" from all extant shares. + (3) If all tests in (1) passed, then write tw_vectors shares. + (4) Return "whether-the-tests-passed" and the "old data", which does not + include any modifications made by the writes. This method is, um, large. The goal is to allow clients to update all the shares associated with a mutable file in a single round trip. @@ -232,9 +235,11 @@ class RIStorageServer(RemoteInterface): than the size of the data after applying all write vectors. The read vector is used to extract data from all known shares, - *before* any writes have been applied. The same vector is used for - all shares. This captures the state that was tested by the test - vector. + *before* any writes have been applied. The same read vector is used + for all shares. This captures the state that was tested by the test + vector, for extant shares. Of course, the extracted data contains no + information about shares written to new shares that did not previously + exist. This method returns two values: a boolean and a dict. The boolean is True if the write vectors were applied, False if not. The dict is ```

zancas added the

labels 2012-05-19 08:10:04 +00:00

zancas added this to the undecided milestone 2012-05-19 08:10:04 +00:00

davidsarah commented

2012-05-19 19:03:25 +00:00

+1, but I think the sentence "Of course, the extracted data contains no information about shares written to new shares that did not previously exist." is redundant, since it's implied by "old data" and "extant shares".

Please also apply the following diff:

@@ -204,7 +204,9 @@
 
         The write vector will be applied to the given share, expanding it if
         necessary. A write vector applied to a share number that did not
-        exist previously will cause that share to be created.
+        exist previously will cause that share to be created. Write vectors
+        must not overlap (if they do, this will either cause an error or
+        apply them in an unspecified order).
 
         In Tahoe-LAFS v1.8.3 or later (except 1.9.0a1), if you send a write
         vector whose offset is beyond the end of the current data, the space

+1, but I think the sentence "Of course, the extracted data contains no information about shares written to new shares that did not previously exist." is redundant, since it's implied by "old data" and "extant shares". Please also apply the following diff: ``` @@ -204,7 +204,9 @@ The write vector will be applied to the given share, expanding it if necessary. A write vector applied to a share number that did not - exist previously will cause that share to be created. + exist previously will cause that share to be created. Write vectors + must not overlap (if they do, this will either cause an error or + apply them in an unspecified order). In Tahoe-LAFS v1.8.3 or later (except 1.9.0a1), if you send a write vector whose offset is beyond the end of the current data, the space ```

tahoe-lafs modified the milestone from undecided to 1.9.2

2012-05-19 19:03:25 +00:00

zancas was assigned by tahoe-lafs

2012-05-19 19:03:25 +00:00

david-sarah@jacaranda.org commented

2012-06-13 16:58:13 +00:00

In changeset:daa24bce8b61e25b:

Clarify documentation of RIStorageServer.slot_testv_and_readv_and_writev. fixes #1744

In changeset:daa24bce8b61e25b: ``` Clarify documentation of RIStorageServer.slot_testv_and_readv_and_writev. fixes #1744 ```

tahoe-lafs added the

fixed

label 2012-06-13 16:58:13 +00:00

tahoe-lafs closed this issue

2012-06-13 16:58:13 +00:00

david-sarah@jacaranda.org commented

2012-06-13 17:06:21 +00:00

In changeset:5509/1.9.2:

Clarify documentation of RIStorageServer.slot_testv_and_readv_and_writev. fixes #1744

In changeset:5509/1.9.2: ``` Clarify documentation of RIStorageServer.slot_testv_and_readv_and_writev. fixes #1744 ```

david-sarah <david-sarah@jacaranda.org> commented

2012-06-14 06:18:16 +00:00

In changeset:daa24bce8b61e25b:

Clarify documentation of RIStorageServer.slot_testv_and_readv_and_writev. fixes #1744

In changeset:daa24bce8b61e25b: ``` Clarify documentation of RIStorageServer.slot_testv_and_readv_and_writev. fixes #1744 ```

david-sarah@jacaranda.org commented

2012-07-10 20:04:57 +00:00

In changeset:5858/cloud-backend:

Clarify documentation of RIStorageServer.slot_testv_and_readv_and_writev. fixes #1744

In changeset:5858/cloud-backend: ``` Clarify documentation of RIStorageServer.slot_testv_and_readv_and_writev. fixes #1744 ```

Sign in to join this conversation.

No Label

LeastAuthority.com automation

code-frontend-ftp-sftp

code-frontend-magic-folder

somebody else's problem

No Milestone

No Assignees

2 Participants

Notifications

Due Date

The due date is invalid or out of range. Please use the format 'yyyy-mm-dd'.

No due date set.

Reference: tahoe-lafs/trac-2024-07-25#1744