-
Notifications
You must be signed in to change notification settings - Fork 38
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
use Kafka Producer Consumer interfaces #245
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you, I have just one comment!
I will release a snapshot if you'd like to try it out before including it in the official release
} | ||
|
||
/** Builds a [[KafkaProducer]] instance with provided Apache Producer. */ | ||
def apply[K, V](config: KafkaProducerConfig, sc: Scheduler, producerRef: ApacheProducer[K, V])( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think producerRef
should be taken lazily, or as a Task
/ Coeval
.
The implementation can close ApacheProducer
and that should be only done by the producer (creator) of the resource.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this what you mean?
/** Builds a [[KafkaProducer]] instance with provided Apache Producer. */
def apply[K, V](config: KafkaProducerConfig, sc: Scheduler, producer: Coeval[ApacheProducer[K, V]])(
implicit K: Serializer[K],
V: Serializer[V]): KafkaProducer[K, V] = {
lazy val producerRef = producer.value()
new Implementation[K, V](config, sc, producerRef)
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, although Implementation
would have to use Coeval
as well, otherwise it will initialize right away
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, pushed a commit that uses Coeval producer.
@Avasil - Just to let you know, we were able to use the snapshot jar which includes this PR and we were able to wrap Tracing as expected. |
@arun0009 That's awesome, thanks for the update :) |
Use Kafka Producer, Consumer interfaces as types where appropriate instead of using implementation types. Resolves #244