pulsar-rs icon indicating copy to clipboard operation
pulsar-rs copied to clipboard

Published 20 hours ago • verified publisher icon streamnative

feat: transactions

Open rkrishn7 opened this issue 1 year ago • 11 comments

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 avatar Aug 17 '24 02:08 rkrishn7

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

rkrishn7 avatar Aug 17 '24 02:08 rkrishn7

Bumping and tagging some additional folks - @freeznet @bewaremypower

rkrishn7 avatar Aug 25 '24 17:08 rkrishn7

I'm going to review this PR soon.

BewareMyPower avatar Oct 18 '24 01:10 BewareMyPower

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

FlorentinDUBOIS avatar Oct 18 '24 09:10 FlorentinDUBOIS

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.

BewareMyPower avatar Oct 18 '24 12:10 BewareMyPower

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.

rkrishn7 avatar Nov 17 '24 20:11 rkrishn7

Hi @BewareMyPower @FlorentinDUBOIS

Just bumping this PR! Would love to get this in soon 😄

rkrishn7 avatar Jan 11 '25 19:01 rkrishn7

I am on holiday until the 10 of February, I will be able to test after.

FlorentinDUBOIS avatar Jan 13 '25 11:01 FlorentinDUBOIS

Bumping again @FlorentinDUBOIS @BewareMyPower 😄

rkrishn7 avatar Feb 22 '25 19:02 rkrishn7

Hey :) @BewareMyPower @rkrishn7 This feature will help us at Fhenix a lot, is there any way we can help and push this through? Is it currently stuck on something specific?

roeezolantz avatar Jun 23 '25 19:06 roeezolantz

There were some test failures before, so this PR was not pushed.

image

Could you help resolve the conflicts first? I have time to review it again these days.

BewareMyPower avatar Jun 24 '25 01:06 BewareMyPower