[GitHub] [kafka] AlanConfluent commented on a diff in pull request #12458: MINOR: Adds KRaft versions of most streams system tests

[GitBox <gi...@apache.org>]

AlanConfluent commented on code in PR #12458:
URL: https://github.com/apache/kafka/pull/12458#discussion_r933614497


##########
tests/kafkatest/tests/streams/streams_standby_replica_test.py:
##########
@@ -73,9 +77,9 @@ def test_standby_tasks_rebalance(self):
 
         processor_3.start()
 
-        self.wait_for_verification(processor_1, "ACTIVE_TASKS:2 STANDBY_TASKS:[1-3]", processor_1.STDOUT_FILE)
-        self.wait_for_verification(processor_2, "ACTIVE_TASKS:2 STANDBY_TASKS:[1-3]", processor_2.STDOUT_FILE)
-        self.wait_for_verification(processor_3, "ACTIVE_TASKS:2 STANDBY_TASKS:[1-3]", processor_3.STDOUT_FILE)
+        self.wait_for_verification(processor_1, "ACTIVE_TASKS:2 STANDBY_TASKS:2", processor_1.STDOUT_FILE)
+        self.wait_for_verification(processor_2, "ACTIVE_TASKS:2 STANDBY_TASKS:2", processor_2.STDOUT_FILE)
+        self.wait_for_verification(processor_3, "ACTIVE_TASKS:2 STANDBY_TASKS:2", processor_3.STDOUT_FILE)

Review Comment:
   Ahh, I know how this change got made.  I had made my changes in another repo locally and copied the file over to this one...  Must have been made by another commit in the original repo.  Will revert this and make sure there are no similar changes elsewhere.



##########
tests/kafkatest/tests/streams/streams_broker_bounce_test.py:
##########
@@ -205,11 +211,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])

Review Comment:
   I added this extra matrix configuration to avoid `broker_type=="controller"` for `metadata_quorum=="quorum.remote_kraft"`-- not sure if you noticed that, otherwise I would have just added `metadata_quorum=quorum.all_non_upgrade` to the existing matrix to cover both modes as you said.  This is because killing the controller when it's separate from the broker is maybe not what the test is attempting to test and also requires other changes to accommodate killing off the separate KRaft controller node, but that seems a bit overkill for this purpose.
   
   Is that what



-- 
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-unsubscribe@kafka.apache.org

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