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

Closed
dlyash opened this issue Oct 14, 2024 · 4 comments · Fixed by #3602
Closed

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!

@Dltmd202
Copy link
Contributor

I created fix PR #3602~!

@sobychacko sobychacko added this to the 3.3.0 milestone Oct 30, 2024
sobychacko pushed a commit that referenced this issue Oct 30, 2024
…eddedkafka

Fixes: #3557 

#3557

 * Adjust the replication factor for the transaction state topic on `EmbeddedKafka` based on the broker count in 
  `EmbeddedKafka`.
* Keep the default replication factor of 3. 
* Adding tests to verify
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants