-
Notifications
You must be signed in to change notification settings - Fork 13.9k
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
KAFKA-17459: Stablize reassign_partitions_test.py #17250
Conversation
95b0014
to
f761342
Compare
Test result on my local machine (branch 3.9)
|
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.
@frankvicky thanks for this fix. Could you please test this patch for 3.8 as we will have 3.8.1 release
@@ -176,7 +176,7 @@ def test_reassign_partitions(self, bounce_brokers, reassign_from_offset_zero, me | |||
self.producer = VerifiableProducer(self.test_context, self.num_producers, | |||
self.kafka, self.topic, | |||
throughput=self.producer_throughput, | |||
enable_idempotence=True) | |||
enable_idempotence=True, repeating_keys=100) |
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.
Could you please add comments?
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.
Done. 😺
Hello @chia7712
|
enable_idempotence=True) | ||
enable_idempotence=True, | ||
# To avoid the reassignment behavior being affected by `partitioner.ignore.keys=true`, | ||
# we set a key for the message to ensure the message order is preserved within the partition. |
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.
We need this change due to the test behavior, so it would be helpful to emphasize that point.
This test expects that each partition can receive the record, so using a non-null key helps distribute the records more randomly.
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.
Fair enough, I will add some context.
enable_idempotence=True) | ||
enable_idempotence=True, | ||
# This test aims to verify the reassignment without failure, assuming that all partitions have data. | ||
# To avoid the reassignment behavior being affected by `partitioner.ignore.keys=true`, |
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.
This description is not accurate as the default value of partitioner.ignore.keys
is false
, and the root cause is that the BuiltInPartitioner
tries to send requests to the fast brokers.
Could you please keep the description "This test aims to verify the reassignment without failure, assuming that all partitions have data" and "we set a key for the message to ensure both even data distribution across all partitions"
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.
It seems that I have some misunderstanding before.
I have updated the comment, please take a look 😺
This test expects that each partition can receive the record, so using a non-null key helps distribute the records more randomly. Reviewers: Chia-Ping Tsai <[email protected]>
This test expects that each partition can receive the record, so using a non-null key helps distribute the records more randomly. Reviewers: Chia-Ping Tsai <[email protected]>
JIRA: KAFKA-17459
Committer Checklist (excluded from commit message)