Skip to content
This repository has been archived by the owner on Jun 23, 2023. It is now read-only.

Use JVM default partitioner as default partitioner #60

Merged
merged 4 commits into from
Feb 5, 2021

Conversation

sixstone-qq
Copy link
Contributor

It uses murmur2 as custom hasher partitioner which is used by default
from JVM kafka client by default.

This is a backwards incompatible change to make easy to work in Kafka
ecosystem specially in Kakfa Streams cooperation.

@sixstone-qq sixstone-qq self-assigned this Feb 3, 2021
@sixstone-qq sixstone-qq requested a review from a team February 3, 2021 10:44
@sixstone-qq sixstone-qq marked this pull request as ready for review February 4, 2021 08:38
@skateinmars
Copy link
Member

Should we change this default without any warning?
Is it worth bumping the major version?

@christophe-dufour
Copy link

Agreed about major version bump

@sixstone-qq
Copy link
Contributor Author

sixstone-qq commented Feb 4, 2021

Yeah, new major version will be used. #61

@philippgille
Copy link
Member

Could it be an option to keep felice as is, and only modify the Config returned from NewConfig in the caller in kafka-go?

@sixstone-qq
Copy link
Contributor Author

Could it be an option to keep felice as is, and only modify the Config returned from NewConfig in the caller in kafka-go?

Why so? If we bump to next major version, downstream users are aware of this.

@philippgille
Copy link
Member

Why so? If we bump to next major version, downstream users are aware of this.

I'm not worried about downstream users, I just don't see the need to change the low level lib and release a new major version, while its config seems to be easily changeable in higher level libs.

@sixstone-qq
Copy link
Contributor Author

Why so? If we bump to next major version, downstream users are aware of this.

I'm not worried about downstream users, I just don't see the need to change the low level lib and release a new major version, while its config seems to be easily changeable in higher level libs.

As stated in README, it's an opinionated library for Kafka, so that could imply that it has a default configuration that must not be changed in the most of users. I think any other configuration change that could affect downstream users like AckLocal would imply the same bump version in my opinion. What do you think?

@skateinmars
Copy link
Member

I'd say if we bump the major version we can freely change this default
That said it's probably worth mentioning the default configuration in the README (and we can also fill in the changelog)

@philippgille
Copy link
Member

philippgille commented Feb 4, 2021

I think any other configuration change that could affect downstream users like AckLocal would imply the same bump version in my opinion.

When changing the config here, yes definitely. But given that it's a public repo, we can have the OSS community in mind: After a bump we're likely to focus on 2.x for future fixes and features, so non-Heetch users of the library now all have to change their code to set the previous partitioner, without gaining anything else from the major version bump. For a struct or method signature change there's sometimes no way around a breaking change, but here it's just a config change which could be done in our other libraries.

I see the pros on the other side: Less code to change for us, there are not many non-Heetch users of the lib, it's described as opinionated which I agree often includes configuration.

Ok let's do it this way.

README.md Outdated Show resolved Hide resolved
Co-authored-by: Jean-Philippe Moal <skateinmars@users.noreply.github.com>
@sixstone-qq sixstone-qq merged commit d681469 into master Feb 5, 2021
@sixstone-qq sixstone-qq deleted the add-jvm-partitioner branch February 5, 2021 08:48
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants