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.

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

KAFKA-17609:[1/4] Changes needed to convert system tests to use KRaft and remove ZK #17275

Merged
merged 15 commits into from
Nov 5, 2024
Merged

KAFKA-17609:[1/4] Changes needed to convert system tests to use KRaft and remove ZK #17275

merged 15 commits into from
Nov 5, 2024

Conversation

bbejeck
Copy link
Contributor

@bbejeck bbejeck commented Sep 25, 2024 •
edited
Loading

This is part one of a multi-pr effort to convert Kafka Streams system tests to KRaft. I decided to break down the changes into multiple PRs to reduce the review load

Committer Checklist (excluded from commit message)

  • Verify design and implementation
  • Verify test coverage and CI build status
  • Verify documentation (including upgrade notes)

@github-actions github-actions bot added the tests Test fixes (including flaky tests) label Sep 25, 2024
@mjsax
Copy link
Member

mjsax commented Sep 26, 2024

Thanks Bill. Made a pass. Can you also trigger a system test run and share the link?

@bbejeck bbejeck force-pushed the KAFKA-17609_convert_system_tests_to_kraft_part_1 branch from b5df169 to ec294cf Compare September 30, 2024 15:05
@bbejeck bbejeck changed the title KAFKA-17609: Changes needed to convert system tests to use KRaft and remove ZK KAFKA-17609:[1/4] Changes needed to convert system tests to use KRaft and remove ZK Sep 30, 2024
@bbejeck
Copy link
Contributor Author

bbejeck commented Sep 30, 2024

Thanks Bill. Made a pass. Can you also trigger a system test run and share the link?

Passing system tests for ApplicationUpgradeTest
Passing system tests for BrokerBounceTest

Also in the PR description

@bbejeck
Copy link
Contributor Author

bbejeck commented Oct 1, 2024

@mjsax all comments addressed

bbejeck
bbejeck commented Oct 1, 2024
View reviewed changes
tests/setup.py Outdated Show resolved Hide resolved
@bbejeck
Copy link
Contributor Author

bbejeck commented Oct 1, 2024

I kicked off a new branch builder for streams_application_upgrade_test and I'll post the link here when it completes.

@bbejeck
Copy link
Contributor Author

bbejeck commented Oct 1, 2024

System test results 29 passed, 3 failed - I'll look into the failures.

@bbejeck bbejeck force-pushed the KAFKA-17609_convert_system_tests_to_kraft_part_1 branch from c7c36fb to 38d44da Compare October 16, 2024 17:30
@github-actions github-actions bot added the small Small PRs label Oct 16, 2024
mjsax
mjsax reviewed Oct 16, 2024
View reviewed changes
tests/kafkatest/tests/streams/streams_application_upgrade_test.py Show resolved Hide resolved
tests/kafkatest/tests/streams/streams_application_upgrade_test.py Show resolved Hide resolved
tests/kafkatest/tests/streams/streams_broker_bounce_test.py Outdated Show resolved Hide resolved
tests/kafkatest/tests/streams/streams_broker_bounce_test.py Outdated Show resolved Hide resolved
tests/kafkatest/tests/streams/streams_broker_bounce_test.py
@@ -265,8 +253,8 @@ def test_broker_type_bounce_at_start(self, failure_mode, broker_type, sleep_time
@cluster(num_nodes=7)
@matrix(failure_mode=["clean_shutdown", "hard_shutdown", "clean_bounce", "hard_bounce"],
num_failures=[2],
metadata_quorum=quorum.all_non_upgrade)
def test_many_brokers_bounce(self, failure_mode, num_failures, metadata_quorum=quorum.zk):
metadata_quorum=[quorum.isolated_kraft])
Copy link
Member

Choose a reason for hiding this comment

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

as above

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Required to indicate to use KRaft for metadata quorum. Failing to do so without a @matrix annotation results in this error as the test will default to ZK without it.

    zk_sasl=self.zk.zk_sasl if self.quorum_info.using_zk else False, zk_tls=self.zk_client_secure,
AttributeError: 'NoneType' object has no attribute 'zk_sasl'

Copy link
Contributor Author

@bbejeck bbejeck Nov 5, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

update: some of these tests were failing as previously configured so I needed to update to isolated_kraft and increase the number of nodes.

many_brokers_bounce_failures_part_1

tests/kafkatest/tests/streams/streams_broker_bounce_test.py
@@ -284,8 +272,8 @@ def test_many_brokers_bounce(self, failure_mode, num_failures, metadata_quorum=q
@cluster(num_nodes=7)
@matrix(failure_mode=["clean_bounce", "hard_bounce"],
num_failures=[3],
metadata_quorum=quorum.all_non_upgrade)
def test_all_brokers_bounce(self, failure_mode, num_failures, metadata_quorum=quorum.zk):
metadata_quorum=[quorum.isolated_kraft])
Copy link
Member

Choose a reason for hiding this comment

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

as above

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Required to indicate to use KRaft for metadata quorum. Failing to do so without a @matrix annotation results in this error as the test will default to ZK without it.

    zk_sasl=self.zk.zk_sasl if self.quorum_info.using_zk else False, zk_tls=self.zk_client_secure,
AttributeError: 'NoneType' object has no attribute 'zk_sasl'

Copy link
Contributor Author

@bbejeck bbejeck Nov 5, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

update: some of these tests were failing as previously configured so I needed to update to isolated_kraft and increase the number of nodes.

test_all_brokers_bounce_failure

@bbejeck bbejeck force-pushed the KAFKA-17609_convert_system_tests_to_kraft_part_1 branch from 38d44da to 3351d6b Compare October 22, 2024 01:26
mjsax
mjsax approved these changes Oct 22, 2024
View reviewed changes
Copy link
Member

@mjsax mjsax left a comment

Choose a reason for hiding this comment

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

LGTM

@bbejeck bbejeck force-pushed the KAFKA-17609_convert_system_tests_to_kraft_part_1 branch from 3351d6b to 6d489f5 Compare October 28, 2024 19:37
@github-actions github-actions bot added the tools label Oct 28, 2024
bbejeck
bbejeck commented Oct 28, 2024
View reviewed changes
Vagrantfile Outdated Show resolved Hide resolved
bbejeck
bbejeck commented Oct 28, 2024
View reviewed changes
vagrant/base.sh Outdated Show resolved Hide resolved
@bbejeck bbejeck force-pushed the KAFKA-17609_convert_system_tests_to_kraft_part_1 branch from 07058ea to 8f8a159 Compare November 1, 2024 19:11
@bbejeck bbejeck force-pushed the KAFKA-17609_convert_system_tests_to_kraft_part_1 branch from 8f8a159 to d81fcfc Compare November 4, 2024 20:33
@bbejeck
Copy link
Contributor Author

bbejeck commented Nov 5, 2024

Link to all passing tests.

zk_to_kraft_migration_pr_1_results

@bbejeck bbejeck merged commit 36c131e into apache:trunk Nov 5, 2024
4 of 5 checks passed
@bbejeck
Copy link
Contributor Author

bbejeck commented Nov 5, 2024 •
edited
Loading

Merged #17275 into trunk

@bbejeck bbejeck deleted the KAFKA-17609_convert_system_tests_to_kraft_part_1 branch November 5, 2024 16:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mjsax mjsax mjsax approved these changes

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

Successfully merging this pull request may close these issues.
2 participants
@bbejeck @mjsax