-
Notifications
You must be signed in to change notification settings - Fork 15.4k
KAFKA-9146: KIP-571 Add option to force delete active members in StreamsResetter #8589
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
8f00c6f
617dcf6
e02c3c6
c6b4ef3
6a845f3
6b15ded
d43ce29
1853cfe
d88ad33
4764677
eb50b5d
91c81b6
6c5778a
6dedd1c
f015c22
f92a3f6
87346c0
174ba7e
5329315
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -182,7 +182,6 @@ | |
| import org.apache.kafka.common.requests.FindCoordinatorResponse; | ||
| import org.apache.kafka.common.requests.IncrementalAlterConfigsRequest; | ||
| import org.apache.kafka.common.requests.IncrementalAlterConfigsResponse; | ||
| import org.apache.kafka.common.requests.JoinGroupRequest; | ||
| import org.apache.kafka.common.requests.LeaveGroupRequest; | ||
| import org.apache.kafka.common.requests.LeaveGroupResponse; | ||
| import org.apache.kafka.common.requests.ListGroupsRequest; | ||
|
|
@@ -3612,6 +3611,25 @@ private boolean dependsOnSpecificNode(ConfigResource resource) { | |
| || resource.type() == ConfigResource.Type.BROKER_LOGGER; | ||
| } | ||
|
|
||
| private List<MemberIdentity> getMembersFromGroup(String groupId) { | ||
| Collection<MemberDescription> members; | ||
| try { | ||
| members = describeConsumerGroups(Collections.singleton(groupId)).describedGroups().get(groupId).get().members(); | ||
| } catch (Exception ex) { | ||
| throw new KafkaException("Encounter exception when trying to get members from group: " + groupId, ex); | ||
| } | ||
|
|
||
| List<MemberIdentity> membersToRemove = new ArrayList<>(); | ||
| for (final MemberDescription member : members) { | ||
| if (member.groupInstanceId().isPresent()) { | ||
| membersToRemove.add(new MemberIdentity().setGroupInstanceId(member.groupInstanceId().get())); | ||
| } else { | ||
| membersToRemove.add(new MemberIdentity().setMemberId(member.consumerId())); | ||
| } | ||
| } | ||
| return membersToRemove; | ||
| } | ||
|
|
||
| @Override | ||
| public RemoveMembersFromConsumerGroupResult removeMembersFromConsumerGroup(String groupId, | ||
| RemoveMembersFromConsumerGroupOptions options) { | ||
|
|
@@ -3623,22 +3641,24 @@ public RemoveMembersFromConsumerGroupResult removeMembersFromConsumerGroup(Strin | |
| ConsumerGroupOperationContext<Map<MemberIdentity, Errors>, RemoveMembersFromConsumerGroupOptions> context = | ||
| new ConsumerGroupOperationContext<>(groupId, options, deadline, future); | ||
|
|
||
| Call findCoordinatorCall = getFindCoordinatorCall(context, | ||
| () -> getRemoveMembersFromGroupCall(context)); | ||
| List<MemberIdentity> members; | ||
| if (options.removeAll()) { | ||
| members = getMembersFromGroup(groupId); | ||
| } else { | ||
| members = options.members().stream().map(MemberToRemove::toMemberIdentity).collect(Collectors.toList()); | ||
| } | ||
| Call findCoordinatorCall = getFindCoordinatorCall(context, () -> getRemoveMembersFromGroupCall(context, members)); | ||
| runnable.call(findCoordinatorCall, startFindCoordinatorMs); | ||
|
|
||
| return new RemoveMembersFromConsumerGroupResult(future, options.members()); | ||
| } | ||
|
|
||
| private Call getRemoveMembersFromGroupCall(ConsumerGroupOperationContext<Map<MemberIdentity, Errors>, RemoveMembersFromConsumerGroupOptions> context) { | ||
| return new Call("leaveGroup", | ||
| context.deadline(), | ||
| new ConstantNodeIdProvider(context.node().get().id())) { | ||
| private Call getRemoveMembersFromGroupCall(ConsumerGroupOperationContext<Map<MemberIdentity, Errors>, RemoveMembersFromConsumerGroupOptions> context, | ||
| List<MemberIdentity> members) { | ||
| return new Call("leaveGroup", context.deadline(), new ConstantNodeIdProvider(context.node().get().id())) { | ||
| @Override | ||
| LeaveGroupRequest.Builder createRequest(int timeoutMs) { | ||
| return new LeaveGroupRequest.Builder(context.groupId(), | ||
| context.options().members().stream().map( | ||
| MemberToRemove::toMemberIdentity).collect(Collectors.toList())); | ||
| return new LeaveGroupRequest.Builder(context.groupId(), members); | ||
| } | ||
|
|
||
| @Override | ||
|
|
@@ -3647,7 +3667,7 @@ void handleResponse(AbstractResponse abstractResponse) { | |
|
|
||
| // If coordinator changed since we fetched it, retry | ||
| if (ConsumerGroupOperationContext.hasCoordinatorMoved(response)) { | ||
| Call call = getRemoveMembersFromGroupCall(context); | ||
| Call call = getRemoveMembersFromGroupCall(context, members); | ||
| rescheduleFindCoordinatorTask(context, () -> call, this); | ||
| return; | ||
| } | ||
|
|
@@ -3657,10 +3677,8 @@ void handleResponse(AbstractResponse abstractResponse) { | |
|
|
||
| final Map<MemberIdentity, Errors> memberErrors = new HashMap<>(); | ||
| for (MemberResponse memberResponse : response.memberResponses()) { | ||
| // We set member.id to empty here explicitly, so that the lookup will succeed as user doesn't | ||
| // know the exact member.id. | ||
| memberErrors.put(new MemberIdentity() | ||
| .setMemberId(JoinGroupRequest.UNKNOWN_MEMBER_ID) | ||
| .setMemberId(memberResponse.memberId()) | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I could see this doesn't hold true for a plain static member removal. Let's discuss why skipping the individual member check in
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. For removing static members, this still true because we put memberId as
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Not sure if I understand the change. Also not sure if I can follow the comments. Can you elaborate? |
||
| .setGroupInstanceId(memberResponse.groupInstanceId()), | ||
| Errors.forCode(memberResponse.errorCode())); | ||
| } | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -19,6 +19,7 @@ | |
| import org.apache.kafka.common.annotation.InterfaceStability; | ||
|
|
||
| import java.util.Collection; | ||
| import java.util.Collections; | ||
| import java.util.HashSet; | ||
| import java.util.Set; | ||
|
|
||
|
|
@@ -34,10 +35,21 @@ public class RemoveMembersFromConsumerGroupOptions extends AbstractOptions<Remov | |
| private Set<MemberToRemove> members; | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Could we just make members to be
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Sure. Taking a step further, can we just keep the the type
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Updated~ |
||
|
|
||
| public RemoveMembersFromConsumerGroupOptions(Collection<MemberToRemove> members) { | ||
| if (members.isEmpty()) { | ||
| throw new IllegalArgumentException("Invalid empty members has been provided"); | ||
| } | ||
| this.members = new HashSet<>(members); | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. As we have different semantics for an empty collection (it was "remove nothing" originally, and we change it to "remove all"), I am wondering if we should do a check if
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Make sense. It will throw exception if empty members provided now. |
||
| } | ||
|
|
||
| public RemoveMembersFromConsumerGroupOptions() { | ||
| this.members = Collections.emptySet(); | ||
| } | ||
|
|
||
| public Set<MemberToRemove> members() { | ||
| return members; | ||
| } | ||
|
|
||
| public boolean removeAll() { | ||
| return members.isEmpty(); | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -16,6 +16,7 @@ | |
| */ | ||
| package org.apache.kafka.clients.admin; | ||
|
|
||
| import org.apache.kafka.common.KafkaException; | ||
| import org.apache.kafka.common.KafkaFuture; | ||
| import org.apache.kafka.common.internals.KafkaFutureImpl; | ||
| import org.apache.kafka.common.message.LeaveGroupRequestData.MemberIdentity; | ||
|
|
@@ -51,9 +52,21 @@ public KafkaFuture<Void> all() { | |
| if (throwable != null) { | ||
| result.completeExceptionally(throwable); | ||
| } else { | ||
| for (MemberToRemove memberToRemove : memberInfos) { | ||
| if (maybeCompleteExceptionally(memberErrors, memberToRemove.toMemberIdentity(), result)) { | ||
| return; | ||
| if (removeAll()) { | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Not sure why the
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Because in non
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Well, while
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm not sure I understand the question, could you elaborate more?
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can we just do for both cases? The "issue" with using
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm afraid not because, in the non
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thanks for explaining! |
||
| for (Map.Entry<MemberIdentity, Errors> entry: memberErrors.entrySet()) { | ||
| Exception exception = entry.getValue().exception(); | ||
| if (exception != null) { | ||
| Throwable ex = new KafkaException("Encounter exception when trying to remove: " | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Wrap to let the failed member info available for caller like There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Let's put the exception in the cause so that we could verify the cause in
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Cool, updated |
||
| + entry.getKey(), exception); | ||
| result.completeExceptionally(ex); | ||
| return; | ||
| } | ||
| } | ||
| } else { | ||
| for (MemberToRemove memberToRemove : memberInfos) { | ||
| if (maybeCompleteExceptionally(memberErrors, memberToRemove.toMemberIdentity(), result)) { | ||
| return; | ||
| } | ||
| } | ||
| } | ||
| result.complete(null); | ||
|
|
@@ -66,6 +79,9 @@ public KafkaFuture<Void> all() { | |
| * Returns the selected member future. | ||
| */ | ||
| public KafkaFuture<Void> memberResult(MemberToRemove member) { | ||
| if (removeAll()) { | ||
| throw new IllegalArgumentException("The method: memberResult is not applicable in 'removeAll' mode"); | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why that? I understand that we expect that users don't know the memberId if the so a "remove all"; however, I don't see why we need to disallow this call? Can you elaborate?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Since in the
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I see. Makes sense. |
||
| } | ||
| if (!memberInfos.contains(member)) { | ||
| throw new IllegalArgumentException("Member " + member + " was not included in the original request"); | ||
| } | ||
|
|
@@ -93,4 +109,8 @@ private boolean maybeCompleteExceptionally(Map<MemberIdentity, Errors> memberErr | |
| return false; | ||
| } | ||
| } | ||
|
|
||
| private boolean removeAll() { | ||
| return memberInfos.isEmpty(); | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -114,6 +114,7 @@ | |
| import org.apache.kafka.common.requests.ElectLeadersResponse; | ||
| import org.apache.kafka.common.requests.FindCoordinatorResponse; | ||
| import org.apache.kafka.common.requests.IncrementalAlterConfigsResponse; | ||
| import org.apache.kafka.common.requests.JoinGroupRequest; | ||
| import org.apache.kafka.common.requests.LeaveGroupResponse; | ||
| import org.apache.kafka.common.requests.ListGroupsResponse; | ||
| import org.apache.kafka.common.requests.ListOffsetResponse; | ||
|
|
@@ -379,6 +380,23 @@ private static MetadataResponse prepareMetadataResponse(Cluster cluster, Errors | |
| MetadataResponse.AUTHORIZED_OPERATIONS_OMITTED); | ||
| } | ||
|
|
||
| private static DescribeGroupsResponseData prepareDescribeGroupsResponseData(String groupId, | ||
| List<String> groupInstances, | ||
| List<TopicPartition> topicPartitions) { | ||
| final ByteBuffer memberAssignment = ConsumerProtocol.serializeAssignment(new ConsumerPartitionAssignor.Assignment(topicPartitions)); | ||
| List<DescribedGroupMember> describedGroupMembers = groupInstances.stream().map(groupInstance -> DescribeGroupsResponse.groupMember(JoinGroupRequest.UNKNOWN_MEMBER_ID, | ||
| groupInstance, "clientId0", "clientHost", new byte[memberAssignment.remaining()], null)).collect(Collectors.toList()); | ||
| DescribeGroupsResponseData data = new DescribeGroupsResponseData(); | ||
| data.groups().add(DescribeGroupsResponse.groupMetadata( | ||
| groupId, | ||
| Errors.NONE, | ||
| "", | ||
| ConsumerProtocol.PROTOCOL_TYPE, | ||
| "", | ||
| describedGroupMembers, | ||
| Collections.emptySet())); | ||
| return data; | ||
| } | ||
| /** | ||
| * Test that the client properly times out when we don't receive any metadata. | ||
| */ | ||
|
|
@@ -2411,6 +2429,50 @@ public void testRemoveMembersFromGroup() throws Exception { | |
| assertNull(noErrorResult.all().get()); | ||
| assertNull(noErrorResult.memberResult(memberOne).get()); | ||
| assertNull(noErrorResult.memberResult(memberTwo).get()); | ||
|
|
||
| // Test the "removeAll" scenario | ||
| final List<TopicPartition> topicPartitions = Arrays.asList(1, 2, 3).stream().map(partition -> new TopicPartition("my_topic", partition)) | ||
| .collect(Collectors.toList()); | ||
| // construct the DescribeGroupsResponse | ||
| DescribeGroupsResponseData data = prepareDescribeGroupsResponseData(groupId, Arrays.asList(instanceOne, instanceTwo), topicPartitions); | ||
|
|
||
| // Return with partial failure for "removeAll" scenario | ||
| // 1 prepare response for AdminClient.describeConsumerGroups | ||
| env.kafkaClient().prepareResponse(prepareFindCoordinatorResponse(Errors.NONE, env.cluster().controller())); | ||
| env.kafkaClient().prepareResponse(new DescribeGroupsResponse(data)); | ||
|
|
||
| // 2 KafkaAdminClient encounter partial failure when trying to delete all members | ||
| env.kafkaClient().prepareResponse(prepareFindCoordinatorResponse(Errors.NONE, env.cluster().controller())); | ||
| env.kafkaClient().prepareResponse(new LeaveGroupResponse( | ||
| new LeaveGroupResponseData().setErrorCode(Errors.NONE.code()).setMembers( | ||
| Arrays.asList(responseOne, responseTwo)) | ||
| )); | ||
| final RemoveMembersFromConsumerGroupResult partialFailureResults = env.adminClient().removeMembersFromConsumerGroup( | ||
| groupId, | ||
| new RemoveMembersFromConsumerGroupOptions() | ||
| ); | ||
| ExecutionException exception = assertThrows(ExecutionException.class, () -> partialFailureResults.all().get()); | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. No existing help method to assert the cause of exception throw by |
||
| assertTrue(exception.getCause() instanceof KafkaException); | ||
| assertTrue(exception.getCause().getCause() instanceof UnknownMemberIdException); | ||
|
|
||
| // Return with success for "removeAll" scenario | ||
| // 1 prepare response for AdminClient.describeConsumerGroups | ||
| env.kafkaClient().prepareResponse(prepareFindCoordinatorResponse(Errors.NONE, env.cluster().controller())); | ||
| env.kafkaClient().prepareResponse(new DescribeGroupsResponse(data)); | ||
|
|
||
| // 2. KafkaAdminClient should delete all members correctly | ||
| env.kafkaClient().prepareResponse(prepareFindCoordinatorResponse(Errors.NONE, env.cluster().controller())); | ||
| env.kafkaClient().prepareResponse(new LeaveGroupResponse( | ||
| new LeaveGroupResponseData().setErrorCode(Errors.NONE.code()).setMembers( | ||
| Arrays.asList(responseTwo, | ||
| new MemberResponse().setGroupInstanceId(instanceOne).setErrorCode(Errors.NONE.code()) | ||
| )) | ||
| )); | ||
| final RemoveMembersFromConsumerGroupResult successResult = env.adminClient().removeMembersFromConsumerGroup( | ||
| groupId, | ||
| new RemoveMembersFromConsumerGroupOptions() | ||
| ); | ||
| assertNull(successResult.all().get()); | ||
| } | ||
| } | ||
|
|
||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If
option.members()is empty, it implies that we do aremoveAll()-- hence, should we pass inmembersinto theRemoveMembersFromConsumerGroupResultinstead ofoptions.members()?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
--- If option.members() is empty, it implies that we do a removeAll()
=> Yes, that is correct.
--- hence, should we pass in members into the RemoveMembersFromConsumerGroupResult instead of options.members()
=> The members is of type
List<MemberIdentity>andMemberIdentitycontains field:memberIdwhich supports the removal of dynamic members, whileoptions.members()is of type:Set<MemberToRemove>, MemberToRemove only supports static member removal specification, in RemoveMembersFromConsumerGroupResult we treat similarly like inRemoveMembersFromConsumerGroupOptions, emptymembersimpliesremoveAll,we handle it in this way because we think in
non removeAllscenario we would only remove static members, while inremoveAllscenario we may remove both static and dynamic members.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for clarifying.