Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Sequential function invokations when using Kafka triggers #1569

Open
ccamel opened this issue Apr 12, 2020 · 12 comments · May be fixed by #1814
Open

Sequential function invokations when using Kafka triggers #1569

ccamel opened this issue Apr 12, 2020 · 12 comments · May be fixed by #1814

Comments

@ccamel
Copy link
Contributor

ccamel commented Apr 12, 2020

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 ?

@life1347
Copy link
Member

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?

@vishal-biyani
Copy link
Member

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).

@daniel-shuy
Copy link

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

@vishal-biyani
Copy link
Member

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.

@daniel-shuy
Copy link

daniel-shuy commented Oct 6, 2020

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

@ccamel
Copy link
Contributor Author

ccamel commented Oct 6, 2020

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

@vishal-biyani
Copy link
Member

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.

@daniel-shuy
Copy link

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

@daniel-shuy
Copy link

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.

@vishal-biyani
Copy link
Member

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)

@daniel-shuy daniel-shuy linked a pull request Oct 7, 2020 that will close this issue
@daniel-shuy
Copy link

I added the flag to the mqtrigger command.

I've created a preliminary PR at #1814

@daniel-shuy
Copy link

@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?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants