feat: transactions - Githubissues

streamnative / pulsar-rs

Rust Client library for Apache Pulsar

Other

369 stars 121 forks source link

feat: transactions #323

Open rkrishn7 opened 3 months ago

rkrishn7 commented 3 months ago

This PR adds client support for Pulsar Transactions.

Resolves #253

Notes:

This PR tries to mimic the Java client where possible. More details on transactions in Pulsar can be found here.
Will likely need to edit the Pulsar service container in CI to enable the TC and bootstrap transaction metadata
This PR necessitates a breaking version change as it adds a new error variant

Open Questions:

Should these changes be gated behind a feature flag?
This change likely has implications when buffering in the producer. Thoughts on how we should handle that?
The main Transaction API uses internal synchronization to provide a "no mut" public interface. Are there any alternative approaches that may be better?

rkrishn7 commented 3 months ago

@FlorentinDUBOIS Tagging you for visibility. I know this is a lot so no rush here!

rkrishn7 commented 2 months ago

Bumping and tagging some additional folks - @freeznet @bewaremypower

BewareMyPower commented 1 month ago

I'm going to review this PR soon.

FlorentinDUBOIS commented 1 month ago

Thanks you @rkrishn7, I will take a look at this pull request and test it in production next week. Sorry for the delay for my answer

BewareMyPower commented 1 month ago

Should these changes be gated behind a feature flag?

I don't think so.

This change likely has implications when buffering in the producer. Thoughts on how we should handle that?

Sorry I don't get it. It only registers the partition with the transaction id via the ADD_PARTITION_TO_TXN command before adding the message to the buffer. It's necessary for transactional messages and has no impact on the regular send without transaction.

rkrishn7 commented 5 days ago

Should these changes be gated behind a feature flag?

I don't think so.

👍🏾

This change likely has implications when buffering in the producer. Thoughts on how we should handle that?

Sorry I don't get it. It only registers the partition with the transaction id via the ADD_PARTITION_TO_TXN command before adding the message to the buffer. It's necessary for transactional messages and has no impact on the regular send without transaction.

Sorry! To clarify, I think there's a potential footgun lurking here. For example:

Let's say we've enabled transactions and start producing transactional messages with a batching producer. If we haven't hit the batching threshold before the transaction's timeout, then all those buffered messages will fail to be produced. This is subtle, but definitely seems undesirable.

It seems like either transactional messages should bypass batching or we should warn the user if both transactions and batching are enabled.