-
Notifications
You must be signed in to change notification settings - Fork 386
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
feat: Add transaction prefix option #1733
Conversation
Added some fixes, instead of waiting for a review cycle. |
Should we think about adding a spec for this? |
I don't think we can observe the transaction id in any reasonable way in a test without a lot of effort, so probably not worth it. |
We can try running one of the existing test cases with a prefix specified though. |
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.
LGTM.
@@ -137,7 +138,7 @@ object Transactional { | |||
|
|||
val flow = Flow | |||
.fromGraph( | |||
new TransactionalProducerStage[K, V, ConsumerMessage.PartitionOffset](settings, settings.transactionIdPrefix + transactionalId) | |||
new TransactionalProducerStage[K, V, ConsumerMessage.PartitionOffset](settings, transactionalId) |
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 had considered adding it to the above method, but wouldn't it be better to keep it at the underlying one? Maybe you chose the top level since this one is deprecated? I had assumed it might be made private is all?
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.
the deprecated one expects the user to "design" their entire transaction id, so we shouldn't mess with it, they can (and do) already have whatever prefix they want
An add-on to #1728 for a bit more configurability