lianetm commented on code in PR #15778:
URL: https://github.com/apache/kafka/pull/15778#discussion_r1578363393
##########
tests/kafkatest/tests/client/consumer_test.py:
##########
@@ -242,16 +242,15 @@ def test_static_consumer_bounce(self, clean_shutdown,
static_membership, bounce_
self.rolling_bounce_consumers(consumer, keep_alive=num_keep_alive,
num_bounces=num_bounces)
num_revokes_after_bounce = consumer.num_revokes_for_alive() -
num_revokes_before_bounce
-
- check_condition = num_revokes_after_bounce != 0
+
# under static membership, the live consumer shall not revoke any
current running partitions,
# since there is no global rebalance being triggered.
if static_membership:
- check_condition = num_revokes_after_bounce == 0
-
- assert check_condition, \
- "Total revoked count %d does not match the expectation of having 0
revokes as %d" % \
- (num_revokes_after_bounce, check_condition)
+ assert num_revokes_after_bounce == 0, \
+ "Unexpected revocation triggered when bouncing static member.
Expecting 0 but had %d revocations" % num_revokes_after_bounce
+ elif consumer.is_eager():
+ assert num_revokes_after_bounce != 0, \
Review Comment:
This test was not testing the cooperative case before because it was using
the base
[setup_consumer](https://github.com/apache/kafka/blob/b8b2415d5e006cf91c0f74dcf60b764933c9c1d0/tests/kafkatest/tests/verifiable_consumer_test.py#L56)
that has a range assignor by default.
We do have a way to detect rebalances (number of calls to partitions
assigned), but that does not allow any special check for dynamic only, since
static members will get those same calls to partitions assigned (the static
membership is only making that the group assignment is not re-computed, so
partition is not re-assigned while the session lasts, expecting that the member
might come back, but from the callbacks POV, the static member that leaves gets
the same as the dynamic one).
I agree that the value of the dynamic + cooperative is questionable, but
there is value in verifying the message delivery semantics further down, when
checking the total consumed. The `test_consumer_bounce` does a very similar
check for dynamic members btw, and again, focusing on the delivery semantics,
but both tests are not exactly the same (mainly keeping 1 node alive or not),
so I would lean towards not removing the combination from this static test,
WDYT?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
