Skip to content

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

Closed
mdedetrich wants to merge 1 commit into
apache:trunkfrom
mdedetrich:migrated-streams-tests-from-zk-to-kraft
Closed

KAFKA-14569: Migrate Kafka Streams tests from Zookeeper to KRaft#15341
mdedetrich wants to merge 1 commit into
apache:trunkfrom
mdedetrich:migrated-streams-tests-from-zk-to-kraft

Conversation

@mdedetrich

@mdedetrich mdedetrich commented Feb 8, 2024

Copy link
Copy Markdown
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
Comment thread 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)

@OmniaGM OmniaGM Feb 12, 2024

Copy link
Copy Markdown
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

@OmniaGM OmniaGM left a comment

Copy link
Copy Markdown
Contributor

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.

Comment thread 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);

@OmniaGM OmniaGM Feb 12, 2024

Copy link
Copy Markdown
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

mdedetrich commented Feb 14, 2024 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

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

OmniaGM commented Feb 14, 2024

Copy link
Copy Markdown
Contributor

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

mdedetrich commented Feb 15, 2024

Copy link
Copy Markdown
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

mdedetrich commented Apr 18, 2024

Copy link
Copy Markdown
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

lucasbru commented Aug 16, 2024

Copy link
Copy Markdown
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

OmniaGM commented Aug 19, 2024

Copy link
Copy Markdown
Contributor

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

lucasbru commented Aug 19, 2024

Copy link
Copy Markdown
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

mdedetrich commented Aug 22, 2024

Copy link
Copy Markdown
Contributor Author

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

@OmniaGM

OmniaGM commented Aug 22, 2024

Copy link
Copy Markdown
Contributor

I have some capacity to pickup this one.

@mjsax

mjsax commented Aug 25, 2024 •
edited
Loading

Copy link
Copy Markdown
Member

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

OmniaGM commented Aug 26, 2024

Copy link
Copy Markdown
Contributor

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