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

Lower the replication factor for transactions topic when using embedded Kafka broker for tests #3557

Open
dlyash opened this issue Oct 14, 2024 · 3 comments

Comments

@dlyash
Copy link

dlyash commented Oct 14, 2024

Expected Behavior

It would be nice if the embedded broker started by @EmbeddedKafka (or the similar JUnit Rule) defaulted the replication factor for the transaction state topic to the number of brokers, or a min(<number-of-brokers>, 3).

Current Behavior
The embedded broker starts with the default configuration of transaction.state.log.replication.factor = 3 which doesn't make sense for a single-broker cluster, which is quite common in tests:

org.apache.kafka.common.errors.InvalidReplicationFactorException: Replication factor: 3 larger than available brokers: 1.

Context
It's not a big deal, as it can be resolved by explicitly setting broker properties like follows. But a meaningful default would save some time for thousands of developers googling the problem again and again.

@EmbeddedKafka(brokerProperties={
    "transaction.state.log.replication.factor=1"
})
@sobychacko
Copy link
Contributor

sobychacko commented Oct 14, 2024

@dlyash The tests we have in the framework for transactions that use EmbeddedKafka do exactly that - i.e., setting that property transaction.state.log.replication.factor to 1. Should we document that and not make any code changes primarily because retrieving the number of brokers consistently might be challenging? What do you think?

@dlyash
Copy link
Author

dlyash commented Oct 15, 2024

@sobychacko A few lines in the documentation surely won't hurt. Technically it's already documented in Kafka's own documentation:

Note that, by default, transactions require a cluster of at least three brokers 
which is the recommended setting for production; for development you can change this, 
by adjusting broker setting transaction.state.log.replication.factor.

But I bet 99% of developers will only look for them once running into the issue and then will end up on the StackOverflow anyway.

As a developer, I would appreciate the testing framework taking care of it for me. That's why I was suggesting the programmatic solution.


.. retrieving the number of brokers consistently might be challenging

Oh, is it? I didn't dive too deep into the code, but I was imagining something like this in the EmbeddedKafkaContextCustomizer, but I may be simplifying things of course:

properties.putIfAbsent("transaction.state.log.replication.factor", Math.min(embeddedKafka.count(), 3));

@sobychacko
Copy link
Contributor

Ok, you can give it a try and see if that works and is stable. If so, feel free to submit a PR. Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants