Arvados

"200 OK, request failed, but I think you should continue as if it had succeeded" doesn't sit well with me.

Other ideas:

The client could handle the 422 -- retrieve the current version and check whether the state is still Uncommitted before giving up.

We could add a separate "commit" API that changes Uncommitted to Committed and is a no-op in other states, similar to our lock/unlock APIs.

Tom Clegg wrote in #note-2:

"200 OK, request failed, but I think you should continue as if it had succeeded" doesn't sit well with me.

Ok, but sending an update with {"state": "Committed"} and getting back a record that has {"state": "Final"} is already a thing under job reuse. We get to set the rules. Also, this has the advantage of working with older versions of arvados-cwl-runner.

Other ideas:

The client could handle the 422 -- retrieve the current version and check whether the state is still Uncommitted before giving up.

So, arvados-cwl-runner could do this, but it's going to be hard for anyone else who might be writing similar code to know that "if there's a network hiccup and we automatically retry, this API call that we're assuming is idempotent actually isn't". But it's not that hard to add a little extra code that does "catch error, try to re-fetch container, if it turns out to be in Committed or Final state, then proceed anyway".

We could add a separate "commit" API that changes Uncommitted to Committed and is a no-op in other states, similar to our lock/unlock APIs.

This is just a different way of spelling the same semantic change that I suggested. The best time to do that would have been when we designed the crunch v2 API however many years ago. Now it means we have we will have two ways to do something to maintain backwards compatibility, then have to check the API revision to decide which one to use. To be honest, I'd rather change nothing and handle it on the client than introduce a new dedicated API for this corner case.

22160-retry-container-req @ f58030d74160771bebb87dc9407614762b086445

developer-run-tests: #4628

• All agreed upon points are implemented / addressed. Describe changes from pre-implementation design.
  • yes. Attempts to recovers from errors on the client side, as agreed.
• Anything not implemented (discovered or discussed during work) has a follow-up story.
  • n/a
• Code is tested and passing, both automated and manual, what manual testing was done is described.
  • did not add additional tests, existing tests pass, but unrelated tests have failed a couple of times
• New or changed UX/UX and has gotten feedback from stakeholders.
  • n/a
• Documentation has been updated.
  • n/a
• Behaves appropriately at the intended scale (describe intended scale).
  • Improves scale by making this edge case more robust.
• Considered backwards and forwards compatibility issues between client and server.
  • no issues
• Follows our coding standards and GUI style guidelines.
  • yes

Peter Amstutz wrote in #note-7:

22160-retry-container-req @ f58030d74160771bebb87dc9407614762b086445

What's here is fine. Given how much time we spent discussing this and convincing ourselves that this was best done in the client, it seems to me like it'd be really good to have a test specifically for it. But I won't insist.

Brett Smith wrote in #note-9:

Peter Amstutz wrote in #note-7:

22160-retry-container-req @ f58030d74160771bebb87dc9407614762b086445

What's here is fine. Given how much time we spent discussing this and convincing ourselves that this was best done in the client, it seems to me like it'd be really good to have a test specifically for it. But I won't insist.

I went ahead and added a test. Since it didn't turn up any unexpected behavior and the code you already signed off on didn't change, I went ahead and merged. Thanks.