cloudstate
cloudstate copied to clipboard
cloudstateio
FunctionCommand has no id
In function.proto the message FunctionReply defines response to be one of Failure and other messages. Failure is defined in entity.proto and carries a command_id field which "Must match the input command". Unfortunately FunctionCommand – which is used for all StatelessFunction methods – carries no command id.
Why do we use FunctionCommand instead of the "normal" Command (from entity.proto) which is used elsewhere (e.g. for EventSourced)? That "normal" Command carries an id.
Alternatively we could add an id to FunctionCommand. But we should also clarify and document why StatelessFunction uses a specialized command.
Browsing through the protocol I get the impression that function.proto is very different from event_sourced.proto and crdt.proto. Why is it not using ClientAction and SideEffect from entity.proto? Is it a little outdated and needs some love?
I think the reason that function.proto is different is because it doesn't deal with Entities at all.
Well, it's not an entity in the sense that it is served with data by the platform nor is able to write data to the platform. But nevertheless it is treated as an Entity in the Cloudstate protocol and by the proxy.
Apart from this rather philosophical question, the protocol is not correct wrt StatelessFunctions as I have described above: it is impossible to return a FunctionReply with a response of type Failure. We need to fix that either way.
@hseeberger Having a command_id does indeed make sense in the general case, yes, because of correlation.
Instead of special casing StatelessFunction we should consider using Command instead of FunctionCommand and inside of FunctionReply.response we should use ClientAction.
On a similar topic in the Spec Draft PR about concurrent command handling, James mentioned https://github.com/cloudstateio/cloudstate/pull/119#discussion_r404449523 that
the proxy only issues one command at a time
This was lead to the question how the proxy can correlate a Failure without a command_id set. Since "The protocol supports commands being handled concurrently" it would be beneficial to have this correlation explicitly.
I agree with @marcellanz that if simultaneous manipulations are possible then the correlations must be explicit. In fact, mandatorily they should be explicit. @hseeberger maybe that's why it is not possible to return a failure currently
I've had a look through, including the history for this.
So #48 added support for multiple entities — entity.Reply was simplified down to just the payload, EventSourcedReply and CrdtReply track the command id, event-sourced and CRDT share the common entity.Failure while the FunctionReply doesn't use failures.
In #154, FunctionReply responses could be failures, and I expect that entity.Failure should've had the command id removed, with EventSourcedFailure and CrdtFailure messages added — so that everything mirrored the reply messages. There's no command id tracking or checking for functions.
So it looks like there shouldn't be a command id on entity.Failure, and there should instead be failure messages specific to event-sourced and CRDTs.
I think supporting correlation ids for functions would be separate from cleaning up the command ids.
And agree the way that things are structured and messages shared could be more understandable, especially with functions being and not being entities.
