Add an overwrite bit to UpdateActionResultRequest #314
Conversation
| // On a successful call, if a preexisting cache result already exists, | ||
| // the server MUST ignore the provided ActionResult and return the existing | ||
| // one instead. If a preexisting cache result does not exist, the server | ||
| // MUST write the provided ActionResult to the cache and use that | ||
| // ActionResult as the return value. | ||
| NOOVERWRITE = 2; |
There was a problem hiding this comment.
How will the client be able to accurately tell whether the overwrite took place or not? Calling proto.Equal() or something may not be realistic, as there is no guarantee that a server does not modify/normalize/... ActionResults provided by the client.
There was a problem hiding this comment.
This might require an API version bump.
There was a problem hiding this comment.
Does the client need to know from a functional standpoint, or just from a performance one? Functionally, the client mostly cares that the set of output paths and digests match, and explicitly does not care about some fields like execution_metadata. (I'm actually not sure how we expect the client to handle things like stdout/stderr, but that's an existing sore spot in the API, not something specific to this change.) I can see the argument, though, that for performance reasons the client doesn't want to have to reprocess the whole output tree just to verify that things haven't changed.
If performance is the only concern, we could choose to break with the usual API semantics and specify that the server can return SUCCESS with an empty ActionResult if the provided ActionResult was written to the cache, so then the client only needs to compare the ActionResult if it's non-empty. That's clearly a larger change than I intended here (although we could also limit the impact by saying that this new semantic doesn't apply when the client uses UNSPECIFIED).
I'd also be open to creating a new API call, something like WriteActionResult, and deprecating UpdateActionResult. We had basically included UpdateActionResult in the API for completeness, but didn't really give much thought into how it works. Some of those gaps are more painfully obvious now.
There was a problem hiding this comment.
I'm absolutely fine with not providing a mechanism for informing the client whether an overwrite took place. But could we at least document this as part of the .proto?
There was a problem hiding this comment.
I took another look at the existing comments, which are delightfully vague. I've updated this PR with what I hope works as a way to preserve the legacy behavior while providing a better path forward for clients and servers that support the new overwrite bit.
Obviously this will be a minor version bump.
The existing UpdateActionResult allows for the server to return either the input ActionResult as the output, or return a pre-existing ActionResult from the cache. This bit allows the client to specify whether or not to overwrite an existing entry.