Skip to content

Conversation

JustinPihony
Copy link
Contributor

An add-on to #1728 for a bit more configurability

@johanandren
Copy link
Contributor

Added some fixes, instead of waiting for a review cycle.

@sebastian-alfers
Copy link
Contributor

Should we think about adding a spec for this?

@johanandren
Copy link
Contributor

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.

@johanandren
Copy link
Contributor

We can try running one of the existing test cases with a prefix specified though.

@johanandren johanandren changed the title Add transaction prefix option feat: Add transaction prefix option Apr 12, 2024
Copy link
Contributor

@ennru ennru left a 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)
Copy link
Contributor Author

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?

Copy link
Contributor

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

@johanandren johanandren merged commit 0c02af9 into main Apr 15, 2024
@johanandren johanandren deleted the transaction-prefix branch April 15, 2024 07:01
@johanandren johanandren added this to the 5.0.1 milestone May 3, 2024
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.

4 participants