KAFKA-6789: Handle COORDINATOR_LOAD_IN_PROGRESS, COORDINATOR_NOT_AVAILABLE retriable errors in AdminClient API

[omkreddy]

• Handle COORDINATOR_LOAD_IN_PROGRESS, COORDINATOR_NOT_AVAILABLE retriable errors in
  AdminClient describe groups, delete Groups, list Offsets API

Committer Checklist (excluded from commit message)

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

[omkreddy]

retest this please

[viktorsomogyi]

I think handleRetriableError is a bit misleading. I mean it handles both retriable and non-retriable error. From this perspective the old naming was better (from my perspective).

[shunge]

@viktorsomogyi now this error handle method also applied on describeConsumerGroups,so the old naming does not fit perfectly? But I do agree the naming is a little misleading.

[viktorsomogyi]

This is very similar to handleRetriableError, perhaps there is an opportunity to use a common code.

[viktorsomogyi]

Hi @omkreddy , if you don't mind, I've reviewed it too and left a few comments.

[shunge]

NIT: extra whitespace

[shunge]

NIT: same, whitespace

[shunge]

This is the same, under the hood:

    public boolean hasError() {
        return this.error != Errors.NONE;
    }

So I think you can keep this?

[omkreddy]

@hachikuji pinging for review

[ijuma]

@cmccabe can you please review this?

[cmccabe]

AdminClient already has retry logic for consumer group requests.

However, there are a few cases that we don't handle. One is where the leader has moved between the call to findCoordinator and our attempt to describe the group that the coordinator owns. This patch doesn't fix that case either, though.

This patch replaces a lot of checks for specific errors with checks to see if something inherits from RetriableException. As I've commented before on other JIRAs, I don't think this is a good idea for AdminClient. Specific exceptions have to be handled in a specific way based on what they indicate. For example, if there is a load in progess, maybe we need to back off for a little bit until it completes. If the coordinator has moved, we should try to send to the new coordinator. Just checking for what inherits from what tends to lead to the wrong behavior because people don't think about what each specific error case means and how it should be handled.

Also, what is the rationale for renaming COORDINATOR_LOADING_IN_PROGRESS to COORDINATOR_LOAD_IN_PROGRESS?

I can see that the patch adds retry logic to deleteConsumer groups. However, I don't understand why we would want to do this. It seems like all the errors for deleting groups are not retryable and related to things like authorization, etc.

In general I think we should probably drop this PR and rethink the approach. It would be particularly useful to know what specific errors people want to handle that aren't handled now.

[omkreddy]

@cmccabe This PR implementation is consistent with existing code (listConsumerGroups), where we try for COORDINATOR_LOAD_IN_PROGRESS, COORDINATOR_NOT_AVAILABLE errors.
Yes, this PR does not solve NOT_COORDINATOR errors (where leader moved).
This is an attempt to maintain consistent behaviour across all methods calls.

There is no rename of COORDINATOR_LOAD_IN_PROGRESS, I just updated the comment with correct enum name.

In deleteConsumer groups also, We are only retrying retriable exceptions, not all exceptions.

This PR implementation approach is same as #5595 PR.

[cmccabe]

This PR implementation is consistent with existing code (listConsumerGroups), where we try for COORDINATOR_LOAD_IN_PROGRESS, COORDINATOR_NOT_AVAILABLE errors.

I agree that we should make listGroups and describeGroups consistent. What I disagree with is that I don't think we should be checking whether something is a RetriableException. We should list what to do with each particular exception type.

Yes, this PR does not solve NOT_COORDINATOR errors (where leader moved). This is an attempt to maintain consistent behaviour across all methods calls.

Understood. We certainly don't have to solve every problem in this PR. However, I think that the current name of the PR, "Add retry logic to AdminClient consumer group requests" is misleading, since it implies that there is no existing retry logic.

There is no rename of COORDINATOR_LOAD_IN_PROGRESS, I just updated the comment with correct enum name.

That's true -- my mistake. Your change just fixes a comment.

This PR implementation approach is same as #5595 PR.

The approach in #5595 is to enumerate specific errors and handle them as needed. That's different than handling all subclasses of RetriableException the same way.

[omkreddy]

ok..I tried to extract duplicate code to a common method. this me made to handle all RetriableExceptions in same way. I will try to revert some of the changes and will try to handle specific errors .

[omkreddy]

@cmccabe @hachikuji Updated the PR to handle COORDINATOR_LOAD_IN_PROGRESS, COORDINATOR_NOT_AVAILABLE retriable errors in consumer group API. This may help in fixing few flaky tests.

[hachikuji]

Thanks for the patch. Just a couple comments.

[hachikuji]

We can probably drop this comment. I filed https://issues.apache.org/jira/browse/KAFKA-8341 for retrying NOT_COORDINATOR errors since that is a bit more work.

[omkreddy]

@hachikuji Thanks for the review. Updated the PR, Please take a look.

[hachikuji]

LGTM. Thanks for the patch. I'll merge after the build completes.