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

Introduce partitioner interface #592

Merged
merged 13 commits into from
Jun 30, 2023
Merged

Conversation

mlowicki
Copy link
Collaborator

@mlowicki mlowicki commented Jun 23, 2023

Add interface to specify custom partitioners by extending ProducerContext trait with capability to return optional custom partitioner.

@mlowicki mlowicki marked this pull request as ready for review June 23, 2023 08:39
@scanterog
Copy link
Collaborator

scanterog commented Jun 23, 2023

Thanks for contributing. I've reviewed this and I personally think it would be better to use an associated type instead of generic type for the Partitioner in the ProducerContext.

I've spent some time and created this patch so you can observe the difference. By doing that we avoid introducing the Partitioner trait pretty much everywhere. The patch might require further testing. It was mostly to illustrate the idea. Ideally we also want a separate CollectingContext context for testing the custom partitioner on the low level producer tests.

I'm going on vacation next week and I'll let you sync with @duarten and @davidblewett on what you all think is better for this.

@mlowicki
Copy link
Collaborator Author

Thanks for contributing. I've reviewed this and I personally think it would be better to use an associated type instead of generic type for the Partitioner in the ProducerContext.

I've spent some time and created this patch so you can observe the difference. By doing that we avoid introducing the Partitioner trait pretty much everywhere. The patch might require further testing. It was mostly to illustrate the idea. Ideally we also want a separate CollectingContext context for testing the custom partitioner on the low level producer tests.

I'm going on vacation next week and I'll let you sync with @duarten and @davidblewett on what you all think is better for this.

Yeah, so using associated type was my first approach (I've working version somewhere). Later on decided to use generic type because of few reasons:

For sure using generic types requires more changes in the code base + signatures are becoming more complex. I'll start a thread on Slack to discuss on the direction here.

src/producer/mod.rs Outdated Show resolved Hide resolved
Copy link
Collaborator

@davidblewett davidblewett left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1

@davidblewett davidblewett merged commit d036b2e into fede1024:master Jun 30, 2023
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.

5 participants