You are viewing a plain text version of this content. The canonical link for it is here.
Posted to jira@kafka.apache.org by "rreddy-22 (via GitHub)" <gi...@apache.org> on 2023/10/11 01:03:39 UTC
[PR] System Tests for KIP-848 [kafka]
rreddy-22 opened a new pull request, #14524:
URL: https://github.com/apache/kafka/pull/14524
Added configs and custom decorators to facilitate the testing of the old protocol with the new group coordinator (KIP-848) in kraft mode.
The new coordinator doesn't support zookeeper mode and hence, this combination will be skipped.
**Files Changed:**
1) kafka.py : Added an argument called use_new_coordinator to the kafka service which can be set in the matrix of every test. The default value for this config will be set as false.
2) util.py : Added a custom decorator that will skip the test for zk=true and use_new_coordinator=true since this isn't supported.
--
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
Re: [PR] KAFKA-15578: System Tests for running old protocol with new coordinator [kafka]
Posted by "rreddy-22 (via GitHub)" <gi...@apache.org>.
rreddy-22 commented on PR #14524:
URL: https://github.com/apache/kafka/pull/14524#issuecomment-1778311241
https://jenkins.confluent.io/job/system-test-kafka-branch-builder/5907/ - Latest test build
--
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
Re: [PR] KAFKA-15578: System Tests for running old protocol with new coordinator [kafka]
Posted by "dajac (via GitHub)" <gi...@apache.org>.
dajac commented on code in PR #14524:
URL: https://github.com/apache/kafka/pull/14524#discussion_r1371289551
##########
tests/kafkatest/services/kafka/kafka.py:
##########
@@ -277,6 +279,22 @@ def __init__(self, context, num_nodes, zk, security_protocol=SecurityConfig.PLAI
self.isolated_controller_quorum = None # will define below if necessary
self.configured_for_zk_migration = False
+ default_use_new_coordinator = False
+
+ # If 'use_new_coordinator' is not explicitly set, determine it based on context.
+ if use_new_coordinator is None:
+ arg_name = 'use_new_coordinator'
+
+ # Default to the global setting if no arguments are injected.
+ if not context.injected_args:
Review Comment:
How about?
```
if use_new_coordinator is None:
use_new_coordinator = context.globals.get(`use_new_coordinator`)
if context.injected_args is not None:
use_new_coordinator = context.injected_args.get(`use_new_coordinator`, use_new_coordinator)
```
--
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
Re: [PR] System Tests for KIP-848 [kafka]
Posted by "rreddy-22 (via GitHub)" <gi...@apache.org>.
rreddy-22 commented on code in PR #14524:
URL: https://github.com/apache/kafka/pull/14524#discussion_r1355388919
##########
tests/kafkatest/tests/core/consume_bench_test.py:
##########
@@ -114,7 +126,11 @@ def test_single_partition(self, metadata_quorum=quorum.zk):
self.logger.info("TASKS: %s\n" % json.dumps(tasks, sort_keys=True, indent=2))
@cluster(num_nodes=10)
- @matrix(metadata_quorum=quorum.all_non_upgrade)
+ @matrix(
+ metadata_quorum=quorum.all_non_upgrade,
+ se_new_coordinator=[True, False]
+ )
+ @skip_if_new_coordinator_and_zk
def test_multiple_consumers_random_group_topics(self, metadata_quorum=quorum.zk):
Review Comment:
no, sry again the changes got missed
--
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
Re: [PR] KAFKA-15578: System Tests for running old protocol with new coordinator [kafka]
Posted by "dajac (via GitHub)" <gi...@apache.org>.
dajac commented on code in PR #14524:
URL: https://github.com/apache/kafka/pull/14524#discussion_r1364174604
##########
tests/kafkatest/services/kafka/templates/log4j.properties:
##########
@@ -136,3 +136,6 @@ log4j.additivity.state.change.logger=false
log4j.logger.kafka.authorizer.logger={{ log_level|default("DEBUG") }}, authorizerInfoAppender, authorizerDebugAppender
log4j.additivity.kafka.authorizer.logger=false
+#New Group Coordinator logging.
+log4j.logger.org.apache.kafka.coordinator.group={{ log_level|default("DEBUG") }}, kafkaDebugAppender
+log4j.additivity.kafka.coordinator.group=false
Review Comment:
You also need to change this one.
##########
tests/kafkatest/services/kafka/templates/log4j.properties:
##########
@@ -136,3 +136,6 @@ log4j.additivity.state.change.logger=false
log4j.logger.kafka.authorizer.logger={{ log_level|default("DEBUG") }}, authorizerInfoAppender, authorizerDebugAppender
log4j.additivity.kafka.authorizer.logger=false
+#New Group Coordinator logging.
+log4j.logger.org.apache.kafka.coordinator.group={{ log_level|default("DEBUG") }}, kafkaDebugAppender
Review Comment:
You must use `kafkaInfoAppender, kafkaDebugAppender` here.
--
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
Re: [PR] KAFKA-15578: System Tests for running old protocol with new coordinator [kafka]
Posted by "rreddy-22 (via GitHub)" <gi...@apache.org>.
rreddy-22 commented on code in PR #14524:
URL: https://github.com/apache/kafka/pull/14524#discussion_r1372147456
##########
tests/kafkatest/services/kafka/kafka.py:
##########
@@ -277,6 +279,22 @@ def __init__(self, context, num_nodes, zk, security_protocol=SecurityConfig.PLAI
self.isolated_controller_quorum = None # will define below if necessary
self.configured_for_zk_migration = False
+ default_use_new_coordinator = False
+
+ # If 'use_new_coordinator' is not explicitly set, determine it based on context.
+ if use_new_coordinator is None:
+ arg_name = 'use_new_coordinator'
+
+ # Default to the global setting if no arguments are injected.
+ if not context.injected_args:
Review Comment:
I asked Ian in the office yesterday, he said the current implementation should work
--
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
Re: [PR] KAFKA-15578: System Tests for running old protocol with new coordinator [kafka]
Posted by "rreddy-22 (via GitHub)" <gi...@apache.org>.
rreddy-22 commented on code in PR #14524:
URL: https://github.com/apache/kafka/pull/14524#discussion_r1372148618
##########
tests/kafkatest/services/kafka/kafka.py:
##########
@@ -277,6 +279,22 @@ def __init__(self, context, num_nodes, zk, security_protocol=SecurityConfig.PLAI
self.isolated_controller_quorum = None # will define below if necessary
self.configured_for_zk_migration = False
+ default_use_new_coordinator = False
+
+ # If 'use_new_coordinator' is not explicitly set, determine it based on context.
+ if use_new_coordinator is None:
+ arg_name = 'use_new_coordinator'
+
+ # Default to the global setting if no arguments are injected.
+ if not context.injected_args:
Review Comment:
The code you provided would first take the arg value from globals and then from context, we want the first priority to be from context args and then global and if nothing is provided then we use the default value
--
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
Re: [PR] KAFKA-15578: System Tests for running old protocol with new coordinator [kafka]
Posted by "rreddy-22 (via GitHub)" <gi...@apache.org>.
rreddy-22 commented on code in PR #14524:
URL: https://github.com/apache/kafka/pull/14524#discussion_r1372148618
##########
tests/kafkatest/services/kafka/kafka.py:
##########
@@ -277,6 +279,22 @@ def __init__(self, context, num_nodes, zk, security_protocol=SecurityConfig.PLAI
self.isolated_controller_quorum = None # will define below if necessary
self.configured_for_zk_migration = False
+ default_use_new_coordinator = False
+
+ # If 'use_new_coordinator' is not explicitly set, determine it based on context.
+ if use_new_coordinator is None:
+ arg_name = 'use_new_coordinator'
+
+ # Default to the global setting if no arguments are injected.
+ if not context.injected_args:
Review Comment:
The code you provided would first take the arg value from globals and then from context, we want the first priority to be from context args and then global and if nothing is provided then we use the default value
--
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
Re: [PR] KAFKA-15578: System Tests for running old protocol with new coordinator [kafka]
Posted by "imcdo (via GitHub)" <gi...@apache.org>.
imcdo commented on code in PR #14524:
URL: https://github.com/apache/kafka/pull/14524#discussion_r1355617795
##########
tests/kafkatest/tests/core/consume_bench_test.py:
##########
@@ -68,12 +67,21 @@ def produce_messages(self, topics, max_messages=10000):
self.logger.debug("Produce workload finished")
@cluster(num_nodes=10)
- @matrix(topics=[["consume_bench_topic[0-5]"]], metadata_quorum=quorum.all_non_upgrade) # topic subscription
- @matrix(topics=[["consume_bench_topic[0-5]:[0-4]"]], metadata_quorum=quorum.all_non_upgrade) # manual topic assignment
- def test_consume_bench(self, topics, metadata_quorum=quorum.zk):
+ @matrix(
+ topics=[
+ ["consume_bench_topic[0-5]"],
+ ["consume_bench_topic[0-5]:[0-4]"]
+ ],
+ metadata_quorum=quorum.all_non_upgrade,
+ use_new_coordinator=[True, False]
+ ) # topic subscription
Review Comment:
I think the biggest object to that proposition i have is it isn't sustainable to any other kraft specific configs that might have to be added in the future.
--
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
Re: [PR] KAFKA-15578: System Tests for running old protocol with new coordinator [kafka]
Posted by "imcdo (via GitHub)" <gi...@apache.org>.
imcdo commented on code in PR #14524:
URL: https://github.com/apache/kafka/pull/14524#discussion_r1357170513
##########
tests/kafkatest/tests/core/consume_bench_test.py:
##########
@@ -68,12 +67,21 @@ def produce_messages(self, topics, max_messages=10000):
self.logger.debug("Produce workload finished")
@cluster(num_nodes=10)
- @matrix(topics=[["consume_bench_topic[0-5]"]], metadata_quorum=quorum.all_non_upgrade) # topic subscription
- @matrix(topics=[["consume_bench_topic[0-5]:[0-4]"]], metadata_quorum=quorum.all_non_upgrade) # manual topic assignment
- def test_consume_bench(self, topics, metadata_quorum=quorum.zk):
+ @matrix(
+ topics=[
+ ["consume_bench_topic[0-5]"],
+ ["consume_bench_topic[0-5]:[0-4]"]
+ ],
+ metadata_quorum=quorum.all_non_upgrade,
+ use_new_coordinator=[True, False]
+ ) # topic subscription
Review Comment:
Though no strong opinion either way. Just throwing in my 2 cents.
--
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
Re: [PR] KAFKA-15578: System Tests for running old protocol with new coordinator [kafka]
Posted by "dajac (via GitHub)" <gi...@apache.org>.
dajac commented on code in PR #14524:
URL: https://github.com/apache/kafka/pull/14524#discussion_r1356467053
##########
tests/kafkatest/tests/core/consume_bench_test.py:
##########
@@ -68,12 +67,28 @@ def produce_messages(self, topics, max_messages=10000):
self.logger.debug("Produce workload finished")
@cluster(num_nodes=10)
- @matrix(topics=[["consume_bench_topic[0-5]"]], metadata_quorum=quorum.all_non_upgrade) # topic subscription
- @matrix(topics=[["consume_bench_topic[0-5]:[0-4]"]], metadata_quorum=quorum.all_non_upgrade) # manual topic assignment
- def test_consume_bench(self, topics, metadata_quorum=quorum.zk):
+ @matrix(
+ topics=[
+ ["consume_bench_topic[0-5]"],
+ ["consume_bench_topic[0-5]:[0-4]"]
Review Comment:
Let's keep the comments for each of those.
--
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
Re: [PR] System Tests for KIP-848 [kafka]
Posted by "dajac (via GitHub)" <gi...@apache.org>.
dajac commented on code in PR #14524:
URL: https://github.com/apache/kafka/pull/14524#discussion_r1354878772
##########
tests/kafkatest/services/kafka/kafka.py:
##########
@@ -764,6 +769,9 @@ def prop_file(self, node):
else:
override_configs[config_property.ZOOKEEPER_SSL_CLIENT_ENABLE] = 'false'
+ if self.use_new_coordinator:
+ override_configs[config_property.NEW_GROUP_COORDINATOR_ENABLE] = 'true'
Review Comment:
I don't see where we update the kafka config based on this flag. Is it missing?
##########
tests/kafkatest/tests/core/consume_bench_test.py:
##########
@@ -114,7 +126,11 @@ def test_single_partition(self, metadata_quorum=quorum.zk):
self.logger.info("TASKS: %s\n" % json.dumps(tasks, sort_keys=True, indent=2))
@cluster(num_nodes=10)
- @matrix(metadata_quorum=quorum.all_non_upgrade)
+ @matrix(
+ metadata_quorum=quorum.all_non_upgrade,
+ se_new_coordinator=[True, False]
+ )
+ @skip_if_new_coordinator_and_zk
def test_multiple_consumers_random_group_topics(self, metadata_quorum=quorum.zk):
Review Comment:
Is having `use_new_coordinator` as a param optional?
##########
tests/kafkatest/tests/core/consume_bench_test.py:
##########
@@ -68,12 +67,21 @@ def produce_messages(self, topics, max_messages=10000):
self.logger.debug("Produce workload finished")
@cluster(num_nodes=10)
- @matrix(topics=[["consume_bench_topic[0-5]"]], metadata_quorum=quorum.all_non_upgrade) # topic subscription
- @matrix(topics=[["consume_bench_topic[0-5]:[0-4]"]], metadata_quorum=quorum.all_non_upgrade) # manual topic assignment
- def test_consume_bench(self, topics, metadata_quorum=quorum.zk):
+ @matrix(
+ topics=[
+ ["consume_bench_topic[0-5]"],
+ ["consume_bench_topic[0-5]:[0-4]"]
+ ],
Review Comment:
Is this equivalent to the previous one? Let's keep the comments.
##########
tests/kafkatest/tests/core/consume_bench_test.py:
##########
@@ -68,12 +67,21 @@ def produce_messages(self, topics, max_messages=10000):
self.logger.debug("Produce workload finished")
@cluster(num_nodes=10)
- @matrix(topics=[["consume_bench_topic[0-5]"]], metadata_quorum=quorum.all_non_upgrade) # topic subscription
- @matrix(topics=[["consume_bench_topic[0-5]:[0-4]"]], metadata_quorum=quorum.all_non_upgrade) # manual topic assignment
- def test_consume_bench(self, topics, metadata_quorum=quorum.zk):
+ @matrix(
+ topics=[
+ ["consume_bench_topic[0-5]"],
+ ["consume_bench_topic[0-5]:[0-4]"]
+ ],
+ metadata_quorum=quorum.all_non_upgrade,
+ use_new_coordinator=[True, False]
+ ) # topic subscription
+ @skip_if_new_coordinator_and_zk
+ def test_consume_bench(self, topics, metadata_quorum=quorum.zk, use_new_coordinator=False):
"""
Runs a ConsumeBench workload to consume messages
"""
+ self.kafka.use_new_coordinator = use_new_coordinator
Review Comment:
I see that you only do this here, why that?
##########
tests/kafkatest/tests/core/consume_bench_test.py:
##########
@@ -114,7 +126,11 @@ def test_single_partition(self, metadata_quorum=quorum.zk):
self.logger.info("TASKS: %s\n" % json.dumps(tasks, sort_keys=True, indent=2))
@cluster(num_nodes=10)
- @matrix(metadata_quorum=quorum.all_non_upgrade)
+ @matrix(
+ metadata_quorum=quorum.all_non_upgrade,
+ se_new_coordinator=[True, False]
Review Comment:
typo. same for the others.
--
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
Re: [PR] KAFKA-15578: System Tests for running old protocol with new coordinator [kafka]
Posted by "dajac (via GitHub)" <gi...@apache.org>.
dajac commented on code in PR #14524:
URL: https://github.com/apache/kafka/pull/14524#discussion_r1356464517
##########
tests/kafkatest/tests/core/consume_bench_test.py:
##########
@@ -68,12 +67,21 @@ def produce_messages(self, topics, max_messages=10000):
self.logger.debug("Produce workload finished")
@cluster(num_nodes=10)
- @matrix(topics=[["consume_bench_topic[0-5]"]], metadata_quorum=quorum.all_non_upgrade) # topic subscription
- @matrix(topics=[["consume_bench_topic[0-5]:[0-4]"]], metadata_quorum=quorum.all_non_upgrade) # manual topic assignment
- def test_consume_bench(self, topics, metadata_quorum=quorum.zk):
+ @matrix(
+ topics=[
+ ["consume_bench_topic[0-5]"],
+ ["consume_bench_topic[0-5]:[0-4]"]
+ ],
+ metadata_quorum=quorum.all_non_upgrade,
+ use_new_coordinator=[True, False]
+ ) # topic subscription
Review Comment:
Yeah, I agree that relying on the `quorum.all_non_upgrade` is not perfect. The configuration that we are talking about here is temporary and will be removed in AK 4.0 (~Q2 24). At this time, the old group coordinator will be removed and KRaft will run by default with the new one. At the same time, we will remove ZK as well. Therefore, this flag won't be needed at all in a couple of months. We also want all the KRaft tests to run with the new group coordinator until we reach this goal.
@imcdo Knowing this, does it change your point of view or do you still believe that it is not good at all?
--
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
Re: [PR] KAFKA-15578: System Tests for running old protocol with new coordinator [kafka]
Posted by "rreddy-22 (via GitHub)" <gi...@apache.org>.
rreddy-22 commented on code in PR #14524:
URL: https://github.com/apache/kafka/pull/14524#discussion_r1357170133
##########
tests/kafkatest/services/kafka/config_property.py:
##########
@@ -199,6 +200,9 @@
val SSLEndpointIdentificationAlgorithmProp = SSLConfigs.SSL_ENDPOINT_IDENTIFICATION_ALGORITHM_CONFIG
val SSLSecureRandomImplementationProp = SSLConfigs.SSL_SECURE_RANDOM_IMPLEMENTATION_CONFIG
val SSLClientAuthProp = SSLConfigs.SSL_CLIENT_AUTH_CONFIG
+
+ /** New group coordinator configs */
+ val NewGroupCoordinatorEnableProp = "group.coordinator.new.enable"
Review Comment:
This javadoc is just to mention what is present in KafkaConfig.scala, this is the string used there so I copied the same name here. They did the same thing for the rest of the properties too. I don't think the name has to match but the string does.
--
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
Re: [PR] KAFKA-15578: System Tests for running old protocol with new coordinator [kafka]
Posted by "rreddy-22 (via GitHub)" <gi...@apache.org>.
rreddy-22 commented on code in PR #14524:
URL: https://github.com/apache/kafka/pull/14524#discussion_r1355653216
##########
tests/kafkatest/services/kafka/kafka.py:
##########
@@ -764,6 +769,9 @@ def prop_file(self, node):
else:
override_configs[config_property.ZOOKEEPER_SSL_CLIENT_ENABLE] = 'false'
+ if self.use_new_coordinator:
+ override_configs[config_property.NEW_GROUP_COORDINATOR_ENABLE] = 'true'
Review Comment:
I guess it doesn't matter but let me look into it
--
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
Re: [PR] System Tests for KIP-848 [kafka]
Posted by "rreddy-22 (via GitHub)" <gi...@apache.org>.
rreddy-22 commented on code in PR #14524:
URL: https://github.com/apache/kafka/pull/14524#discussion_r1355362411
##########
tests/kafkatest/tests/core/consume_bench_test.py:
##########
@@ -68,12 +67,21 @@ def produce_messages(self, topics, max_messages=10000):
self.logger.debug("Produce workload finished")
@cluster(num_nodes=10)
- @matrix(topics=[["consume_bench_topic[0-5]"]], metadata_quorum=quorum.all_non_upgrade) # topic subscription
- @matrix(topics=[["consume_bench_topic[0-5]:[0-4]"]], metadata_quorum=quorum.all_non_upgrade) # manual topic assignment
- def test_consume_bench(self, topics, metadata_quorum=quorum.zk):
+ @matrix(
+ topics=[
+ ["consume_bench_topic[0-5]"],
+ ["consume_bench_topic[0-5]:[0-4]"]
+ ],
Review Comment:
yep its the same
--
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
Re: [PR] KAFKA-15578: System Tests for running old protocol with new coordinator [kafka]
Posted by "dajac (via GitHub)" <gi...@apache.org>.
dajac commented on code in PR #14524:
URL: https://github.com/apache/kafka/pull/14524#discussion_r1373513993
##########
tests/kafkatest/services/kafka/kafka.py:
##########
@@ -277,6 +279,22 @@ def __init__(self, context, num_nodes, zk, security_protocol=SecurityConfig.PLAI
self.isolated_controller_quorum = None # will define below if necessary
self.configured_for_zk_migration = False
+ default_use_new_coordinator = False
+
+ # If 'use_new_coordinator' is not explicitly set, determine it based on context.
+ if use_new_coordinator is None:
+ arg_name = 'use_new_coordinator'
+
+ # Default to the global setting if no arguments are injected.
+ if not context.injected_args:
Review Comment:
Right. But the code does not work like this, isn't it? if `injected_args` is present but `use_new_coordinator` is not in it, for whatever reason (say another feature), it ignores the globals, right?
--
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
Re: [PR] KAFKA-15578: System Tests for running old protocol with new coordinator [kafka]
Posted by "rreddy-22 (via GitHub)" <gi...@apache.org>.
rreddy-22 commented on code in PR #14524:
URL: https://github.com/apache/kafka/pull/14524#discussion_r1373681142
##########
tests/kafkatest/services/kafka/kafka.py:
##########
@@ -277,6 +279,22 @@ def __init__(self, context, num_nodes, zk, security_protocol=SecurityConfig.PLAI
self.isolated_controller_quorum = None # will define below if necessary
self.configured_for_zk_migration = False
+ default_use_new_coordinator = False
+
+ # If 'use_new_coordinator' is not explicitly set, determine it based on context.
+ if use_new_coordinator is None:
+ arg_name = 'use_new_coordinator'
+
+ # Default to the global setting if no arguments are injected.
+ if not context.injected_args:
Review Comment:
```
if use_new_coordinator is None:
use_new_coordinator = context.globals.get(`use_new_coordinator`)
if context.injected_args is not None:
use_new_coordinator = context.injected_args.get(`use_new_coordinator`, use_new_coordinator)
```
This would've overwritten the global value with the default in case injected args didn't have the value specified. I changed it to something different now. This could've also allowed use_new_coordinator to be None in case the global value wasn't set and context args was None.
--
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
Re: [PR] KAFKA-15578: System Tests for running old protocol with new coordinator [kafka]
Posted by "rreddy-22 (via GitHub)" <gi...@apache.org>.
rreddy-22 commented on code in PR #14524:
URL: https://github.com/apache/kafka/pull/14524#discussion_r1372146226
##########
tests/kafkatest/services/kafka/kafka.py:
##########
@@ -277,6 +279,22 @@ def __init__(self, context, num_nodes, zk, security_protocol=SecurityConfig.PLAI
self.isolated_controller_quorum = None # will define below if necessary
self.configured_for_zk_migration = False
+ default_use_new_coordinator = False
+
+ # If 'use_new_coordinator' is not explicitly set, determine it based on context.
+ if use_new_coordinator is None:
+ arg_name = 'use_new_coordinator'
+
+ # Default to the global setting if no arguments are injected.
+ if not context.injected_args:
Review Comment:
> What would happen if context.injected_args is defined but not for use_new_coordinator
--
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
Re: [PR] KAFKA-15578: System Tests for running old protocol with new coordinator [kafka]
Posted by "dajac (via GitHub)" <gi...@apache.org>.
dajac commented on code in PR #14524:
URL: https://github.com/apache/kafka/pull/14524#discussion_r1371278858
##########
tests/kafkatest/services/kafka/kafka.py:
##########
@@ -277,6 +279,22 @@ def __init__(self, context, num_nodes, zk, security_protocol=SecurityConfig.PLAI
self.isolated_controller_quorum = None # will define below if necessary
self.configured_for_zk_migration = False
+ default_use_new_coordinator = False
+
+ # If 'use_new_coordinator' is not explicitly set, determine it based on context.
+ if use_new_coordinator is None:
+ arg_name = 'use_new_coordinator'
+
+ # Default to the global setting if no arguments are injected.
+ if not context.injected_args:
Review Comment:
@imcdo Could you advice on how to get this right?
--
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
Re: [PR] KAFKA-15578: System Tests for running old protocol with new coordinator [kafka]
Posted by "dajac (via GitHub)" <gi...@apache.org>.
dajac commented on code in PR #14524:
URL: https://github.com/apache/kafka/pull/14524#discussion_r1355512521
##########
tests/kafkatest/tests/core/consume_bench_test.py:
##########
@@ -68,12 +67,21 @@ def produce_messages(self, topics, max_messages=10000):
self.logger.debug("Produce workload finished")
@cluster(num_nodes=10)
- @matrix(topics=[["consume_bench_topic[0-5]"]], metadata_quorum=quorum.all_non_upgrade) # topic subscription
- @matrix(topics=[["consume_bench_topic[0-5]:[0-4]"]], metadata_quorum=quorum.all_non_upgrade) # manual topic assignment
- def test_consume_bench(self, topics, metadata_quorum=quorum.zk):
+ @matrix(
+ topics=[
+ ["consume_bench_topic[0-5]"],
+ ["consume_bench_topic[0-5]:[0-4]"]
+ ],
+ metadata_quorum=quorum.all_non_upgrade,
+ use_new_coordinator=[True, False]
+ ) # topic subscription
Review Comment:
I am +1 for not polluting the report. However, it is a bit annoying that we have to change all the tests... I was considering whether we should just add an extra "profile" to `quorum.all_non_upgrade` instead. Is there any downside to this approach?
--
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
Re: [PR] KAFKA-15578: System Tests for running old protocol with new coordinator [kafka]
Posted by "imcdo (via GitHub)" <gi...@apache.org>.
imcdo commented on code in PR #14524:
URL: https://github.com/apache/kafka/pull/14524#discussion_r1355750615
##########
tests/kafkatest/tests/core/consume_bench_test.py:
##########
@@ -44,7 +44,6 @@ def setUp(self):
self.trogdor.start()
if self.zk:
self.zk.start()
- self.kafka.start()
Review Comment:
this is kinda a limitation of ducktape, where the setup call doesn't directly get any of the parameters of the test, meaning we can't know for sure here directly what the args are via parameters. However how metadata team around this was pulling the args directly from the test context (which we could do here). I feel like its a bit less hacky to just move the start call as setup isn't all that useful imo (you can write your own setup with the args you want and call it at the start of test to achieve a similar result). but either way will work.
--
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
Re: [PR] KAFKA-15578: System Tests for running old protocol with new coordinator [kafka]
Posted by "rreddy-22 (via GitHub)" <gi...@apache.org>.
rreddy-22 commented on PR #14524:
URL: https://github.com/apache/kafka/pull/14524#issuecomment-1781535440
https://jenkins.confluent.io/job/system-test-kafka-branch-builder/5918/ - after rebase, consume_bench_test run
--
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
Re: [PR] System Tests for KIP-848 [kafka]
Posted by "rreddy-22 (via GitHub)" <gi...@apache.org>.
rreddy-22 commented on code in PR #14524:
URL: https://github.com/apache/kafka/pull/14524#discussion_r1355360984
##########
tests/kafkatest/services/kafka/kafka.py:
##########
@@ -764,6 +769,9 @@ def prop_file(self, node):
else:
override_configs[config_property.ZOOKEEPER_SSL_CLIENT_ENABLE] = 'false'
+ if self.use_new_coordinator:
+ override_configs[config_property.NEW_GROUP_COORDINATOR_ENABLE] = 'true'
Review Comment:
didn't get committed the first time, it's there yep
--
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
Re: [PR] KAFKA-15578: System Tests for running old protocol with new coordinator [kafka]
Posted by "imcdo (via GitHub)" <gi...@apache.org>.
imcdo commented on code in PR #14524:
URL: https://github.com/apache/kafka/pull/14524#discussion_r1355617795
##########
tests/kafkatest/tests/core/consume_bench_test.py:
##########
@@ -68,12 +67,21 @@ def produce_messages(self, topics, max_messages=10000):
self.logger.debug("Produce workload finished")
@cluster(num_nodes=10)
- @matrix(topics=[["consume_bench_topic[0-5]"]], metadata_quorum=quorum.all_non_upgrade) # topic subscription
- @matrix(topics=[["consume_bench_topic[0-5]:[0-4]"]], metadata_quorum=quorum.all_non_upgrade) # manual topic assignment
- def test_consume_bench(self, topics, metadata_quorum=quorum.zk):
+ @matrix(
+ topics=[
+ ["consume_bench_topic[0-5]"],
+ ["consume_bench_topic[0-5]:[0-4]"]
+ ],
+ metadata_quorum=quorum.all_non_upgrade,
+ use_new_coordinator=[True, False]
+ ) # topic subscription
Review Comment:
I think the biggest object to that proposition i have is it isn't sustainable to any other kraft only configs that might have to be added in the future.
--
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
Re: [PR] KAFKA-15578: System Tests for running old protocol with new coordinator [kafka]
Posted by "dajac (via GitHub)" <gi...@apache.org>.
dajac commented on code in PR #14524:
URL: https://github.com/apache/kafka/pull/14524#discussion_r1356465862
##########
tests/kafkatest/services/kafka/util.py:
##########
@@ -12,9 +12,11 @@
# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
# See the License for the specific language governing permissions and
# limitations under the License.
-
from collections import namedtuple
+from ducktape.tests.test import TestContext
Review Comment:
Let's revert all changes in this file.
--
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
Re: [PR] KAFKA-15578: System Tests for running old protocol with new coordinator [kafka]
Posted by "rreddy-22 (via GitHub)" <gi...@apache.org>.
rreddy-22 commented on code in PR #14524:
URL: https://github.com/apache/kafka/pull/14524#discussion_r1370533091
##########
tests/kafkatest/services/kafka/kafka.py:
##########
@@ -277,6 +279,22 @@ def __init__(self, context, num_nodes, zk, security_protocol=SecurityConfig.PLAI
self.isolated_controller_quorum = None # will define below if necessary
self.configured_for_zk_migration = False
+ default_use_new_coordinator = False
+
+ # If 'use_new_coordinator' is not explicitly set, determine it based on context.
+ if use_new_coordinator is None:
+ arg_name = 'use_new_coordinator'
+
+ # Default to the global setting if no arguments are injected.
+ if not context.injected_args:
Review Comment:
This is what I had before but this was giving a ERROR: `NoneType' object has no attribute 'get` on using get when context args is None
--
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
Re: [PR] KAFKA-15578: System Tests for running old protocol with new coordinator [kafka]
Posted by "rreddy-22 (via GitHub)" <gi...@apache.org>.
rreddy-22 commented on code in PR #14524:
URL: https://github.com/apache/kafka/pull/14524#discussion_r1372148618
##########
tests/kafkatest/services/kafka/kafka.py:
##########
@@ -277,6 +279,22 @@ def __init__(self, context, num_nodes, zk, security_protocol=SecurityConfig.PLAI
self.isolated_controller_quorum = None # will define below if necessary
self.configured_for_zk_migration = False
+ default_use_new_coordinator = False
+
+ # If 'use_new_coordinator' is not explicitly set, determine it based on context.
+ if use_new_coordinator is None:
+ arg_name = 'use_new_coordinator'
+
+ # Default to the global setting if no arguments are injected.
+ if not context.injected_args:
Review Comment:
The code you provided would first take from globals and then from context, we want the first priority to be from context args and then global and if nothing is provided then we use the default value
--
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
Re: [PR] KAFKA-15578: System Tests for running old protocol with new coordinator [kafka]
Posted by "rreddy-22 (via GitHub)" <gi...@apache.org>.
rreddy-22 commented on code in PR #14524:
URL: https://github.com/apache/kafka/pull/14524#discussion_r1370533091
##########
tests/kafkatest/services/kafka/kafka.py:
##########
@@ -277,6 +279,22 @@ def __init__(self, context, num_nodes, zk, security_protocol=SecurityConfig.PLAI
self.isolated_controller_quorum = None # will define below if necessary
self.configured_for_zk_migration = False
+ default_use_new_coordinator = False
+
+ # If 'use_new_coordinator' is not explicitly set, determine it based on context.
+ if use_new_coordinator is None:
+ arg_name = 'use_new_coordinator'
+
+ # Default to the global setting if no arguments are injected.
+ if not context.injected_args:
Review Comment:
This is what I had before but this was giving a ERROR: `NoneType' object has no attribute 'get` on using get when use_new_coordinator isn't in the context args
--
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
Re: [PR] KAFKA-15578: System Tests for running old protocol with new coordinator [kafka]
Posted by "rreddy-22 (via GitHub)" <gi...@apache.org>.
rreddy-22 commented on code in PR #14524:
URL: https://github.com/apache/kafka/pull/14524#discussion_r1372146226
##########
tests/kafkatest/services/kafka/kafka.py:
##########
@@ -277,6 +279,22 @@ def __init__(self, context, num_nodes, zk, security_protocol=SecurityConfig.PLAI
self.isolated_controller_quorum = None # will define below if necessary
self.configured_for_zk_migration = False
+ default_use_new_coordinator = False
+
+ # If 'use_new_coordinator' is not explicitly set, determine it based on context.
+ if use_new_coordinator is None:
+ arg_name = 'use_new_coordinator'
+
+ # Default to the global setting if no arguments are injected.
+ if not context.injected_args:
Review Comment:
> What would happen if context.injected_args is defined but not for use_new_coordinator
If get doesn't find the arg name then it uses the default value I provided as the second argument
--
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
Re: [PR] KAFKA-15578: System Tests for running old protocol with new coordinator [kafka]
Posted by "dajac (via GitHub)" <gi...@apache.org>.
dajac commented on code in PR #14524:
URL: https://github.com/apache/kafka/pull/14524#discussion_r1373214870
##########
tests/kafkatest/services/kafka/kafka.py:
##########
@@ -277,6 +279,22 @@ def __init__(self, context, num_nodes, zk, security_protocol=SecurityConfig.PLAI
self.isolated_controller_quorum = None # will define below if necessary
self.configured_for_zk_migration = False
+ default_use_new_coordinator = False
+
+ # If 'use_new_coordinator' is not explicitly set, determine it based on context.
+ if use_new_coordinator is None:
+ arg_name = 'use_new_coordinator'
+
+ # Default to the global setting if no arguments are injected.
+ if not context.injected_args:
Review Comment:
Hum... I suppose that I miss some understanding about how `injected_args` works. My understanding here is that whenever `injected_args` is defined, we take the value for `use_new_coordinator` from it or default to `default_use_new_coordinator`. This means that we basically ignore the global setting in this case.
What is the precedence of those here? value from the constructor, value from `injected_args` and value from `globals`?
--
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
Re: [PR] KAFKA-15578: System Tests for running old protocol with new coordinator [kafka]
Posted by "rreddy-22 (via GitHub)" <gi...@apache.org>.
rreddy-22 commented on code in PR #14524:
URL: https://github.com/apache/kafka/pull/14524#discussion_r1373502458
##########
tests/kafkatest/services/kafka/kafka.py:
##########
@@ -277,6 +279,22 @@ def __init__(self, context, num_nodes, zk, security_protocol=SecurityConfig.PLAI
self.isolated_controller_quorum = None # will define below if necessary
self.configured_for_zk_migration = False
+ default_use_new_coordinator = False
+
+ # If 'use_new_coordinator' is not explicitly set, determine it based on context.
+ if use_new_coordinator is None:
+ arg_name = 'use_new_coordinator'
+
+ # Default to the global setting if no arguments are injected.
+ if not context.injected_args:
Review Comment:
Injected args > globals > default where injected is in the args in each test, globals can be set at run time with ducktape arguments in the command line and default is the value we provided in this section of the code
--
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
Re: [PR] KAFKA-15578: System Tests for running old protocol with new coordinator [kafka]
Posted by "dajac (via GitHub)" <gi...@apache.org>.
dajac commented on code in PR #14524:
URL: https://github.com/apache/kafka/pull/14524#discussion_r1356465229
##########
tests/kafkatest/services/kafka/config_property.py:
##########
@@ -199,6 +200,9 @@
val SSLEndpointIdentificationAlgorithmProp = SSLConfigs.SSL_ENDPOINT_IDENTIFICATION_ALGORITHM_CONFIG
val SSLSecureRandomImplementationProp = SSLConfigs.SSL_SECURE_RANDOM_IMPLEMENTATION_CONFIG
val SSLClientAuthProp = SSLConfigs.SSL_CLIENT_AUTH_CONFIG
+
+ /** New group coordinator configs */
+ val NewGroupCoordinatorEnableProp = "group.coordinator.new.enable"
Review Comment:
nit: Could we reuse NEW_GROUP_COORDINATOR_ENABLE here?
--
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
Re: [PR] KAFKA-15578: System Tests for running old protocol with new coordinator [kafka]
Posted by "rreddy-22 (via GitHub)" <gi...@apache.org>.
rreddy-22 commented on code in PR #14524:
URL: https://github.com/apache/kafka/pull/14524#discussion_r1355666083
##########
tests/kafkatest/tests/core/consume_bench_test.py:
##########
@@ -68,12 +67,21 @@ def produce_messages(self, topics, max_messages=10000):
self.logger.debug("Produce workload finished")
@cluster(num_nodes=10)
- @matrix(topics=[["consume_bench_topic[0-5]"]], metadata_quorum=quorum.all_non_upgrade) # topic subscription
- @matrix(topics=[["consume_bench_topic[0-5]:[0-4]"]], metadata_quorum=quorum.all_non_upgrade) # manual topic assignment
- def test_consume_bench(self, topics, metadata_quorum=quorum.zk):
+ @matrix(
+ topics=[
+ ["consume_bench_topic[0-5]"],
+ ["consume_bench_topic[0-5]:[0-4]"]
+ ],
+ metadata_quorum=quorum.all_non_upgrade,
+ use_new_coordinator=[True, False]
+ ) # topic subscription
Review Comment:
I also agree with Ian, I would definitely not want to tie these two together, just makes everything more confusing and messy, especially since this is temporary, we'd have to carefully decouple it again in a couple months. We can change it to a global config once we stop supporting zk.
--
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
Re: [PR] KAFKA-15578: System Tests for running old protocol with new coordinator [kafka]
Posted by "rreddy-22 (via GitHub)" <gi...@apache.org>.
rreddy-22 commented on code in PR #14524:
URL: https://github.com/apache/kafka/pull/14524#discussion_r1355663593
##########
tests/kafkatest/tests/core/consume_bench_test.py:
##########
@@ -68,12 +67,21 @@ def produce_messages(self, topics, max_messages=10000):
self.logger.debug("Produce workload finished")
@cluster(num_nodes=10)
- @matrix(topics=[["consume_bench_topic[0-5]"]], metadata_quorum=quorum.all_non_upgrade) # topic subscription
- @matrix(topics=[["consume_bench_topic[0-5]:[0-4]"]], metadata_quorum=quorum.all_non_upgrade) # manual topic assignment
- def test_consume_bench(self, topics, metadata_quorum=quorum.zk):
+ @matrix(
+ topics=[
+ ["consume_bench_topic[0-5]"],
+ ["consume_bench_topic[0-5]:[0-4]"]
+ ],
+ metadata_quorum=quorum.all_non_upgrade,
+ use_new_coordinator=[True, False]
+ ) # topic subscription
Review Comment:
Oh I wasn't aware that Skips show up in the report, I thought it'd just actually skip the combination. I'll change it to what you suggested and remove the custom decorator.
--
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
Re: [PR] System Tests for KIP-848 [kafka]
Posted by "rreddy-22 (via GitHub)" <gi...@apache.org>.
rreddy-22 commented on code in PR #14524:
URL: https://github.com/apache/kafka/pull/14524#discussion_r1355363738
##########
tests/kafkatest/tests/core/consume_bench_test.py:
##########
@@ -68,12 +67,21 @@ def produce_messages(self, topics, max_messages=10000):
self.logger.debug("Produce workload finished")
@cluster(num_nodes=10)
- @matrix(topics=[["consume_bench_topic[0-5]"]], metadata_quorum=quorum.all_non_upgrade) # topic subscription
- @matrix(topics=[["consume_bench_topic[0-5]:[0-4]"]], metadata_quorum=quorum.all_non_upgrade) # manual topic assignment
- def test_consume_bench(self, topics, metadata_quorum=quorum.zk):
+ @matrix(
+ topics=[
+ ["consume_bench_topic[0-5]"],
+ ["consume_bench_topic[0-5]:[0-4]"]
+ ],
+ metadata_quorum=quorum.all_non_upgrade,
+ use_new_coordinator=[True, False]
+ ) # topic subscription
+ @skip_if_new_coordinator_and_zk
+ def test_consume_bench(self, topics, metadata_quorum=quorum.zk, use_new_coordinator=False):
"""
Runs a ConsumeBench workload to consume messages
"""
+ self.kafka.use_new_coordinator = use_new_coordinator
Review Comment:
sry the last commit had these
--
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
Re: [PR] KAFKA-15578: System Tests for running old protocol with new coordinator [kafka]
Posted by "imcdo (via GitHub)" <gi...@apache.org>.
imcdo commented on code in PR #14524:
URL: https://github.com/apache/kafka/pull/14524#discussion_r1355442947
##########
tests/kafkatest/services/kafka/kafka.py:
##########
@@ -277,6 +278,9 @@ def __init__(self, context, num_nodes, zk, security_protocol=SecurityConfig.PLAI
self.isolated_controller_quorum = None # will define below if necessary
self.configured_for_zk_migration = False
+ if use_new_coordinator is None :
+ self.use_new_coordinator = self.context.globals.get["use_new_coordinator", False]
Review Comment:
get is a method, use ()
```suggestion
self.use_new_coordinator = self.context.globals.get("use_new_coordinator", False)
```
##########
tests/kafkatest/services/kafka/config.py:
##########
@@ -28,7 +28,8 @@ class KafkaConfig(dict):
config_property.METADATA_LOG_SEGMENT_BYTES: str(9*1024*1024), # 9 MB
config_property.METADATA_LOG_BYTES_BETWEEN_SNAPSHOTS: str(10*1024*1024), # 10 MB
config_property.METADATA_LOG_RETENTION_BYTES: str(10*1024*1024), # 10 MB
- config_property.METADATA_LOG_SEGMENT_MS: str(1*60*1000) # one minute
+ config_property.METADATA_LOG_SEGMENT_MS: str(1*60*1000), # one minute
+ config_property.NEW_GROUP_COORDINATOR_ENABLE: False
Review Comment:
Nit: is there a better name than new group coordinator? Not very descriptive and could easily become the old group coordinator if we arn't careful.
##########
tests/kafkatest/services/kafka/kafka.py:
##########
@@ -407,6 +411,7 @@ def __init__(self, context, num_nodes, zk, security_protocol=SecurityConfig.PLAI
kraft_broker_configs = {
config_property.PORT: config_property.FIRST_BROKER_PORT,
config_property.NODE_ID: self.idx(node),
+ config_property.NEW_GROUP_COORDINATOR_ENABLE: use_new_coordinator
}
Review Comment:
dont we want to conditionally add this when `use_new_coordinator` is true?
```suggestion
kraft_broker_configs = {
config_property.PORT: config_property.FIRST_BROKER_PORT,
config_property.NODE_ID: self.idx(node)
}
if self.use_new_coordinator:
kraft_broker_configs[config_property.NEW_GROUP_COORDINATOR_ENABLE] = use_new_coordinator
}
```
that way we stick with the default value? Or do we care? Either was is fine.
##########
tests/kafkatest/tests/core/consume_bench_test.py:
##########
@@ -91,11 +99,17 @@ def test_consume_bench(self, topics, metadata_quorum=quorum.zk):
self.logger.info("TASKS: %s\n" % json.dumps(tasks, sort_keys=True, indent=2))
@cluster(num_nodes=10)
- @matrix(metadata_quorum=quorum.all_non_upgrade)
- def test_single_partition(self, metadata_quorum=quorum.zk):
+ @matrix(
+ metadata_quorum=quorum.all_non_upgrade,
+ use_new_coordinator=[True, False]
+ )
Review Comment:
same applies here:
```suggestion
@matrix(
metadata_quorum=[quorum.isolated_kraft],
use_new_coordinator=[True, False]
)
@matrix(
metadata_quorum=[quorum.zk],
use_new_coordinator=[False]
)
```
There are some alternitives, one could be your `skip_if_new_coordinator_and_zk` wrapper could go into the method metadata, and remove the testcases with use_new_coordinator=True and metadata_quorum=zk, though id advise against it as its not that simple.
##########
tests/kafkatest/services/kafka/kafka.py:
##########
@@ -277,6 +278,9 @@ def __init__(self, context, num_nodes, zk, security_protocol=SecurityConfig.PLAI
self.isolated_controller_quorum = None # will define below if necessary
Review Comment:
make sure you add a description of the param you are adding to the docstring for the __init__ method
##########
tests/kafkatest/tests/core/consume_bench_test.py:
##########
@@ -68,12 +67,21 @@ def produce_messages(self, topics, max_messages=10000):
self.logger.debug("Produce workload finished")
@cluster(num_nodes=10)
- @matrix(topics=[["consume_bench_topic[0-5]"]], metadata_quorum=quorum.all_non_upgrade) # topic subscription
- @matrix(topics=[["consume_bench_topic[0-5]:[0-4]"]], metadata_quorum=quorum.all_non_upgrade) # manual topic assignment
- def test_consume_bench(self, topics, metadata_quorum=quorum.zk):
+ @matrix(
+ topics=[
+ ["consume_bench_topic[0-5]"],
+ ["consume_bench_topic[0-5]:[0-4]"]
+ ],
+ metadata_quorum=quorum.all_non_upgrade,
+ use_new_coordinator=[True, False]
+ ) # topic subscription
Review Comment:
You probably shouldn't skip zk + new coordinator test, just dont run it in the first place. We dont really want to polute the report with a bunch of skips (which are normally reserved for broken test that we intend to fix later historicly).
```suggestion
@matrix(
topics=[
["consume_bench_topic[0-5]"],
["consume_bench_topic[0-5]:[0-4]"]
],
metadata_quorum=metadata_quorum=[quorum.isolated_kraft],
use_new_coordinator=[True, False]
)
@matrix(
topics=[
["consume_bench_topic[0-5]"],
["consume_bench_topic[0-5]:[0-4]"]
],
metadata_quorum=[quorum.zk],
use_new_coordinator=[False]
) # topic subscription
```
as annoying as that is, prob for the best.
##########
tests/kafkatest/services/kafka/kafka.py:
##########
@@ -764,6 +769,9 @@ def prop_file(self, node):
else:
override_configs[config_property.ZOOKEEPER_SSL_CLIENT_ENABLE] = 'false'
+ if self.use_new_coordinator:
+ override_configs[config_property.NEW_GROUP_COORDINATOR_ENABLE] = 'true'
Review Comment:
should we limit this to only kraft brokers instead of all brokers or does it not matter?
##########
tests/kafkatest/tests/core/consume_bench_test.py:
##########
@@ -188,12 +220,18 @@ def test_multiple_consumers_random_group_partitions(self, metadata_quorum=quorum
self.logger.info("TASKS: %s\n" % json.dumps(tasks, sort_keys=True, indent=2))
@cluster(num_nodes=10)
- @matrix(metadata_quorum=quorum.all_non_upgrade)
- def test_multiple_consumers_specified_group_partitions_should_raise(self, metadata_quorum=quorum.zk):
+ @matrix(
+ metadata_quorum=quorum.all_non_upgrade,
+ use_new_coordinator=[True, False]
+ )
+ @skip_if_new_coordinator_and_zk
+ def test_multiple_consumers_specified_group_partitions_should_raise(self, metadata_quorum=quorum.zk, use_new_coordinator=False):
"""
Runs multiple consumers in the same group to read messages from specific partitions.
It is an invalid configuration to provide a consumer group and specific partitions.
"""
+ self.kafka.use_new_coordinator = use_new_coordinator
Review Comment:
make sure in your testing this actually works by sshing onto all the brokers to see if its set properly. The way its configured now, at `kafka.py` construction, the kraft brokers configs sets this to False, then at runtime, the override sets it to `use_new_coordinator` so it should work.
--
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
Re: [PR] KAFKA-15578: System Tests for running old protocol with new coordinator [kafka]
Posted by "imcdo (via GitHub)" <gi...@apache.org>.
imcdo commented on code in PR #14524:
URL: https://github.com/apache/kafka/pull/14524#discussion_r1355435960
##########
tests/kafkatest/services/kafka/config.py:
##########
@@ -28,7 +28,8 @@ class KafkaConfig(dict):
config_property.METADATA_LOG_SEGMENT_BYTES: str(9*1024*1024), # 9 MB
config_property.METADATA_LOG_BYTES_BETWEEN_SNAPSHOTS: str(10*1024*1024), # 10 MB
config_property.METADATA_LOG_RETENTION_BYTES: str(10*1024*1024), # 10 MB
- config_property.METADATA_LOG_SEGMENT_MS: str(1*60*1000) # one minute
+ config_property.METADATA_LOG_SEGMENT_MS: str(1*60*1000), # one minute
+ config_property.NEW_GROUP_COORDINATOR_ENABLE: False
Review Comment:
Nit: is there a better name than new group coordinator? Not very descriptive and could easily become the old group coordinator if we arn't careful.
--
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
Re: [PR] KAFKA-15578: System Tests for running old protocol with new coordinator [kafka]
Posted by "rreddy-22 (via GitHub)" <gi...@apache.org>.
rreddy-22 commented on PR #14524:
URL: https://github.com/apache/kafka/pull/14524#issuecomment-1769219146
https://jenkins.confluent.io/job/system-test-kafka-branch-builder/5894/
--
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
Re: [PR] KAFKA-15578: System Tests for running old protocol with new coordinator [kafka]
Posted by "dajac (via GitHub)" <gi...@apache.org>.
dajac commented on code in PR #14524:
URL: https://github.com/apache/kafka/pull/14524#discussion_r1368566885
##########
tests/kafkatest/services/verifiable_consumer.py:
##########
@@ -168,7 +168,7 @@ class VerifiableConsumer(KafkaPathResolverMixin, VerifiableClientMixin, Backgrou
def __init__(self, context, num_nodes, kafka, topic, group_id,
static_membership=False, max_messages=-1, session_timeout_sec=30, enable_autocommit=False,
assignment_strategy=None,
- version=DEV_BRANCH, stop_timeout_sec=30, log_level="INFO", jaas_override_variables=None,
+ version=DEV_BRANCH, stop_timeout_sec=30, log_level="DEBUG", jaas_override_variables=None,
Review Comment:
Should we revert this?
--
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
Re: [PR] KAFKA-15578: System Tests for running old protocol with new coordinator [kafka]
Posted by "rreddy-22 (via GitHub)" <gi...@apache.org>.
rreddy-22 commented on code in PR #14524:
URL: https://github.com/apache/kafka/pull/14524#discussion_r1373672235
##########
tests/kafkatest/services/kafka/kafka.py:
##########
@@ -277,6 +279,22 @@ def __init__(self, context, num_nodes, zk, security_protocol=SecurityConfig.PLAI
self.isolated_controller_quorum = None # will define below if necessary
self.configured_for_zk_migration = False
+ default_use_new_coordinator = False
+
+ # If 'use_new_coordinator' is not explicitly set, determine it based on context.
+ if use_new_coordinator is None:
+ arg_name = 'use_new_coordinator'
+
+ # Default to the global setting if no arguments are injected.
+ if not context.injected_args:
Review Comment:
yep you're right, I changed it now. I misunderstood what you meant earlier.
--
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
Re: [PR] KAFKA-15578: System Tests for running old protocol with new coordinator [kafka]
Posted by "rreddy-22 (via GitHub)" <gi...@apache.org>.
rreddy-22 commented on code in PR #14524:
URL: https://github.com/apache/kafka/pull/14524#discussion_r1372146226
##########
tests/kafkatest/services/kafka/kafka.py:
##########
@@ -277,6 +279,22 @@ def __init__(self, context, num_nodes, zk, security_protocol=SecurityConfig.PLAI
self.isolated_controller_quorum = None # will define below if necessary
self.configured_for_zk_migration = False
+ default_use_new_coordinator = False
+
+ # If 'use_new_coordinator' is not explicitly set, determine it based on context.
+ if use_new_coordinator is None:
+ arg_name = 'use_new_coordinator'
+
+ # Default to the global setting if no arguments are injected.
+ if not context.injected_args:
Review Comment:
> What would happen if context.injected_args is defined but not for use_new_coordinator
If get doesn't find the arg name then it uses the default value I provided as the second argument
--
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
Re: [PR] KAFKA-15578: System Tests for running old protocol with new coordinator [kafka]
Posted by "rreddy-22 (via GitHub)" <gi...@apache.org>.
rreddy-22 commented on PR #14524:
URL: https://github.com/apache/kafka/pull/14524#issuecomment-1783215920
https://jenkins.confluent.io/job/system-test-kafka-branch-builder/5925/ - Passing test after changes
--
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
Re: [PR] KAFKA-15578: System Tests for running old protocol with new coordinator [kafka]
Posted by "dajac (via GitHub)" <gi...@apache.org>.
dajac merged PR #14524:
URL: https://github.com/apache/kafka/pull/14524
--
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
Re: [PR] KAFKA-15578: System Tests for running old protocol with new coordinator [kafka]
Posted by "dajac (via GitHub)" <gi...@apache.org>.
dajac commented on code in PR #14524:
URL: https://github.com/apache/kafka/pull/14524#discussion_r1356466186
##########
tests/kafkatest/tests/core/consume_bench_test.py:
##########
@@ -23,14 +23,14 @@
from kafkatest.services.trogdor.task_spec import TaskSpec
from kafkatest.services.trogdor.trogdor import TrogdorService
from kafkatest.services.zookeeper import ZookeeperService
-
+from kafkatest.services.kafka.util import skip_if_new_coordinator_and_zk
class ConsumeBenchTest(Test):
def __init__(self, test_context):
""":type test_context: ducktape.tests.test.TestContext"""
super(ConsumeBenchTest, self).__init__(test_context)
self.zk = ZookeeperService(test_context, num_nodes=3) if quorum.for_test(test_context) == quorum.zk else None
- self.kafka = KafkaService(test_context, num_nodes=3, zk=self.zk)
+ self.kafka = KafkaService(test_context, num_nodes=3, zk=self.zk,)
Review Comment:
Do we need that extra `,`?
--
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
Re: [PR] System Tests for KIP-848 [kafka]
Posted by "dajac (via GitHub)" <gi...@apache.org>.
dajac commented on code in PR #14524:
URL: https://github.com/apache/kafka/pull/14524#discussion_r1355468634
##########
tests/kafkatest/tests/core/consume_bench_test.py:
##########
@@ -44,7 +44,6 @@ def setUp(self):
self.trogdor.start()
if self.zk:
self.zk.start()
- self.kafka.start()
Review Comment:
Is there a way to avoid having to remove this? It seems that we were able to do something for the `metadata_quorum` param.
--
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
Re: [PR] KAFKA-15578: System Tests for running old protocol with new coordinator [kafka]
Posted by "rreddy-22 (via GitHub)" <gi...@apache.org>.
rreddy-22 commented on code in PR #14524:
URL: https://github.com/apache/kafka/pull/14524#discussion_r1355652198
##########
tests/kafkatest/services/kafka/kafka.py:
##########
@@ -407,6 +411,7 @@ def __init__(self, context, num_nodes, zk, security_protocol=SecurityConfig.PLAI
kraft_broker_configs = {
config_property.PORT: config_property.FIRST_BROKER_PORT,
config_property.NODE_ID: self.idx(node),
+ config_property.NEW_GROUP_COORDINATOR_ENABLE: use_new_coordinator
}
Review Comment:
I figured the value get's overwritten anyways on start and here it's gonna be false at init by default anyways so just assigned it until the overwrite happens if its set to true
--
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
Re: [PR] KAFKA-15578: System Tests for running old protocol with new coordinator [kafka]
Posted by "dajac (via GitHub)" <gi...@apache.org>.
dajac commented on code in PR #14524:
URL: https://github.com/apache/kafka/pull/14524#discussion_r1356456410
##########
tests/kafkatest/tests/core/consume_bench_test.py:
##########
@@ -44,7 +44,6 @@ def setUp(self):
self.trogdor.start()
if self.zk:
self.zk.start()
- self.kafka.start()
Review Comment:
I would prefer to not change this because we would need to change it in all the tests.
--
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
Re: [PR] KAFKA-15578: System Tests for running old protocol with new coordinator [kafka]
Posted by "imcdo (via GitHub)" <gi...@apache.org>.
imcdo commented on code in PR #14524:
URL: https://github.com/apache/kafka/pull/14524#discussion_r1357169852
##########
tests/kafkatest/tests/core/consume_bench_test.py:
##########
@@ -68,12 +67,21 @@ def produce_messages(self, topics, max_messages=10000):
self.logger.debug("Produce workload finished")
@cluster(num_nodes=10)
- @matrix(topics=[["consume_bench_topic[0-5]"]], metadata_quorum=quorum.all_non_upgrade) # topic subscription
- @matrix(topics=[["consume_bench_topic[0-5]:[0-4]"]], metadata_quorum=quorum.all_non_upgrade) # manual topic assignment
- def test_consume_bench(self, topics, metadata_quorum=quorum.zk):
+ @matrix(
+ topics=[
+ ["consume_bench_topic[0-5]"],
+ ["consume_bench_topic[0-5]:[0-4]"]
+ ],
+ metadata_quorum=quorum.all_non_upgrade,
+ use_new_coordinator=[True, False]
+ ) # topic subscription
Review Comment:
I think if its only temporary its probably best as suggested as it would be easiest to role back and has the least complexity. Yeah its a bit repetitive, but it wont get in the way of anyone trying to add configs in the next 2.5 quarters. If we need a full test analysis of the feature, we can use the ducktape globals, and just live with the zk test results. (and just not run them nightly, just when we want to validate).
--
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
Re: [PR] KAFKA-15578: System Tests for running old protocol with new coordinator [kafka]
Posted by "imcdo (via GitHub)" <gi...@apache.org>.
imcdo commented on code in PR #14524:
URL: https://github.com/apache/kafka/pull/14524#discussion_r1355605756
##########
tests/kafkatest/tests/core/consume_bench_test.py:
##########
@@ -68,12 +67,21 @@ def produce_messages(self, topics, max_messages=10000):
self.logger.debug("Produce workload finished")
@cluster(num_nodes=10)
- @matrix(topics=[["consume_bench_topic[0-5]"]], metadata_quorum=quorum.all_non_upgrade) # topic subscription
- @matrix(topics=[["consume_bench_topic[0-5]:[0-4]"]], metadata_quorum=quorum.all_non_upgrade) # manual topic assignment
- def test_consume_bench(self, topics, metadata_quorum=quorum.zk):
+ @matrix(
+ topics=[
+ ["consume_bench_topic[0-5]"],
+ ["consume_bench_topic[0-5]:[0-4]"]
+ ],
+ metadata_quorum=quorum.all_non_upgrade,
+ use_new_coordinator=[True, False]
+ ) # topic subscription
Review Comment:
id argue we shouldn't tie configurations to eachother. if you wanted to do that, then that would effect a lot of tests immediately. That would be the easiest way to do that however now the configs are tied to eachother and you'd have to idividually support each permutation of using the config (for instance now you want to add colocated craft tests with new coordinator, or any other flag in the future that will be kraft exclusive which i imagine might be many).
If you want to run with this globaly (all tests) Id argue the correct approach is to just use the `session_context.globals` (the issue is that kraft doesn't use this so kinda just stuck between a rock and a hard place, unless we run with the skip descriptor that is already there on all tests as well).
--
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
Re: [PR] KAFKA-15578: System Tests for running old protocol with new coordinator [kafka]
Posted by "dajac (via GitHub)" <gi...@apache.org>.
dajac commented on code in PR #14524:
URL: https://github.com/apache/kafka/pull/14524#discussion_r1365682473
##########
tests/kafkatest/services/kafka/templates/log4j.properties:
##########
@@ -136,3 +136,6 @@ log4j.additivity.state.change.logger=false
log4j.logger.kafka.authorizer.logger={{ log_level|default("DEBUG") }}, authorizerInfoAppender, authorizerDebugAppender
log4j.additivity.kafka.authorizer.logger=false
+#New Group Coordinator logging.
+log4j.logger.org.apache.kafka.coordinator.group={{ log_level|default("DEBUG") }}, kafkaInfoAppender, kafkaDebugAppender
+log4j.additivity.kafka.coordinator.group=false
Review Comment:
Shouldn't we use `org.apache.kafka.coordinator.group` here as well?
--
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
Re: [PR] KAFKA-15578: System Tests for running old protocol with new coordinator [kafka]
Posted by "rreddy-22 (via GitHub)" <gi...@apache.org>.
rreddy-22 commented on PR #14524:
URL: https://github.com/apache/kafka/pull/14524#issuecomment-1769217647
https://jenkins.confluent.io/job/system-test-kafka-branch-builder/5882/
--
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
Re: [PR] KAFKA-15578: System Tests for running old protocol with new coordinator [kafka]
Posted by "dajac (via GitHub)" <gi...@apache.org>.
dajac commented on code in PR #14524:
URL: https://github.com/apache/kafka/pull/14524#discussion_r1370316861
##########
tests/kafkatest/services/kafka/kafka.py:
##########
@@ -277,6 +279,22 @@ def __init__(self, context, num_nodes, zk, security_protocol=SecurityConfig.PLAI
self.isolated_controller_quorum = None # will define below if necessary
self.configured_for_zk_migration = False
+ default_use_new_coordinator = False
+
+ # If 'use_new_coordinator' is not explicitly set, determine it based on context.
+ if use_new_coordinator is None:
+ arg_name = 'use_new_coordinator'
+
+ # Default to the global setting if no arguments are injected.
+ if not context.injected_args:
Review Comment:
Are you sure about this change? What would happen if `context.injected_args` is defined but not for `use_new_coordinator`?
How about?
```
if use_new_coordinator is None:
use_new_coordinator = context.injected_args.get(`use_new_coordinator`)
if use_new_coordinator is None:
use_new_coordinator = context.globals.get(`use_new_coordinator`)
```
--
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