Arvados

21701-replace-files-current-version @ b08b7b575fb8abf3848e2315d903f752b3f65924 -- developer-run-tests: #4317

With documentation update:

21701-replace-files-current-version @ 5bdbbe819f834bc3bed46899e1eff33f5c08ec0c -- developer-run-tests: #4318

• All agreed upon points are implemented / addressed.
  • ✅
• Anything not implemented (discovered or discussed during work) has a follow-up story.
  • See below
• Code is tested and passing, both automated and manual, what manual testing was done is described
  • ✅ automated tests for atomic rename/copy/upload
• Documentation has been updated.
  • ✅ updated, examples added
• Behaves appropriately at the intended scale (describe intended scale).
  • Expected to do everything previous version could do, in equal or less time/memory when the atomic operations are used.
• Considered backwards and forwards compatibility issues between client and server.
  • Currently no clients make use of the new features.
  • API revision is incremented to 20240627 so future clients can check whether the new features are supported.
• Follows our coding standards and GUI style guidelines.
  • ✅

TBD: Previously, it was an error to supply both non-empty replace_files and non-empty manifest_text in a request. Now, replace_files can reference the supplied manifest_text so it's not always an error. Should we make it an error if both are non-empty and replace_files does not actually refer to manifest_text? The manifest_text value is not used in such cases, so it's likely a mistake, OTOH it's now clear (and could be stated explicitly in the docs) what the expected outcome should be. I'm leaning "error".

Tom Clegg wrote in #note-12:

TBD: Previously, it was an error to supply both non-empty replace_files and non-empty manifest_text in a request. Now, replace_files can reference the supplied manifest_text so it's not always an error. Should we make it an error if both are non-empty and replace_files does not actually refer to manifest_text? The manifest_text value is not used in such cases, so it's likely a mistake, OTOH it's now clear (and could be stated explicitly in the docs) what the expected outcome should be. I'm leaning "error".

Yes, I agree, the user's intention is ambiguous in that case, so we should error out if it happens. (This kind of confusion is why I wasn't enthusiastic about overloading the "manifest_text" field this way in the first place).

21701-replace-files-current-version @ 360402d2fa39b3c823bc15141ab1e99b0cc83be7 -- developer-run-tests: #4319

✅ Updated code, tests, and docs. It's an error to supply a manifest_text that won't get used at all because no entry in replace_files references it.

Tom Clegg wrote in #note-14:

21701-replace-files-current-version @ 360402d2fa39b3c823bc15141ab1e99b0cc83be7 -- developer-run-tests: #4319

All these comments are minor, nothing would block a merge, but for your consideration.

In collection_test.go, both comments are about expectFileSizes:

• if expected[path] == -1 {
          found[path] = -1
  }

  Does this condition also need to check whether path currently exists in found? As written it seems like it will cause the check to always pass even when the path didn't exist at all.
• The comment above this function starts talking about expectFiles when it means expectFileSizes.

In collections.html.textile.liquid:

• The source values for replace_files now have a mix of literal strings and placeholders. I feel like the documentation could do a better job of distinguishing which are which. In other parts of our documentation we mark up placeholders with <angle brackets>. If we followed this pattern, that means the values would be <PDH>/<path>, manifest_text/<path>, and current/<path>, with the descriptions updated to match too. I don't super love that but at least it would be consistent. What do you think?
• I think it would be a nice navigational aid if the example headers were written as a more general description of the task and marked up with h4. This would mirror the presentation of the PySDK cookbook, which we've gotten positive reader feedback on (although obviously I'm biased).

Thanks.

Brett Smith wrote in #note-15:

• [...] Does this condition also need to check whether path currently exists in found? As written it seems like it will cause the check to always pass even when the path didn't exist at all.

Pretty sure found[path] always exists at this point -- path either came from range found or was inserted in the "trimmed" bit above.

• The comment above this function starts talking about expectFiles when it means expectFileSizes.

Fixed.

• [...] mark up placeholders with <angle brackets>

Yes, I did this and I think it's much clearer, thanks.

• I think it would be a nice navigational aid if the example headers were written as a more general description of the task and marked up with h4.

Yes. Re-worded and added h4 tags with IDs.

21701-replace-files-current-version @ ab9ce35006b15e514cf71eca51570afac022e761

Tom Clegg wrote in #note-16:

21701-replace-files-current-version @ ab9ce35006b15e514cf71eca51570afac022e761

All these changes are good, thanks. I realize I could've made these comments earlier, but for some reason the new headers helped me see something else. If you want to spin this off into a separate ticket instead for scope reasons, that's fine. If you want to just commit-and-merge, that's also fine.

The documentation in this branch says a valid source is:

<PDH>/<path> where <PDH> is the portable data hash of a collection on the cluster

I assume "on the cluster" means I can't refer to collections on other clusters in a federation. But I think you have to be pretty deep in Arvados, and read very carefully, to recognize that. If I followed right, should we specifically call this out under the "Usage restrictions" section?

Does this limitation also apply to PDHs in the manifest_text? If so, that seems worth calling out too.

Under the update method header, the description for replace_files reads:

Delete and replace files and directories using content from other collections

With these changes this seems to narrow. The argument can do more than "delete and replace files and directories," and those operations can work on multiple data sources, not just "other collections." It would be nice to broaden this. I also recognize that it's a challenge to do this without making it almost uselessly generic. "Update this collection's contents by referencing collection manifests" maybe?

Thanks.