search

fd4s / fs2-kafka

Functional Kafka Streams for Scala
https://fd4s.github.io/fs2-kafka
Apache License 2.0
295 stars 100 forks source link

KafkaConsumer should not be sealed #380

Closed dcsobral closed 3 years ago

dcsobral commented 4 years ago

KafkaConsumer is a sealed abstract class. That serves no purpose since it is not an enumeration that gets pattern matched somewhere and it has no concrete implementations.

On the flip side, being sealed means you can't mock it easily for testing. KafkaProducer is also abstract but not sealed.

bplommer commented 3 years ago

I agree. @vlovgr any thoughts on this?

vlovgr commented 3 years ago

We should probably add the remaining bits in https://github.com/fd4s/fs2-kafka/issues/94 before unsealing.

bplommer commented 3 years ago

Rather than unseal KafkaConsumer, can we modularise the API into several unsealed traits that users can code and mock against? Then KafkaConsumer itself can mix those traits in but remain sealed, letting us keep the flexibility to extend the API, and users only need to mock the traits they're using.

vlovgr commented 3 years ago

If we can figure out a good split, that sounds pretty reasonable to me. 👍

dcsobral commented 3 years ago

I'm not sure there's anything to split here, given the underlying API it provides access to. If admin was there, that would be one thing, but KafkaConsumer is rather focused.

dcsobral commented 3 years ago

And, on the other hand... why seal KafkaConsumer? There is no gain there. It is a pure trait, with no implementation, and it is not an ADT. The only argument for keeping it sealed is that it is sealed right now. Can anyone come up with a reason other than that? It's not that I think there's little gain in keeping it sealed, but that I don't see any gain whatsoever.

vlovgr commented 3 years ago

The only gain is that we can keep adding functions to the sealed KafkaConsumer without breaking binary compatibility.

bplommer commented 3 years ago

I'm not sure there's anything to split here, given the underlying API it provides access to. If admin was there, that would be one thing, but KafkaConsumer is rather focused.

Don't think I agree. KafkaConsumer has methods for:

There's a definite possibility that we'll want to support new features that are added to Kafka in the future, and/or add higher-level functionality. Splitting out traits also lets us restrict which capabilities we pass around, so makes it easier to reason about what a program does and doesn't do.