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

Changed consumer/message processing to synchronized mode - former async (#4095) #4105

Merged

Conversation

dao74
Copy link

@dao74 dao74 commented Jun 5, 2023

resolves #4095

The asynchronous processing of incoming message can exhaust the database connection pool if there is a (big) lag in the kafka queue.
On other hand, the ACK of each message should perform after workflow is successfully created (or retried when the process fails) to not lose the kafka message.
This PR is fixing the synchronize processing and ACK issue.

Note:
Changing to synchronize will slowdown the whole process for larger amount of kafka messages. Therefore, the consumer should be increased - in the same count like the partition size of the topic.
Therefore the partition size should be configurable in the Kafka Activity.

@dao74
Copy link
Author

dao74 commented Jun 6, 2023

@dotnet-policy-service agree

@sfmskywalker sfmskywalker changed the base branch from 2.12.x to master June 8, 2023 09:37
@sfmskywalker sfmskywalker merged commit 7378681 into elsa-workflows:master Jun 8, 2023
@dwoldo
Copy link

dwoldo commented Oct 10, 2023

This also resolves #4192

@Snotax
Copy link

Snotax commented Oct 11, 2023

I'm not sure this fixes the problem correctly. With this implementation, the consumation speed is unusable for us(we're talking seconds per message). @dao74 What did you do to increase the speed or with what kind of data did you test this ?

We're thinking of reverting to the old version again.

FYI @sfmskywalker

@Snotax
Copy link

Snotax commented Oct 11, 2023

@dao74 Where did you even encounter your original error, we couldn't reproduce this?

@dao74
Copy link
Author

dao74 commented Oct 11, 2023 via email

@Snotax
Copy link

Snotax commented Oct 11, 2023

We are experiencing an unusable increase in time used to handle incoming messages. Like multiple seconds per message.

@dao74
Copy link
Author

dao74 commented Oct 11, 2023 via email

@Snotax
Copy link

Snotax commented Oct 12, 2023

I know this line. But the increase in consumation time is significantly higher with your change.

@dao74
Copy link
Author

dao74 commented Oct 12, 2023

I know this line. But the increase in consumation time is significantly higher with your change.

Of course, process time might be increased because of the sync mode (instead of async) but I am not sure how significant - we didn't compared or made a benchmark. The async processing brings other problems, therefore - in my opinion - it must be stay in sync mode.

We made the performance tests (bulk processing) without the delay line and we were satisfied with the result. You should do the same before making a decision.

@Snotax
Copy link

Snotax commented Oct 12, 2023

Ok, we will do some more performance testing next week. We'll keep you updated.

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

Successfully merging this pull request may close these issues.

Elsa 2: Kafka Activity module is preparing consumed message in an asynchronous way
4 participants