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

Fix: Issues with cooperative-sticky strategy #593

Open
wants to merge 8 commits into
base: master
Choose a base branch
from

Conversation

golanbz
Copy link

@golanbz golanbz commented Sep 2, 2024

Description

How Has This Been Tested?

  • Introduced a new sample project specifically designed to evaluate and test the support for the cooperative sticky strategy, allowing for more thorough analysis.
  • Unit tests were added for the updated classes to address the new behavior.

Checklist

  • My code follows the style guidelines of this project
  • I have performed a self-review of my own code
  • I have added tests to cover my changes
  • I have made corresponding changes to the documentation

Disclaimer

By sending us your contributions, you are agreeing that your contribution is made subject to the terms of our Contributor Ownership Statement.

…world" scenarios (e.g., cooperative sticky).

* Enabled automatic committing with `confluent auto commit: true` instead of relying solely on manual commits, but only when the consumer strategy is cooperative sticky. (Refer to the open librdkafka issue at confluentinc/librdkafka#4059).
… support for the cooperative sticky strategy, allowing for more thorough testing and analysis.
…world" scenarios (e.g., cooperative sticky). Fixes issue Farfetch#557 and Fixes issue Farfetch#456

* Enabled automatic committing with `confluent auto commit: true` instead of relying solely on manual commits, but only when the consumer strategy is cooperative sticky. (Refer to the open librdkafka issue at confluentinc/librdkafka#4059).
… support for the cooperative sticky strategy, allowing for more thorough testing and analysis.
@dosper7
Copy link

dosper7 commented Sep 11, 2024

Great work 🚀 with this PR. Can you also add some tests as well to validate this use case? 🙇

- Added 3 new test files to improve coverage:
  - `ConsumerConfigurationBuilderTests.cs`
  - `KafkaConfigTests.cs`
  - `PartitionAssignmentStrategyTests.cs`
- Fixed a bug in `ClusterConfiguration` related to the `AutoCommitInterval` initialization.
@BasJ93
Copy link

BasJ93 commented Nov 14, 2024

We'd love to have this feature. Any updates on the implementation of the tests?

@golanbz
Copy link
Author

golanbz commented Nov 14, 2024

We'd love to have this feature. Any updates on the implementation of the tests?

Unit tests were added 2 months ago.

@BasJ93
Copy link

BasJ93 commented Nov 15, 2024

Great. Any updates on getting the feature reviewed @dosper7?

@AlexeyRaga
Copy link

Great to see it happening! Thanks @golanbz for working on it!

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

Successfully merging this pull request may close these issues.

4 participants