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

Add producer transaction support #293

Closed
wants to merge 16 commits into from

Conversation

roignpar
Copy link
Contributor

@roignpar roignpar commented Aug 20, 2020

To do:

  • ThreadedProducer
  • FutureProducer
  • proper error handling
  • understand if it makes sense to serialize and deserialize consumer group metadata like other librdkafka wrappers do
  • documentation
  • tests

@roignpar
Copy link
Contributor Author

roignpar commented Aug 20, 2020

Opened this PR following #289.
Will implement proper error handling next. In the meantime, all feedback is welcome!

@roignpar
Copy link
Contributor Author

@benesch any clue how to make the transaction methods async on the FutureProducer? librdkafka doesn't support callbacks for transaction operations.

@benesch
Copy link
Collaborator

benesch commented Aug 28, 2020

Sorry, I've been swamped lately. About to head off on vacation, which means I should actually have a few spare hours to look at this. But I gave it a quick skim and everything looks very promising!

@benesch any clue how to make the transaction methods async on the FutureProducer? librdkafka doesn't support callbacks for transaction operations.

Nope, afraid not. Maybe there are some tricks along the lines mentioned in #273, but any solution along those lines wouldn't be plug n play.

@roignpar
Copy link
Contributor Author

roignpar commented Sep 1, 2020

Even with the event interface, there seem to be no transaction events defined in librdkafka.
I propose that as part of this PR the FutureProducer transaction methods be sync and an async API be implemented separately when it becomes feasible.
Since the methods are the same on all producer types and both ThreadedProducer and FutureProducer delegate to the base producer, would it make sense to create a TransactionalProducer trait similar to the Consumer trait?

@roignpar
Copy link
Contributor Author

While writing the tests I ran into something that I was able to reproduce using the go librdkafka wrapper and opened an issue.
Added an example and more documentation.
Feel free to change anything!

@roignpar roignpar changed the title WIP Add producer transaction support Add producer transaction support Oct 1, 2020
@benesch
Copy link
Collaborator

benesch commented Oct 12, 2020

Sorry for the massive delay here. I'm working through this today one commit at a time.

@damoon
Copy link

damoon commented Nov 30, 2020

I am very new to Rust, but I would love to see exactly once semantics for Kafka.
Anything I can do to help?

benesch added a commit to benesch/rust-rdkafka that referenced this pull request Jan 5, 2021
Co-authored-by: Robert Ignat <robert.ignat91@gmail.com>

Closes fede1024#289.
Closes fede1024#293.
benesch added a commit to benesch/rust-rdkafka that referenced this pull request Jan 5, 2021
Co-authored-by: Robert Ignat <robert.ignat91@gmail.com>

Closes fede1024#289.
Closes fede1024#293.
@benesch
Copy link
Collaborator

benesch commented Jan 5, 2021

Thanks very much for this. This was an absolutely awesome PR. I'm sorry that my review stalled out on this for so long. I took this and made a few changes in #323. The only real substantive change was fixing a few memory safety issues with RDKafkaError.

By the way, there's now a Producer trait, to mirror the Consumer trait, so that's where I moved the transaction-related methods to. It's a shame that they're blocking methods on the FutureProducer, but there's not much we can do about that.

@benesch benesch closed this Jan 5, 2021
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 this pull request may close these issues.

3 participants