search

heetch / felice

Felice is a nascent, opinionated Kafka library for Go, in Go
MIT License
20 stars 1 forks source link

Use JVM default partitioner as default partitioner #60

Closed sixstone-qq closed 3 years ago

sixstone-qq commented 3 years ago

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.

skateinmars commented 3 years ago

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

christophe-dufour commented 3 years ago

Agreed about major version bump

sixstone-qq commented 3 years ago

Yeah, new major version will be used. #61

philippgille commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

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.