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

KafkaConsumerObservable and its subclasses use KafkaConsumer (implementation) instead of Consumer (interface) #244

Closed
girishkolantra opened this issue Dec 30, 2020 · 4 comments · Fixed by #245

Comments

@girishkolantra
Copy link

girishkolantra commented Dec 30, 2020

Use of the implementation instead of the interface as follows

trait KafkaConsumerObservable[K, V, Out] extends Observable[Out] {
  protected def config: KafkaConsumerConfig
  protected def consumer: Task[KafkaConsumer[K, V]]

would make integration with tracing libraries like zipkin more difficult. Using the Consumer interface instead would allow for easy integration with tracing libraries

@Avasil
Copy link
Collaborator

Avasil commented Dec 30, 2020

How would that work?

I think none of the Consumer interface methods are actually implemented by KafkaConsumerObservable, all of it is delegated to actual KafkaConsumer.

There is a constructor that allows to pass KafkaConsumer by the caller:

def apply[K, V](
  cfg: KafkaConsumerConfig,
  consumer: Task[KafkaConsumer[K, V]]): KafkaConsumerObservable[K, V, ConsumerRecord[K, V]]

I didn't use these tracing libraries but if they wrap Kafka Consumer, couldn't we pass this wrapped tracing consumer to the KafkaConsumerObservable and have tracing on poll, commit etc?

@girishkolantra
Copy link
Author

The zipkin tracing library has a tracing consumer that decorates a Consumer[K, V]. So if the apply method in the KafkaConsumerObservable accepted the interface, the integration would be seemless

@Avasil
Copy link
Collaborator

Avasil commented Dec 30, 2020

Ahh, sorry, I misunderstood.

Yes, in this case, the change would be very useful! Not binary compatible but we don't provide a guarantee here so it's fine

@Avasil
Copy link
Collaborator

Avasil commented Jan 1, 2021

@girishkolantra @arun0009

I've released a SNAPSHOT with your changes: 1.0.0-RC6-c7231f9-SNAPSHOT

Let me know if that works out for you, or if you need anything more to be included in a proper release.

Thank you for the PR!

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 a pull request may close this issue.

2 participants