-
Notifications
You must be signed in to change notification settings - Fork 15.4k
MINOR: Adds KRaft versions of most streams system tests #12458
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
Changes from 4 commits
e308d5d
baee81d
ffefae1
9c00c98
a9c315a
9be74f3
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -18,7 +18,7 @@ | |
| from ducktape.mark.resource import cluster | ||
| from ducktape.mark import matrix | ||
| from ducktape.mark import ignore | ||
| from kafkatest.services.kafka import KafkaService | ||
| from kafkatest.services.kafka import KafkaService, quorum | ||
| from kafkatest.services.zookeeper import ZookeeperService | ||
| from kafkatest.services.streams import StreamsSmokeTestDriverService, StreamsSmokeTestJobRunnerService | ||
| import time | ||
|
|
@@ -67,7 +67,8 @@ def hard_bounce(test, topic, broker_type): | |
| # Since this is a hard kill, we need to make sure the process is down and that | ||
| # zookeeper has registered the loss by expiring the broker's session timeout. | ||
|
|
||
| wait_until(lambda: len(test.kafka.pids(prev_broker_node)) == 0 and not test.kafka.is_registered(prev_broker_node), | ||
| wait_until(lambda: len(test.kafka.pids(prev_broker_node)) == 0 and | ||
| not (quorum.for_test(test.test_context) == quorum.zk and test.kafka.is_registered(prev_broker_node)), | ||
| timeout_sec=test.kafka.zk_session_timeout + 5, | ||
| err_msg="Failed to see timely deregistration of hard-killed broker %s" % str(prev_broker_node.account)) | ||
|
|
||
|
|
@@ -151,10 +152,16 @@ def confirm_topics_on_all_brokers(self, expected_topic_set): | |
|
|
||
| def setup_system(self, start_processor=True, num_threads=3): | ||
| # Setup phase | ||
| self.zk = ZookeeperService(self.test_context, num_nodes=1) | ||
| self.zk.start() | ||
|
|
||
| self.kafka = KafkaService(self.test_context, num_nodes=self.replication, zk=self.zk, topics=self.topics) | ||
| self.zk = ( | ||
| ZookeeperService(self.test_context, 1) | ||
| if quorum.for_test(self.test_context) == quorum.zk | ||
| else None | ||
| ) | ||
| if self.zk: | ||
| self.zk.start() | ||
|
|
||
| self.kafka = KafkaService(self.test_context, num_nodes=self.replication, zk=self.zk, topics=self.topics, | ||
| controller_num_nodes_override=1) | ||
| self.kafka.start() | ||
|
|
||
| # allow some time for topics to be created | ||
|
|
@@ -205,11 +212,17 @@ def collect_results(self, sleep_time_secs): | |
| return data | ||
|
|
||
| @cluster(num_nodes=7) | ||
| @matrix(failure_mode=["clean_shutdown", "hard_shutdown", "clean_bounce", "hard_bounce"], | ||
| broker_type=["leader"], | ||
| num_threads=[1, 3], | ||
| sleep_time_secs=[120], | ||
| metadata_quorum=[quorum.remote_kraft]) | ||
| @matrix(failure_mode=["clean_shutdown", "hard_shutdown", "clean_bounce", "hard_bounce"], | ||
| broker_type=["leader", "controller"], | ||
| num_threads=[1, 3], | ||
| sleep_time_secs=[120]) | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What do you think about explicitly specifying If you're in favor, I'd suggest just removing all the default parameter values that got added here to be sure everything is explicitly configured.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I added this extra matrix configuration to avoid Is that what
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thanks, @AlanConfluent . I was actually making a slightly different suggestion, namely to explicitly include Github charmingly won't let me do a suggestion to make this clearer because the diff includes a deleted line.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I see. Done! |
||
| def test_broker_type_bounce(self, failure_mode, broker_type, sleep_time_secs, num_threads): | ||
| def test_broker_type_bounce(self, failure_mode, broker_type, sleep_time_secs, num_threads, | ||
| metadata_quorum=quorum.zk): | ||
| """ | ||
| Start a smoke test client, then kill one particular broker and ensure data is still received | ||
| Record if records are delivered. | ||
|
|
@@ -251,8 +264,9 @@ 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]) | ||
| def test_many_brokers_bounce(self, failure_mode, num_failures): | ||
| num_failures=[2], | ||
| metadata_quorum=quorum.all_non_upgrade) | ||
| def test_many_brokers_bounce(self, failure_mode, num_failures, metadata_quorum=quorum.zk): | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit: since we already set the value range of
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ah just saw @vvcephei 's comment earlier, that makes sense. Please feel free to ignore.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah, I think if it's not defined, it'll default to using zk, but I just wanted to be a bit more explicit about the default. |
||
| """ | ||
| Start a smoke test client, then kill a few brokers and ensure data is still received | ||
| Record if records are delivered | ||
|
|
@@ -269,8 +283,9 @@ def test_many_brokers_bounce(self, failure_mode, num_failures): | |
|
|
||
| @cluster(num_nodes=7) | ||
| @matrix(failure_mode=["clean_bounce", "hard_bounce"], | ||
| num_failures=[3]) | ||
| def test_all_brokers_bounce(self, failure_mode, num_failures): | ||
| num_failures=[3], | ||
| metadata_quorum=quorum.all_non_upgrade) | ||
| def test_all_brokers_bounce(self, failure_mode, num_failures, metadata_quorum=quorum.zk): | ||
| """ | ||
| Start a smoke test client, then kill a few brokers and ensure data is still received | ||
| Record if records are delivered | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why we only want to test remote_kraft but not collocated kraft?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's what was suggested for tests which are not testing core Kafka functionality and could be affected, which I don't think this is. For those, the predefined variable
quorum.all_non_upgradeexists and is equal to[quorum.remote_kraft, quorum.zk]. Also, given how long these tests run, I didn't want to run more variants than is necessary.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Cool, thanks!