fission/fission

Sequential function invokations when using Kafka triggers

Opened this issue · 12 comments

Fission/Kubernetes version

$ fission version
client:
  fission/core:
    BuildDate: "2020-02-03T08:40:57Z"
    GitCommit: bda974a72c9093e241c1dae6a7fc1a2d16e28b02
    Version: 1.8.0
server:
  fission/core:
    BuildDate: "2020-02-03T08:40:57Z"
    GitCommit: bda974a72c9093e241c1dae6a7fc1a2d16e28b02
    Version: 1.8.0

Describe the bug

Not a bug but more a feature request.

From what I see in the code of mqtrigger, the processing of incoming messages is performed in concurrently.

For some use cases, preserving ordering of function calls can be very important from a business point of view (especially if the functions have side-effects). Thus, it would be interesting to have an option to configure the message consumption policy at the trigger level: either concurrent or sequential.

What do you think ?

This feature makes sense to me. I'm thinking maybe we can expose an option that allows user to set the concurrency either to 1 or N and mqtrigger can follow the setting to limit max concurrency for function invocation. Do you think this approach can fulfill your needs?

I think there was a user who was facing delay in consuming Kafka messages and hence as part of related PR (#1355) and issue (#1354) this was changed.

It makes sense that we allow both the behaviours to the user at the trigger level by exposing a new flag (Which can be done potentially for all kinds of triggers eventually).

I would like to contribute this feature, please assign this to me

hey @daniel-shuy Please take it up and let us know if you need any help. GitHub doesn't allow me to assign to you for some reason.

Should I add a message consumption policy (concurrent or sequential) option as @ccamel suggested, or max concurrency option as @life1347 suggested?

@daniel-shuy From my point of view, the suggestion made by @life1347 is fine with me.

Hey @daniel-shuy The use case for sequential function invocations is typically for guarantees of order - so I would suggest a simple flag - either concurrently or sequentially for now.

@vishal-biyani thanks, that would be so much easier!

Should the default be concurrent/sequential?

Just looking at the Kafka message queue alone, for backwards compatibility, concurrent should be the default.

Since the flag is added to mqtrigger, it is shared by all message queues, so I can also implement them for Azure Queue Storage and NATS.

However, the Azure Queue Storage message queue is currently consuming messages concurrently, while the NATS message queue is currently consuming message sequentially. This means that if I were to implement this flag for all message queues, it would break backwards compatibility one way or the other.

Hey @daniel-shuy Are you thinking of adding this as a field on MQTrigger CRD? I am thinking that we should make this an environment variable that is wired up to the Helm values file.

In case of backward compatibility, I would stick to concurrency as default - and you can configure it individually for all MQs in values.yaml without breaking compatibility!

In any case, we will move eventually to Keda based MQT integrations and deprecate these custom-built once. (Details here and here)

I added the flag to the mqtrigger command.

I've created a preliminary PR at #1814

@vishal-biyani are you suggesting that instead of adding a flag to the mqtrigger command, I check for the presence of an environment variable and retain the current MQT specific behavior if its not specified?