Re: [PR] KAFKA-15578: System Tests for running old protocol with new coordinator [kafka]
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-unsubscr...@kafka.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] KAFKA-15578: System Tests for running old protocol with new coordinator [kafka]
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-unsubscr...@kafka.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] KAFKA-15578: System Tests for running old protocol with new coordinator [kafka]
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-unsubscr...@kafka.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] KAFKA-15578: System Tests for running old protocol with new coordinator [kafka]
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-unsubscr...@kafka.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] KAFKA-15578: System Tests for running old protocol with new coordinator [kafka]
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-unsubscr...@kafka.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] KAFKA-15578: System Tests for running old protocol with new coordinator [kafka]
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-unsubscr...@kafka.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] KAFKA-15578: System Tests for running old protocol with new coordinator [kafka]
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-unsubscr...@kafka.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] KAFKA-15578: System Tests for running old protocol with new coordinator [kafka]
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-unsubscr...@kafka.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] KAFKA-15578: System Tests for running old protocol with new coordinator [kafka]
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-unsubscr...@kafka.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] KAFKA-15578: System Tests for running old protocol with new coordinator [kafka]
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-unsubscr...@kafka.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] KAFKA-15578: System Tests for running old protocol with new coordinator [kafka]
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-unsubscr...@kafka.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] KAFKA-15578: System Tests for running old protocol with new coordinator [kafka]
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-unsubscr...@kafka.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] KAFKA-15578: System Tests for running old protocol with new coordinator [kafka]
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-unsubscr...@kafka.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] KAFKA-15578: System Tests for running old protocol with new coordinator [kafka]
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-unsubscr...@kafka.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] KAFKA-15578: System Tests for running old protocol with new coordinator [kafka]
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-unsubscr...@kafka.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] KAFKA-15578: System Tests for running old protocol with new coordinator [kafka]
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-unsubscr...@kafka.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] KAFKA-15578: System Tests for running old protocol with new coordinator [kafka]
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-unsubscr...@kafka.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] KAFKA-15578: System Tests for running old protocol with new coordinator [kafka]
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-unsubscr...@kafka.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] KAFKA-15578: System Tests for running old protocol with new coordinator [kafka]
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-unsubscr...@kafka.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] KAFKA-15578: System Tests for running old protocol with new coordinator [kafka]
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-unsubscr...@kafka.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] KAFKA-15578: System Tests for running old protocol with new coordinator [kafka]
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-unsubscr...@kafka.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] KAFKA-15578: System Tests for running old protocol with new coordinator [kafka]
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-unsubscr...@kafka.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] KAFKA-15578: System Tests for running old protocol with new coordinator [kafka]
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-unsubscr...@kafka.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] KAFKA-15578: System Tests for running old protocol with new coordinator [kafka]
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-unsubscr...@kafka.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] KAFKA-15578: System Tests for running old protocol with new coordinator [kafka]
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-unsubscr...@kafka.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] KAFKA-15578: System Tests for running old protocol with new coordinator [kafka]
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-unsubscr...@kafka.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] KAFKA-15578: System Tests for running old protocol with new coordinator [kafka]
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=1):
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-unsubscr...@kafka.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] KAFKA-15578: System Tests for running old protocol with new coordinator [kafka]
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-unsubscr...@kafka.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] KAFKA-15578: System Tests for running old protocol with new coordinator [kafka]
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=1):
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-unsubscr...@kafka.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] KAFKA-15578: System Tests for running old protocol with new coordinator [kafka]
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-unsubscr...@kafka.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] KAFKA-15578: System Tests for running old protocol with new coordinator [kafka]
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=1):
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-unsubscr...@kafka.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] KAFKA-15578: System Tests for running old protocol with new coordinator [kafka]
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-unsubscr...@kafka.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] KAFKA-15578: System Tests for running old protocol with new coordinator [kafka]
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-unsubscr...@kafka.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] KAFKA-15578: System Tests for running old protocol with new coordinator [kafka]
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=1):
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-unsubscr...@kafka.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] KAFKA-15578: System Tests for running old protocol with new coordinator [kafka]
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-unsubscr...@kafka.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] KAFKA-15578: System Tests for running old protocol with new coordinator [kafka]
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-unsubscr...@kafka.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] KAFKA-15578: System Tests for running old protocol with new coordinator [kafka]
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=1):
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-unsubscr...@kafka.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] KAFKA-15578: System Tests for running old protocol with new coordinator [kafka]
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=1):
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-unsubscr...@kafka.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] KAFKA-15578: System Tests for running old protocol with new coordinator [kafka]
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-unsubscr...@kafka.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] KAFKA-15578: System Tests for running old protocol with new coordinator [kafka]
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-unsubscr...@kafka.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] KAFKA-15578: System Tests for running old protocol with new coordinator [kafka]
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=1):
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-unsubscr...@kafka.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] KAFKA-15578: System Tests for running old protocol with new coordinator [kafka]
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=1):
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-unsubscr...@kafka.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] KAFKA-15578: System Tests for running old protocol with new coordinator [kafka]
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-unsubscr...@kafka.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] KAFKA-15578: System Tests for running old protocol with new coordinator [kafka]
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=1):
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-unsubscr...@kafka.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] KAFKA-15578: System Tests for running old protocol with new coordinator [kafka]
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=1):
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-unsubscr...@kafka.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] KAFKA-15578: System Tests for running old protocol with new coordinator [kafka]
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=1):
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
