Added handling members with `group.instance.id` set i.e. static members #4684

mmaslankaprv · 2022-05-11T11:46:49Z

Cover letter

When static member joins the group its member_id is identified using a mapping stored in _static_members map. This way broker can identify the member_id that is assigned to requested instance id. In dynamic membership protocol member_id is assigned by broker after each restart of consumer (there is no state persistence on the client side) hence every consumer restart causes rebalance since member with new id joins the group. With static members, when group.instance.id is present in the JoinRequest and mapping is already present coordinator will use that mapping to find member metadata.

When using static membership protocol consumers does not send LeaveGroup requests when stopping.

More information:
KIP-345

Fixes: #1335

Depends on: #4673

Release notes

Improvements

support for static group membership protocol.

src/v/kafka/server/group.cc

BenPope

Very cool, just some nits, and a couple of questions.

src/v/kafka/server/group.cc

src/v/kafka/server/tests/consumer_groups_test.cc

dotnwat · 2022-05-16T05:06:36Z

Will review on Monday! Sorry about delay

dotnwat

good stuff. mostly nits / questions

src/v/kafka/server/member.h

src/v/kafka/server/group.cc

src/v/platform/stop_signal.h

src/v/kafka/server/handlers/describe_groups.cc

tests/rptest/clients/rpk.py

tests/rptest/tests/consumer_group_test.py

Signed-off-by: Michal Maslanka <[email protected]>

dotnwat

looks really good.

what should the testing approach be? i see ducktape tests for important scenarios, but the scale of the tests seems to be 2 group members. it seems like we need to expand testing for static members, at a minimum scaling the tests to be more substantial.

separate from this PR, do you think we need more testing for consumer groups in general in ducktape? how can we bring some of the recent consumer group scaling work into ducktape to run in cdt?

dotnwat · 2022-05-20T04:09:19Z

src/v/kafka/server/group.cc

- // <kafka>Only return MEMBER_ID_REQUIRED error if joinGroupRequest version
- // is >= 4 and groupInstanceId is configured to unknown.</kafka>
- if (r.version >= api_version(4) && !r.data.group_instance_id) {
+ if (member->is_joining()) {


why is it ok to remove this condition that is dependent on request version, and which is handled by upstream kafka? is the same condition handled in some other way?

i think that diff is here misleading. This is entirely new code path that was introduced to handle members with group.iinstance.id assigned

yeh, but i guess what i'm saying is that if (r.version >= api_version(4) && !r.data.group_instance_id) { is a specific condition in the protocol controlling behavior that clients expect. and it's not clear if that behavior is preserved and where its new location is at in the implementation.

ahh i get it now. it's implied in add_new_dynamic_member and then the version check is relocated there!

src/v/kafka/server/group.cc

dotnwat · 2022-05-20T04:12:15Z

src/v/kafka/server/group.cc

 // <kafka>Only return MEMBER_ID_REQUIRED error if joinGroupRequest version
 // is >= 4 and groupInstanceId is configured to unknown.</kafka>
- if (r.version >= api_version(4) && !r.data.group_instance_id) {
+ if (r.version >= api_version(4)) {


Is it worth asserting !r.data.group_instance_id?

it seems like it. and also brining back the kafka comment to indicate that we are preserving kafka semantics.

mmaslankaprv · 2022-05-20T05:50:18Z

looks really good.

what should the testing approach be? i see ducktape tests for important scenarios, but the scale of the tests seems to be 2 group members. it seems like we need to expand testing for static members, at a minimum scaling the tests to be more substantial.

separate from this PR, do you think we need more testing for consumer groups in general in ducktape? how can we bring some of the recent consumer group scaling work into ducktape to run in cdt?

I will create a follow up PR. I have modified client-swarm a bit to add multi consumers testing we may leverage that it should be possible to create tens of consumers.

Signed-off-by: Michal Maslanka <[email protected]>

Checkpoint assignments may come from either the sync request or the members metadata stored in group metadata. Added generic checkpoint method that allows to build a checkpoint using different assignments providers. Signed-off-by: Michal Maslanka <[email protected]>

Previously the `stop_signal` was destroyed in `fixture::start()` method leading to the signaling stop immediately after fixture application was started. Signed-off-by: Michal Maslanka <[email protected]>

Added handling members with `group.instance.id` set i.e. static members. When static member joins the group its `member_id` is identified using a mapping stored in `_static_members` map. This way broker can identify the `member_id` that is assigned to requested instance id. In dynamic membership protocol `member_id` is assigned by broker after each restart of consumer (there is no state persistence on the client side) hence every consumer restart causes rebalance since member with new id joins the group. With static members, when `group.instance.id` is present in the `JoinRequest` and mapping is already present coordinator will use that mapping to find member metadata. When using static membership protocol consumers does not send `LeaveGroup` requests when stopping. More information: [KIP-345](https://cwiki.apache.org/confluence/display/KAFKA/KIP-345%3A+Introduce+static+membership+protocol+to+reduce+consumer+rebalances) Signed-off-by: Michal Maslanka <[email protected]>

Signed-off-by: Michal Maslanka <[email protected]>

Added `formatter_properties` parameter to control how messages are formatted when using Kafka CLI tools consumer. Signed-off-by: Michal Maslanka <[email protected]>

When consumer is being stopped it may send `LeaveGroup` request to the broker. Using clean shutdown (SIGTERM) to give the consumer process oportunity to finish gracefully. Signed-off-by: Michal Maslanka <[email protected]>

Signed-off-by: Michal Maslanka <[email protected]>

Added tests validating basic consumer group operations when using dynamic and static membership protocols. Signed-off-by: Michal Maslanka <[email protected]>

mmaslankaprv requested review from dotnwat, NyaliaLui, Lazin and BenPope as code owners May 11, 2022 11:46

github-actions bot added the area/redpanda label May 11, 2022

mmaslankaprv force-pushed the static-membership branch from debcf68 to 2110d3e Compare May 11, 2022 11:54

VadimPlh mentioned this pull request May 11, 2022

Support KIP-447 #3279

Closed

VadimPlh self-requested a review May 11, 2022 13:01

VadimPlh reviewed May 12, 2022

View reviewed changes

src/v/kafka/server/group.cc Outdated Show resolved Hide resolved

BenPope reviewed May 12, 2022

View reviewed changes

mmaslankaprv force-pushed the static-membership branch from 2110d3e to b9c4e05 Compare May 13, 2022 11:54

mmaslankaprv force-pushed the static-membership branch from b9c4e05 to 2f77a7a Compare May 16, 2022 05:49

dotnwat reviewed May 16, 2022

View reviewed changes

mmaslankaprv force-pushed the static-membership branch from 2f77a7a to 692f0b2 Compare May 17, 2022 08:59

k/group: added warn method to group context logger

a334266

Signed-off-by: Michal Maslanka <[email protected]>

mmaslankaprv force-pushed the static-membership branch from 692f0b2 to 47f1d0a Compare May 17, 2022 09:52

mmaslankaprv requested review from dotnwat and removed request for VadimPlh May 18, 2022 05:32

dotnwat reviewed May 20, 2022

View reviewed changes

mmaslankaprv added 9 commits May 20, 2022 07:57

k/generator: using named type for group.instance.id in request

477a2ba

Signed-off-by: Michal Maslanka <[email protected]>

fixture: make stop_signal part of the fixture

a445bfd

Previously the `stop_signal` was destroyed in `fixture::start()` method leading to the signaling stop immediately after fixture application was started. Signed-off-by: Michal Maslanka <[email protected]>

k/join_group: support static membership protocol in join group

578b1bb

Signed-off-by: Michal Maslanka <[email protected]>

k/group_manager: support static membership in group manager

1a64134

Signed-off-by: Michal Maslanka <[email protected]>

k/offset_commit: support static membership in offset commit handler

b9295d9

Signed-off-by: Michal Maslanka <[email protected]>

k/describe_groups: support static membership in describe groups handler

abbfabd

Signed-off-by: Michal Maslanka <[email protected]>

k/tests: added simple consumer group join test

745f999

Signed-off-by: Michal Maslanka <[email protected]>

mmaslankaprv added 6 commits May 20, 2022 07:57

tests: added kafka v3.0.0 cli tools support

c550f60

Signed-off-by: Michal Maslanka <[email protected]>

tests/rpk: added group instance id to group describe output

26f2c69

Signed-off-by: Michal Maslanka <[email protected]>

tests/kafka_cli_consumer: added formatter properties

ad3ac60

Added `formatter_properties` parameter to control how messages are formatted when using Kafka CLI tools consumer. Signed-off-by: Michal Maslanka <[email protected]>

tests/kafka_cli_consumer: added wait_for_started method

e37d184

Signed-off-by: Michal Maslanka <[email protected]>

tests: added consumer group handling tests

a91767e

Added tests validating basic consumer group operations when using dynamic and static membership protocols. Signed-off-by: Michal Maslanka <[email protected]>

mmaslankaprv force-pushed the static-membership branch from 47f1d0a to a91767e Compare May 20, 2022 05:57

mmaslankaprv requested a review from dotnwat May 20, 2022 05:59

dotnwat approved these changes May 20, 2022

View reviewed changes

mmaslankaprv merged commit f7208fe into redpanda-data:dev May 20, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added handling members with `group.instance.id` set i.e. static members #4684

Added handling members with `group.instance.id` set i.e. static members #4684

mmaslankaprv commented May 11, 2022 •

edited

Loading

BenPope left a comment

dotnwat commented May 16, 2022

dotnwat left a comment

dotnwat left a comment

dotnwat May 20, 2022

mmaslankaprv May 20, 2022

dotnwat May 20, 2022

dotnwat May 20, 2022

dotnwat May 20, 2022

mmaslankaprv commented May 20, 2022

Added handling members with group.instance.id set i.e. static members #4684

Added handling members with group.instance.id set i.e. static members #4684

Conversation

mmaslankaprv commented May 11, 2022 • edited Loading

Cover letter

Release notes

Improvements

BenPope left a comment

Choose a reason for hiding this comment

dotnwat commented May 16, 2022

dotnwat left a comment

Choose a reason for hiding this comment

dotnwat left a comment

Choose a reason for hiding this comment

dotnwat May 20, 2022

Choose a reason for hiding this comment

mmaslankaprv May 20, 2022

Choose a reason for hiding this comment

dotnwat May 20, 2022

Choose a reason for hiding this comment

dotnwat May 20, 2022

Choose a reason for hiding this comment

dotnwat May 20, 2022

Choose a reason for hiding this comment

mmaslankaprv commented May 20, 2022

Added handling members with `group.instance.id` set i.e. static members #4684

Added handling members with `group.instance.id` set i.e. static members #4684

mmaslankaprv commented May 11, 2022 •

edited

Loading