lucasbru commented on code in PR #15778:
URL: https://github.com/apache/kafka/pull/15778#discussion_r1579343899


##########
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:
   No, lets not remove a variation.
   
   I'm just thinking about the CONSUMER protocol case. If a dynamic client can 
pass this test with `num_revokes_after_bounce = 0`, how meaningful is it really 
to check that static clients have `num_revokes_after_bounce = 0`. I could write 
a consumer that ignores the static membership configuration completely, and 
still pass this test, right?
   
   The test description writes
   ```
   In order to make
           sure the behavior of static members are different from dynamic ones, 
we take both static and dynamic
           membership into this test suite.
   ```
   
   But in the `CONSUMER` protocol, it seems the behavior isn't all that 
difference, at least if we only look at `num_revokes_after_bounce`.
   
   So maybe we should instead try to get the set of partitions and check that 
it didn't change?
   
   Let me know if I'm understanding something wrong. 



-- 
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: jira-unsubscr...@kafka.apache.org

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org

Reply via email to