Arvados

Just a couple of cosmetic comments:

• File sdk/go/arvados/fs_base.go
  • Lines 84, 85: "It is an error to replace a directory with a file" -- may require correction.
  • Line 174: Repeated 'the' word on comment

Other than that, it LGTM.

18600-copy-by-ref @ ca0b6ff3d75f68b7a2f1821d605fadd49481038e

(just updates comments)

LGTM, thanks!

developer-run-tests-apps-workbench-integration: #3146

developer-run-tests-apps-workbench-integration: #3148

• Rather than a separate "modify" API, I added a virtual attribute "splices" that can be passed in a create or update request in place of manifest_text. This makes it possible to atomically create a new collection using data from existing collections.
• Deleting a subtree (even the root) and then inserting new content below that point is supported, as an exception to the "cannot have a destination that's within another destination" rule proposed above. This makes it possible to atomically clear and rebuild an existing collection from scratch, rather than requiring the client to list every existing file/subdir to delete, which would be inconvenient and prone to unexpected behavior in races.

The attribute name "splices" alludes to the js/ruby/perl array splice operation ("delete some items and insert some new ones in their place") but perhaps there is a better name for it?

Some comments:

• Regarding the name "splices", I find it confusing because the param is about a series of operations that should be done to the data the collection holds (as opposed to the rest of the params, which usually are about data or metadata).
  • As commented on chat: one idea would be to name it something like "manifest_ops" making it explicit that its contents are a list of operations on the collection's data.
  • Another idea would be converting the noun-based name "splices" to "splicings" making it more evident that its contents are actions, not data. Even then, it doesn't have the "data context" in its name, but I think it may be better than using a noun.
• Do we need to update the discovery document to list this new param?
• It seems that removing non-existent targets is currently a no-op; not sure if that's on purpose.
• I think I've found one case that is not a correct behavior:
  • {"/": "SOME-VALID-PDH", "/": ""} → produces an empty collection
  • {"/foo": "SOME-VALID-PDH", "/": ""} → produces a collection with /foo containing SOME-VALID-PDH's contents.

Lucas Di Pentima wrote:

• I think I've found one case that is not a correct behavior:
  • {"/": "SOME-VALID-PDH", "/": ""} → produces an empty collection
  • {"/foo": "SOME-VALID-PDH", "/": ""} → produces a collection with /foo containing SOME-VALID-PDH's contents.

I see now that in the 1st case, I used the same target with different sources. Perhaps we should avoid accepting the same target multiple times or document that the last operation is the one that would be the final result.
If accepting the same target multiple times is OK, it would be useful to avoid doing extra work for the [0..N-1] cases of the same target.

Agh, please forget about my last comment. My tests were being done with the PySDK, so obviously this happens:

>>> {"hello":"world", "hello":"people"}
{'hello': 'people'}

Sorry about that!

• "replace" is a verb, and not a noun
• hints that existing content at specified paths will be deleted (as opposed to "add" or "update" which might imply some kind of merge)

"collection": {
  "replace_files": {
    "/foo.txt": "",
    "/bar.txt": "fa7aeb5140e2848d39b416daeef4ffc5+45/foo.txt",
    "/subdir_to_delete": "" 
  }
}

Re discovery doc, I think this would have to be added as a "Collection" property, which incorrectly implies that it's part of a collection response too. This might be a hint that we should expose it in the API as an update option beside the "new collection properties" object instead of inside it, more like this

PATCH /arvados/v1/collections/{uuid}

{
  "collection": {
    "name": "new_name" 
  },
  "replace_files": {
    "/foo.txt": "",
    "/bar.txt": "fa7aeb5140e2848d39b416daeef4ffc5+45/foo.txt",
    "/subdir_to_delete": "" 
  }
}

Thoughts?

Tom Clegg wrote:

Re discovery doc, I think this would have to be added as a "Collection" property, which incorrectly implies that it's part of a collection response too. This might be a hint that we should expose it in the API as an update option beside the "new collection properties" object instead of inside it, more like this

[...]

Thoughts?

I agree with this, it is an update parameter but not part of the collection object.

I think replace_files is a lot better than splices!
Re: note-23, I see what you mean, is it possible to do that with the current Google API Client support? (not remembering of any existing similar case) If so, I think it's a good idea to split request data that's about the object from actions being done to it.

developer-run-tests: #2957

• instead of a write-only collection attribute called "splices", we now have a create/update parameter called "replace_files"
• add replace_files param to discovery doc
• test replace_files from sdk/cli tests (this relies on the param being in the discovery doc)

Accepting "delete nonexistent path" as a no-op is intentional. Analogous to "rm -rf", it avoids forcing the client to spend extra effort on "check whether X exists before deleting it", and makes it possible to do things like "replace entire contents of dir/ with dir/file.txt" atomically. I think a "can't delete, doesn't exist" error would essentially be a race-detection feature for a certain type of race, which will be better addressed (along with a much wider range of races) by implementing If-Matches support.

This LGTM, thanks!

merge main → 18600-update-files @ f7bf9d69603db2d500563648460e2a96524de266 --

18600-update-files @ b8fb328cc2f084d155add478264ffaf384eda190 -- developer-run-tests: #2965