Skip to content

KAFKA-9146 [WIP]: Add option to force delete active members in StreamsResetter#8020

Closed
feyman2016 wants to merge 10 commits intoapache:trunkfrom
feyman2016:kafka-9146
Closed

KAFKA-9146 [WIP]: Add option to force delete active members in StreamsResetter#8020
feyman2016 wants to merge 10 commits intoapache:trunkfrom
feyman2016:kafka-9146

Conversation

@feyman2016
Copy link
Copy Markdown
Contributor

@feyman2016 feyman2016 commented Jan 30, 2020

This PR is mainly to enhance https://issues.apache.org/jira/browse/KAFKA-9146.

  1. org.apache.kafka.clients.admin.Admin#removeMembersFromConsumerGroup has been changed to support both static or dynamic members~
  2. New cmdline option: --force for StreamsResetter is introduced, if --force specified when using the StreamsResetter, then all the active static/dynamic members will be removed.

Related KIP:
KIP-571: https://cwiki.apache.org/confluence/display/KAFKA/KIP-571%3A+Add+option+to+force+remove+members+in+StreamsResetter

Committer Checklist (excluded from commit message)

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

@feyman2016
Copy link
Copy Markdown
Contributor Author

@abbccdda @mjsax Any comments will be highly appreciated, thanks !

@feyman2016 feyman2016 requested a review from mjsax January 31, 2020 10:17
@abbccdda
Copy link
Copy Markdown

Thanks for the PR. Could you rename it to KAFKA-9146: Add ... so that it could automatically link to the JIRA? I will also leave a comment on JIRA.

@mjsax mjsax added the streams label Jan 31, 2020
@mjsax mjsax changed the title Add option to force delete active members in StreamsResetter KAFKA-9146: Add option to force delete active members in StreamsResetter Jan 31, 2020
@mjsax
Copy link
Copy Markdown
Member

mjsax commented Jan 31, 2020

I updated the PR title -- however, auto-linking only works when a new PR is opened (not when the PR title is changed...). I updated the Jira ticket to link to the PR.

private Set<LeaveGroupRequestData.MemberIdentity> members;

public RemoveMembersFromConsumerGroupOptions(Collection<MemberToRemove> members) {
public RemoveMembersFromConsumerGroupOptions(Collection<LeaveGroupRequestData.MemberIdentity> members) {
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.

This is an breaking public API change -- we can't allow this.

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.

Thanks ,mjsax !
About the public API change, Boyang, Chen has also replied in the related JIRA, let me go through it first and will update soon. :)

}

public Set<MemberToRemove> members() {
public Set<LeaveGroupRequestData.MemberIdentity> members() {
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.

This is an breaking public API change -- we can't allow this.


RemoveMembersFromConsumerGroupResult(KafkaFuture<Map<MemberIdentity, Errors>> future,
Set<MemberToRemove> memberInfos) {
Set<MemberIdentity> memberInfos) {
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.

This is an breaking public API change -- we can't allow this.

* Returns the selected member future.
*/
public KafkaFuture<Void> memberResult(MemberToRemove member) {
public KafkaFuture<Void> memberResult(MemberIdentity 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.

This is an breaking public API change -- we can't allow this.

.withRequiredArg()
.ofType(String.class)
.describedAs("file name");
forceDeleteMemberOption = optionParser.accepts("force-delete-member", "Force delete member when long session time out has been configured").withRequiredArg().ofType(Boolean.class).defaultsTo(false);
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.

This is a public API change, too.

@feyman2016
Copy link
Copy Markdown
Contributor Author

Thanks for @abbccdda to point out the title mis-written and @mjsax to update it, will make it right next time :)

@feyman2016
Copy link
Copy Markdown
Contributor Author

The related JIRA https://issues.apache.org/jira/browse/KAFKA-9146 needs a KIP and detailed plan, will mark this PR as WIP and update later on.

@feyman2016 feyman2016 changed the title KAFKA-9146: Add option to force delete active members in StreamsResetter KAFKA-9146 [WIP]: Add option to force delete active members in StreamsResetter Feb 7, 2020
private void cleanGlobal(final boolean withIntermediateTopics,
final String resetScenario,
final String resetScenarioArg) throws Exception {
private int tryCleanGlobal(final boolean withIntermediateTopics,
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.

No logic change here, just extract the main body of cleanGlobal out and put it in tryCleanGlobal.

@abbccdda
Copy link
Copy Markdown

abbccdda commented Mar 9, 2020

@feyman2016 Are we ready for another review?

@abbccdda
Copy link
Copy Markdown

@feyman2016 Is this still the PR you are working against?

@feyman2016
Copy link
Copy Markdown
Contributor Author

feyman2016 commented Apr 30, 2020

Hi, @abbccdda, sorry for the delay, I have created another PR, and request for review, thanks! https://github.com/apache/kafka/pull/8589/files

@mjsax
Copy link
Copy Markdown
Member

mjsax commented May 1, 2020

@feyman2016 closing this PR in favor of #8589 -- I assume it's a replacement.

@mjsax mjsax closed this May 1, 2020
@feyman2016
Copy link
Copy Markdown
Contributor Author

@mjsax Thanks for closing this.

@mjsax mjsax added the kip Requires or implements a KIP label Jun 12, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

kip Requires or implements a KIP streams

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants