Skip to content

KAFKA-8341. Retry Consumer group operation for NOT_COORDINATOR error#6723

Merged
cmccabe merged 2 commits into
apache:trunkfrom
soondenana:KAFKA-8341
May 25, 2019
Merged

KAFKA-8341. Retry Consumer group operation for NOT_COORDINATOR error#6723
cmccabe merged 2 commits into
apache:trunkfrom
soondenana:KAFKA-8341

Conversation

@soondenana

Copy link
Copy Markdown
Contributor

An api call for consumer groups is made up of two calls:

  1. Find the consumer group coordinator
  2. Send the request to the node found in step 1

But the coordinator can get moved between step 1 and 2. In that case we
currently fail. This change fixes that by detecting this error and then
retrying.

Following APIs are impacted by this behavior:

  1. listConsumerGroupOffsets
  2. deleteConsumerGroups
  3. describeConsumerGroups

Each of these call result in AdminClient making multiple calls to the backend.
As AdminClient code invokes each backend api in a separate event loop, the code
that detects the error (step 2) need to restart whole operation including
step 1. This needed a change to capture the "Call" object for step 1 in
step 2.

This change thus refactors the code to make it easy to perform a retry of
whole operation. It creates a Context object to capture the api arguments
that can then be referred by each "Call" objects. This is just for convenience
and makes method signature simpler as we only need to pass one object instead
of multiple api arguments.

The creation of each "Call" object is done in a new method, so we can
easily resubmit step 1 in step 2.

This change also modifies corresponding unit test to test this scenario.

More detailed description of your change,
if necessary. The PR title and PR message become
the squashed commit message, so use a separate
comment to ping reviewers.

Summary of testing strategy (including rationale)
for the feature or bug fix. Unit and/or integration
tests are expected for any behaviour change and
system tests should be considered for larger changes.

Committer Checklist (excluded from commit message)

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

@soondenana

Copy link
Copy Markdown
Contributor Author

retest this please

1 similar comment
@soondenana

Copy link
Copy Markdown
Contributor Author

retest this please

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.

Since we're only using a batch size of one, we should be able to just verify that we got a list of size 1 and it contained what we wanted, right? Also, we should also try to be robust against malformed responses that don't include the group we asked for (this will currently throw an NPE if we get one of those).

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.

Done. After adding the check one unit test failed, I have fixed it (need to delete some code) and added another unit test that incorporates the deleted code and tests it in correct way. Please take a look at that.

@cmccabe

cmccabe commented May 23, 2019

Copy link
Copy Markdown
Contributor

Thanks, @soondenana. I still wish for an implementation of KAFKA-6788, but this is a nice step in the right direction, and a much smaller change than that would be :) I left a comment.

An api call for consumer groups is made up of two calls:
1. Find the consumer group coordinator
2. Send the request to the node found in step 1

But the coordinator can get moved between step 1 and 2. In that case we
currently fail. This change fixes that by detecting this error and then
retrying.

Following APIs are impacted by this behavior:
1. listConsumerGroupOffsets
2. deleteConsumerGroups
3. describeConsumerGroups

Each of these call result in AdminClient making multiple calls to the backend.
As AdminClient code invokes each backend api in a separate event loop, the code
that detects the error (step 2) need to restart whole operation including
step 1. This needed a change to capture the "Call" object for step 1 in
step 2.

This change thus refactors the code to make it easy to perform a retry of
whole operation. It creates a Context object to capture the api arguments
that can then be referred by each "Call" objects. This is just for convenience
and makes method signature simpler as we only need to pass one object instead
of multiple api arguments.

The creation of each "Call" object is done in a new method, so we can
easily resubmit step 1 in step 2.

This change also modifies corresponding unit test to test this scenario.
@soondenana

Copy link
Copy Markdown
Contributor Author

Messed up when adding a new commit to take care of Colin's comments. Reopening it.

@soondenana soondenana reopened this May 23, 2019

@abbccdda abbccdda left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Thanks for the PR. Just some minor comments

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

nit: also add java doc for type T, O here

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.

Done.

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Do we have NPE risk here?

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.

findCoordinator request should take care of this and we should not have a null/invalid node.

@soondenana
soondenana force-pushed the KAFKA-8341 branch 2 times, most recently from 5ad7991 to 2f47682 Compare May 23, 2019 18:38
…rGroups

Taking care of review comment from Colin where the code was fetching
group from return value of DescribeConsumerGroup call. If the group for
some reason isn't present, then the code would have thrown NPE. This
change checks for that and fails gracefully.
@soondenana

Copy link
Copy Markdown
Contributor Author

Thanks for the PR. Just some minor comments

Thanks Boyang for taking a look. Added the javadoc and updated the second commit where you should be able to see the change (I have left original commit as is).

@soondenana

Copy link
Copy Markdown
Contributor Author

retest this please

@cmccabe
cmccabe merged commit 46a02f3 into apache:trunk May 25, 2019
@soondenana
soondenana deleted the KAFKA-8341 branch May 25, 2019 00:25
haidangdam pushed a commit to haidangdam/kafka that referenced this pull request May 29, 2019
…pache#6723)

An API call for consumer groups must send a FindCoordinatorRequest to find the consumer group coordinator, and then send a follow-up request to that node.  But the coordinator might move after the FindCoordinatorRequest but before the follow-up request is sent.  In that case we currently fail.

This change fixes that by detecting this error and then retrying.  This fixes listConsumerGroupOffsets, deleteConsumerGroups, and describeConsumerGroups.

Reviewers: Colin P. McCabe <cmccabe@apache.org>, Boyang Chen <bchen11@outlook.com>
pengxiaolong pushed a commit to pengxiaolong/kafka that referenced this pull request Jun 14, 2019
…pache#6723)

An API call for consumer groups must send a FindCoordinatorRequest to find the consumer group coordinator, and then send a follow-up request to that node.  But the coordinator might move after the FindCoordinatorRequest but before the follow-up request is sent.  In that case we currently fail.

This change fixes that by detecting this error and then retrying.  This fixes listConsumerGroupOffsets, deleteConsumerGroups, and describeConsumerGroups.

Reviewers: Colin P. McCabe <cmccabe@apache.org>, Boyang Chen <bchen11@outlook.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants