Skip to content
Merged
Show file tree
Hide file tree
Changes from 10 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion checkstyle/suppressions.xml
Original file line number Diff line number Diff line change
Expand Up @@ -347,7 +347,7 @@
<suppress checks="ClassDataAbstractionCouplingCheck"
files="(RecordHelpersTest|GroupMetadataManager|GroupMetadataManagerTest|OffsetMetadataManagerTest|GroupCoordinatorServiceTest|GroupCoordinatorShardTest).java"/>
<suppress checks="JavaNCSS"
files="GroupMetadataManagerTest.java"/>
files="(GroupMetadataManager|GroupMetadataManagerTest).java"/>
Comment thread
dajac marked this conversation as resolved.

<!-- storage -->
<suppress checks="CyclomaticComplexity"
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -2015,21 +2015,7 @@ private <T> CoordinatorResult<T, CoordinatorRecord> consumerGroupFenceMember(
if (validateOnlineDowngrade(group, member.memberId())) {
return convertToClassicGroup(group, member.memberId(), response);
} else {
List<CoordinatorRecord> records = new ArrayList<>();
removeMember(records, group.groupId(), member.memberId());

// We update the subscription metadata without the leaving member.
Map<String, TopicMetadata> subscriptionMetadata = group.computeSubscriptionMetadata(
group.computeSubscribedTopicNames(member, null),
metadataImage.topics(),
metadataImage.cluster()
);

if (!subscriptionMetadata.equals(group.subscriptionMetadata())) {
log.info("[GroupId {}] Computed new subscription metadata: {}.",
group.groupId(), subscriptionMetadata);
records.add(newGroupSubscriptionMetadataRecord(group.groupId(), subscriptionMetadata));
}
List<CoordinatorRecord> records = removeMemberAndMaybeUpdateSubscriptionMetadata(group, member);

// We bump the group epoch.
int groupEpoch = group.groupEpoch() + 1;
Expand All @@ -2041,6 +2027,36 @@ private <T> CoordinatorResult<T, CoordinatorRecord> consumerGroupFenceMember(
}
}

/**
* Remove the member and maybe update the subscription metadata without the removed member.
*
* @param group The ConsumerGroup.
* @param member The ConsumerGroupMember.
* @return The list of CoordinatorRecord.
*/
private List<CoordinatorRecord> removeMemberAndMaybeUpdateSubscriptionMetadata(
ConsumerGroup group,
ConsumerGroupMember member
) {
List<CoordinatorRecord> records = new ArrayList<>();
removeMember(records, group.groupId(), member.memberId());

// We update the subscription metadata without the leaving member.
Map<String, TopicMetadata> subscriptionMetadata = group.computeSubscriptionMetadata(
group.computeSubscribedTopicNames(member, null),
metadataImage.topics(),
metadataImage.cluster()
);

if (!subscriptionMetadata.equals(group.subscriptionMetadata())) {
log.info("[GroupId {}] Computed new subscription metadata: {}.",
group.groupId(), subscriptionMetadata);
records.add(newGroupSubscriptionMetadataRecord(group.groupId(), subscriptionMetadata));
}

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that we should rather do this once, after all the members are processed. I suppose that we could have a method like computeSubscribedTopicNames but which takes a list of members to remove.


return records;
}

/**
* Write tombstones for the member. The order matters here.
*
Expand All @@ -2063,6 +2079,7 @@ private void removeMember(List<CoordinatorRecord> records, String groupId, Strin
private void cancelTimers(String groupId, String memberId) {
cancelConsumerGroupSessionTimeout(groupId, memberId);
cancelConsumerGroupRebalanceTimeout(groupId, memberId);
cancelConsumerGroupJoinTimeout(groupId, memberId);
cancelConsumerGroupSyncTimeout(groupId, memberId);
}

Expand Down Expand Up @@ -4424,14 +4441,113 @@ private ConsumerGroupMember validateConsumerGroupMember(
* @param context The request context.
* @param request The actual LeaveGroup request.
*
* @return The LeaveGroup response and the records to append.
*/
public CoordinatorResult<LeaveGroupResponseData, CoordinatorRecord> classicGroupLeave(
RequestContext context,
LeaveGroupRequestData request
) throws UnknownMemberIdException, GroupIdNotFoundException {
Comment thread
dongnuo123 marked this conversation as resolved.
Outdated
Group group = groups.get(request.groupId(), Long.MAX_VALUE);

if (group == null) {
throw new UnknownMemberIdException(String.format("Group %s not found.", request.groupId()));
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i'm guessing we don't use

Group group = group(request.memberId());

as we throw unknown member id for the old coordinator, is this correct? it seems very counterintuitive unfortunately

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, it's counterintuitive..

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perhaps, a better strategy overall would be to always use GroupIdNotFoundException internally and to translate it to UnknownMemberIdException where needed (e.g. by catching and rethrowing) with a comment explaining why.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like that approach. It's even more confusing now since we have mixed groups

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should we hold onto #16073 then?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes. I will update it. @dongnuo123 Could you please change it here?


if (group.type() == CLASSIC) {
return classicGroupLeaveToClassicGroup((ClassicGroup) group, context, request);
} else {
return classicGroupLeaveToConsumerGroup((ConsumerGroup) group, context, request);
}
}

/**
* Handle a classic LeaveGroupRequest to a ConsumerGroup.
*
* @param group The ConsumerGroup.
* @param context The request context.
* @param request The actual LeaveGroup request.
*
* @return The LeaveGroup response and the records to append.
*/
private CoordinatorResult<LeaveGroupResponseData, CoordinatorRecord> classicGroupLeaveToConsumerGroup(
ConsumerGroup group,
RequestContext context,
LeaveGroupRequestData request
) throws UnknownMemberIdException, GroupIdNotFoundException {
Comment thread
dongnuo123 marked this conversation as resolved.
Outdated
List<MemberResponse> memberResponses = new ArrayList<>();
List<CoordinatorRecord> records = new ArrayList<>();
boolean hasValidLeaveGroupMember = false;

for (MemberIdentity memberIdentity: request.members()) {
Comment thread
dongnuo123 marked this conversation as resolved.
Outdated
String memberId = memberIdentity.memberId();
String instanceId = memberIdentity.groupInstanceId();
String reason = memberIdentity.reason() != null ? memberIdentity.reason() : "not provided";

ConsumerGroupMember member;
try {
if (instanceId == null) {
member = group.getOrMaybeCreateMember(memberId, false);
throwIfMemberDoesNotUseClassicProtocol(member);

log.info("[Group {}] Static Member {} has left group " +
Comment thread
dongnuo123 marked this conversation as resolved.
Outdated
"through explicit `LeaveGroup` request; client reason: {}",
group.groupId(), memberId, reason);
} else {
member = group.staticMember(instanceId);
throwIfStaticMemberIsUnknown(member, memberId);
Comment thread
dongnuo123 marked this conversation as resolved.
Outdated
// The LeaveGroup API allows administrative removal of members by GroupInstanceId
// in which case we expect the MemberId to be undefined.
if (!UNKNOWN_MEMBER_ID.equals(memberId)) {
throwIfInstanceIdIsFenced(member, group.groupId(), memberId, instanceId);
}
throwIfMemberDoesNotUseClassicProtocol(member);

log.info("[Group {}] Static Member {} with instance id {} has left group " +
"through explicit `LeaveGroup` request; client reason: {}",
group.groupId(), memberId, instanceId, reason);
}

records.addAll(removeMemberAndMaybeUpdateSubscriptionMetadata(group, member));

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: I would be better to pass the list to the method.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is because we want to invoke group.computeSubscriptionMetadata() once right?

cancelTimers(group.groupId(), member.memberId());

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This will not be reverted if the replay/persistence fails. Is this something we want to address with the snapshottable timer?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I think that we can address it separately.

memberResponses.add(
new MemberResponse()
.setMemberId(member.memberId())
.setGroupInstanceId(instanceId)
);
hasValidLeaveGroupMember = true;
} catch (KafkaException e) {
memberResponses.add(
new MemberResponse()
.setMemberId(memberId)
.setGroupInstanceId(instanceId)
.setErrorCode(Errors.forException(e).code())
);
}
}

if (hasValidLeaveGroupMember) {
Comment thread
dongnuo123 marked this conversation as resolved.
Outdated
// Bump the group epoch.
records.add(newGroupEpochRecord(group.groupId(), group.groupEpoch() + 1));
}

return new CoordinatorResult<>(records, new LeaveGroupResponseData().setMembers(memberResponses));
}

/**
* Handle a classic LeaveGroupRequest to a ClassicGroup.
*
* @param group The ClassicGroup.
* @param context The request context.
* @param request The actual LeaveGroup request.
*
* @return The LeaveGroup response and the GroupMetadata record to append if the group
* no longer has any members.
*/
public CoordinatorResult<LeaveGroupResponseData, CoordinatorRecord> classicGroupLeave(
private CoordinatorResult<LeaveGroupResponseData, CoordinatorRecord> classicGroupLeaveToClassicGroup(
ClassicGroup group,
RequestContext context,
LeaveGroupRequestData request
) throws UnknownMemberIdException, GroupIdNotFoundException {
Comment thread
dongnuo123 marked this conversation as resolved.
Outdated
ClassicGroup group = getOrMaybeCreateClassicGroup(request.groupId(), false);
if (group.isInState(DEAD)) {
return new CoordinatorResult<>(
Collections.emptyList(),
Expand Down
Loading