Skip to content

KAFKA-14569: Migrate Kafka Streams tests from Zookeeper to KRaft #15341

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.

Sign up for GitHub

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

Jump to bottom
Closed
wants to merge 1 commit into from
Closed

KAFKA-14569: Migrate Kafka Streams tests from Zookeeper to KRaft #15341

wants to merge 1 commit into from

Conversation

mdedetrich
Copy link
Contributor

Migrates stream tests from zookeeper to KRaft

@mdedetrich mdedetrich force-pushed the migrated-streams-tests-from-zk-to-kraft branch 6 times, most recently from f21a684 to c4a1cb0 Compare February 8, 2024 12:01
@gharris1727 gharris1727 added streams tests Test fixes (including flaky tests) labels Feb 9, 2024
OmniaGM
OmniaGM reviewed Feb 12, 2024
View reviewed changes
streams/src/test/java/org/apache/kafka/streams/integration/utils/EmbeddedKafkaCluster.java Outdated
log.debug("ZooKeeper instance is running at {}", zKConnectString());
final KafkaClusterTestKit.Builder clusterBuilder = new KafkaClusterTestKit.Builder(
new TestKitNodes.Builder()
.setNumControllerNodes(1)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Any reason why we don't setup the cluster with combined nodes and number of controllers == number of brokers instead of one controller and one broker? Specially that as far as I can see all the test set number of brokers to 1 anyway. This also can get us to remove KafkaEmbedded which might simplify the setup! WDYT?

OmniaGM
OmniaGM reviewed Feb 12, 2024
View reviewed changes
Copy link
Contributor

@OmniaGM OmniaGM left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the pr. I had a quick run through the PR. And I left couple of comments. One other comment is that the java doc in EmbeddedKafkaCluster line 44 need to be updated as it still mention that it start zookeeper.

streams/src/test/java/org/apache/kafka/streams/integration/utils/KafkaEmbedded.java
*/
@SuppressWarnings("WeakerAccess")
public String zookeeperConnect() {
return effectiveConfig.getProperty("zookeeper.connect", DEFAULT_ZK_CONNECT);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we also remove ZkSessionTimeoutMsProp KafkaEmbedded.effectiveConfigFrom line 99 as we don't need it anymore.

@mdedetrich
Copy link
Contributor Author

mdedetrich commented Feb 14, 2024 •
edited
Loading

Thanks for the comments! I just realized that the PR can use additional improvements, i.e. I can get rid of KafkaEmbedded since with KRaft you can set up the entire Kafka cluster (i.e. brokers + controllers) using the KafkaClusterTestKit.Builder.

@OmniaGM
Copy link
Contributor

OmniaGM commented Feb 14, 2024

Thanks for the comments! I just realized that the PR can use additional improvements, i.e. I can get rid of KafkaEmbedded since with KRaft you can set up the entire Kafka cluster (i.e. brokers + controllers) using the KafkaClusterTestKit.Builder.

Thanks for taking the comments into consideration. One other suggestion is that we can have a look into #13375 to see what is the common between both EmbeddedKafkaCluster in stream and connect and see if we can have one common EmbeddedKafkaCluster between both of them. And later move connect to use it. Which also can be follow up JIRA if we want to keep this one simple. Either way I'm happy to lend a hand in the connect part if needed!

@mdedetrich
Copy link
Contributor Author

Thanks for taking the comments into consideration. One other suggestion is that we can have a look into #13375 to see what is the common between both EmbeddedKafkaCluster in stream and connect and see if we can have one common EmbeddedKafkaCluster between both of them. And later move connect to use it. Which also can be follow up JIRA if we want to keep this one simple. Either way I'm happy to lend a hand in the connect part if needed!

Cool I will look into it.

@mdedetrich mdedetrich force-pushed the migrated-streams-tests-from-zk-to-kraft branch 2 times, most recently from 7f21acb to 5afef77 Compare April 18, 2024 07:27
@mdedetrich mdedetrich marked this pull request as draft April 18, 2024 07:28
@mdedetrich mdedetrich force-pushed the migrated-streams-tests-from-zk-to-kraft branch 3 times, most recently from 04a2d50 to aecd641 Compare April 18, 2024 10:44
@mdedetrich mdedetrich force-pushed the migrated-streams-tests-from-zk-to-kraft branch from aecd641 to 3fe0e67 Compare April 18, 2024 11:30
@mdedetrich mdedetrich marked this pull request as ready for review April 18, 2024 11:32
@mdedetrich
Copy link
Contributor Author

@OmniaGM So the structure of the PR/tests is now finalized, I ended up entirely deleting KafkaEmbedded and so now the tests are a lot closer to idioomatic testing with KRaft/KafkaClusterTestKit. Regarding using a global EmbeddedKafkaCluster, I think this makes sense to do in a separate PR or at least when I manage to get the tests to pass in this PR.

On that note, the current issue with the PR is that not all of the tests are passing, i.e. HandlingSourceTopicDeletionIntegrationTest as an example on the top of my head. Currently debugging to figure out why but any help would be appreciated, its likely some config/prop is not properly set

@lucasbru
Copy link
Member

Hey @mdedetrich - are you still working on this PR? We are getting closer to 4.0, so it would be good to finish this.

@OmniaGM
Copy link
Contributor

OmniaGM commented Aug 19, 2024

We have migrated connect testes to KRAFT, can we reuse the new connect's EmbeddedCluster to move stream tests as well? I can help with this PR if no one has time to move it forward.

@lucasbru
Copy link
Member

That would be great @OmniaGM . I'd be happy to help with the PR as well or at least do the reviews. We just need to agree on an owner and avoid duplicated effort. @mdedetrich are you planning to continue on this PR?

@mdedetrich
Copy link
Contributor Author

Hi, no I don't have the capacity to work on this PR anymore. Please take it over

@OmniaGM
Copy link
Contributor

OmniaGM commented Aug 22, 2024

I have some capacity to pickup this one.

@mjsax
Copy link
Member

mjsax commented Aug 25, 2024 •
edited
Loading

I have some capacity to pickup this one.

Nice! This is highly appreciated. Are you planning to re-use this one, or open a new one (for this case, we should just close this one).

@OmniaGM
Copy link
Contributor

OmniaGM commented Aug 26, 2024

I have some capacity to pickup this one.

Nice! This is highly appreciated. Are you planning to re-use this one, or open a new one (for this case, we should just close this one).

I'll open a new PR so we can close this once I open the new one

@lucasbru lucasbru closed this Aug 26, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@OmniaGM OmniaGM OmniaGM left review comments

Assignees
No one assigned
Labels
streams tests Test fixes (including flaky tests)
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@mdedetrich @OmniaGM @lucasbru @mjsax @gharris1727