RemoteEx 2.2: Promote platform properties from command to action

[edbaunton]

Platform properties are currently a member of the command message which is
referred to in the action by digest. This requires the Execution Service to
make a call to the CAS to retrieve the contents of the command if it wishes to
inspect it. The platform properties are commonly used for making routing
decision about the action. Therefore, in order to route an action common
execution service implementations must introduce an additional call to the CAS
to fully hydrate the action and determine where it should be routed.

This commit promotes the platform properties from the command to the action. We
deprecate the platform properties contained within the action and bump the
minor version to version 2.2, following the model for output_paths.

Fixes #166

Signed-off-by: Ed Baunton ebaunton1@bloomberg.net

[ulfjack]

This seems reasonable.

[EdSchouten]

Looks good!

Question: should we make it a bit more explicit what a transition path looks like? Should clients provide both for the time being? Should the one in Action be preferred over the one in Command?

At the same time, some time ago I got a request on the Buildbarn side whether it's required to specify platform properties at all. Is it valid to send requests where both Action.platform and Command.platform are null/nil/.../None, or is it required that at least one of them contains a message?

[edbaunton]

I am not 100% familiar with the right way to perform a proto field migration but I tried to update the docs as I would myself have expected to do it. Basically if you use 2.2 you must use the new field, if you are not using 2.2 you must populate the old and can populate the new if you want. From 2.2 you shouldn't populate the old.

From your comment @EdSchouten I also added in an 'optional' to the new field to denote that they can be empty. I didn't change that for the deprecated field.

[EdSchouten]

Giving this some more thought, one 'downside' of this approach is that clusters can no longer use the command digest as a key for correlating equivalent actions. For example, inspecting historical build actions to see whether their resource usage increased over time, etc..

That said, clusters are still capable of achieving this by introducing their own composite type:

message Key {
  Digest command_digest = 1;
  Platform platform = 2;
}

And hashing that to obtain a proper key for stuff like that. There's no need to account for that in the client <-> cluster protocol.

[edbaunton]

How do others feel about how strict we should be with version migrations?

[edbaunton]

I have simplified the documentation to keep this simple change moving.

[edbaunton]

Keen for one more approver @EdSchouten and @ulfjack if you have a chance