KAFKA-18156: VerifiableConsumer should ignore "--session-timeout" when using CONSUMER protocol by brandboat · Pull Request #18036 · apache/kafka

brandboat · 2024-12-04T14:54:44Z

The "session.timeout.ms" is unsupported by CONSUMER protocol, so VerifiableConsumer should skip the config when using CONSUMER protocol. Additionally, we should update the docs of "--session-timeout" to highlight this change

Committer Checklist (excluded from commit message)

Verify design and implementation
Verify test coverage and CI build status
Verify documentation (including upgrade notes)

…n using CONSUMER protocol

TaiJuWu · 2024-12-05T08:26:49Z

-        consumerProps.put(ConsumerConfig.SESSION_TIMEOUT_MS_CONFIG, Integer.toString(res.getInt("sessionTimeout")));
+
+        // session.timeout.ms cannot be set when group.protocol=CONSUMER
+        if (!groupProtocol.equalsIgnoreCase(GroupProtocol.CONSUMER.name)) {


If we set --session-timeout for consumer protocol, can we add warning for this config?

Thanks for the suggestion! However, I’m uncertain whether this change is necessary. When group.protocol=CONSUMER is used, setting a session timeout in the consumer prevents it from starting successfully. This makes me feel the change might be redundant. That said, I’d appreciate hearing others’ thoughts on this. Thanks!

maybe we should call consumerProps.put(ConsumerConfig.SESSION_TIMEOUT_MS_CONFIG, xx) only if users do set the --session-timeout - that means we should remove the default value of --session-timeout

Sounds good, btw we already set --session-timeout 30sec by default in

kafka/tests/kafkatest/services/verifiable_consumer.py

Line 234 in 440e0b8

static_membership=False, max_messages=-1, session_timeout_sec=30, enable_autocommit=False,

The default value of session.timeout.ms is 45 seconds, so I'm unsure if it's necessary to set it to 30 seconds in the end-to-end tests. Could you please remove this setting from the e2e configuration? Additionally, e2e should not use --session-timeout when running AsyncConsumer.

Thanks, I already addressed the comment in the latest commit.

chia7712 · 2024-12-06T22:51:53Z

        consumerProps.put(ConsumerConfig.AUTO_OFFSET_RESET_CONFIG, res.getString("resetPolicy"));
-        consumerProps.put(ConsumerConfig.SESSION_TIMEOUT_MS_CONFIG, Integer.toString(res.getInt("sessionTimeout")));
+
+        // session.timeout.ms cannot be set when group.protocol=CONSUMER


Could you please move this check to line#645?

chia7712

@brandboat I have left some comments to align with the following ideas:

The default value should not cause an error.
Honor users' configurations—avoid adding extra if-else statements that swallow errors.

chia7712 · 2024-12-07T17:00:08Z

-            // This means we're using the old consumer group protocol.
+            // This means we're using the CLASSIC consumer group protocol.
            consumerProps.put(ConsumerConfig.PARTITION_ASSIGNMENT_STRATEGY_CONFIG, res.getString("assignmentStrategy"));
+            Integer sessionTimeout = res.getInt("sessionTimeout");


If this tool does not offer the default value, it is fine to throw exception if user does set the --session-timeout

it is fine to throw exception if user does set the --session-timeout

Pardon me, did you mean throw exception if user set session timeout when using CONSUMER group.protocol ? Not sure if I understand correctly 🙏

yes, we should honor users' configs to let him encounter the error.

chia7712 · 2024-12-07T17:01:18Z

+                (self.reset_policy, self.group_id, self.topic)
+
+        # session timeout is not supported when using CONSUMER group protocol
+        if self.session_timeout_sec > 0 and self.is_consumer_group_protocol_enabled():


this will swallow the incorrect configs in e2e. We can just set the session timeout if e2e test case does set it.

Only VerifiableConsumerTest specifically set session timeout, but I think it's a bit redundant, so I refactor them out.

brandboat · 2024-12-08T13:47:54Z

-        self.group_protocol = group_protocol
-        self.group_remote_assignor = group_remote_assignor


these 2 lines are duplicate code, see L245, L246

chia7712 · 2024-12-08T16:53:49Z

    def setup_consumer(self, topic, static_membership=False, enable_autocommit=False,
                       assignment_strategy="org.apache.kafka.clients.consumer.RangeAssignor", group_remote_assignor="range", **kwargs):
        return VerifiableConsumer(self.test_context, self.num_consumers, self.kafka,
-                                  topic, self.group_id, static_membership=static_membership, session_timeout_sec=self.session_timeout_sec,


session_timeout_sec is used by other end-to-end tests, so we need to perform some refactoring for those test cases.

Replace Dynamic Timeouts with Constants
For example, change timeout_sec=self.session_timeout_sec + 5 to timeout_sec=60.

Remove Use Cases of Increasing session_timeout_sec
These cases typically increase session_timeout_sec from 45 seconds to 60 seconds. This adjustment may be unnecessary if the tests can run stably without modifying the timeout.

I tested e2e client again. TC_PATHS="tests/kafkatest/tests/client" bash tests/docker/run_tests.sh and only one test failed.

test_id: kafkatest.tests.client.consumer_test.OffsetValidationTest.test_broker_rolling_bounce.metadata_quorum=ISOLATED_KRAFT.use_new_coordinator=False status: FAIL run time: 3 minutes 23.970 seconds AssertionError('Broker rolling bounce caused 7 unexpected group rebalances') Traceback (most recent call last): File "/usr/local/lib/python3.7/dist-packages/ducktape/tests/runner_client.py", line 351, in _do_run data = self.run_test() File "/usr/local/lib/python3.7/dist-packages/ducktape/tests/runner_client.py", line 411, in run_test return self.test_context.function(self.test) File "/usr/local/lib/python3.7/dist-packages/ducktape/mark/_mark.py", line 438, in wrapper return functools.partial(f, *args, **kwargs)(*w_args, **w_kwargs) File "/opt/kafka-dev/tests/kafkatest/tests/client/consumer_test.py", line 121, in test_broker_rolling_bounce "Broker rolling bounce caused %d unexpected group rebalances" % unexpected_rebalances AssertionError: Broker rolling bounce caused 7 unexpected group rebalances

And looks like it was tracked by https://issues.apache.org/jira/browse/KAFKA-18194

chia7712 · 2024-12-11T13:04:54Z

the hanging test is traced by https://issues.apache.org/jira/browse/KAFKA-17554

… using CONSUMER protocol (apache#18036) Reviewers: TaiJuWu <tjwu1217@gmail.com>, Chia-Ping Tsai <chia7712@gmail.com>

KAFKA-18156: VerifiableConsumer should ignore "--session-timeout" whe…

a74b2fc

…n using CONSUMER protocol

github-actions bot added tools clients small Small PRs labels Dec 4, 2024

TaiJuWu reviewed Dec 5, 2024

View reviewed changes

chia7712 mentioned this pull request Dec 6, 2024

KAFKA-18176: Fix verifiable consumer with unsupported protocol configs #18085

Closed

chia7712 reviewed Dec 6, 2024

View reviewed changes

brandboat added 5 commits December 7, 2024 10:15

Address comment

64c61d5

Merge remote-tracking branch 'origin/trunk' into KAFKA-18156

2084983

Address comment

8a3ccc4

Merge remote-tracking branch 'origin/trunk' into KAFKA-18156

84b078a

Enhance config desc

892a08d

TaiJuWu approved these changes Dec 7, 2024

View reviewed changes

brandboat added 2 commits December 7, 2024 15:03

Address comments

5d77e9e

Merge remote-tracking branch 'origin/trunk' into KAFKA-18156

b876ca6

chia7712 reviewed Dec 7, 2024

View reviewed changes

brandboat added 2 commits December 8, 2024 21:46

Address comment

eb5047e

Merge remote-tracking branch 'origin/trunk' into KAFKA-18156

90d9337

brandboat commented Dec 8, 2024

View reviewed changes

Address comment

bc10813

chia7712 reviewed Dec 8, 2024

View reviewed changes

brandboat added 2 commits December 9, 2024 23:45

Address comments

84665d2

Merge remote-tracking branch 'origin/trunk' into KAFKA-18156

7ad29c5

chia7712 approved these changes Dec 11, 2024

View reviewed changes

chia7712 merged commit d2ad418 into apache:trunk Dec 11, 2024

brandboat deleted the KAFKA-18156 branch December 11, 2024 14:41

jolshan mentioned this pull request Dec 12, 2024

KAFKA-17705: Add Transactions V2 system tests and mark as production ready #18132

Merged

kirktrue added consumer tests Test fixes (including flaky tests) labels Dec 12, 2024

kirktrue added the KIP-848 The Next Generation of the Consumer Rebalance Protocol label Dec 12, 2024

peterxcli pushed a commit to peterxcli/kafka that referenced this pull request Dec 18, 2024

KAFKA-18156 VerifiableConsumer should ignore "--session-timeout" when…

ff19ad8

… using CONSUMER protocol (apache#18036) Reviewers: TaiJuWu <tjwu1217@gmail.com>, Chia-Ping Tsai <chia7712@gmail.com>

tedyu pushed a commit to tedyu/kafka that referenced this pull request Jan 6, 2025

KAFKA-18156 VerifiableConsumer should ignore "--session-timeout" when…

e940fd6

… using CONSUMER protocol (apache#18036) Reviewers: TaiJuWu <tjwu1217@gmail.com>, Chia-Ping Tsai <chia7712@gmail.com>

		self.group_protocol = group_protocol
		self.group_remote_assignor = group_remote_assignor

Conversation

brandboat commented Dec 4, 2024

Committer Checklist (excluded from commit message)

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

chia7712 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

chia7712 commented Dec 11, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants