MINOR: Use timeout config in KafkaAdminClient

[dengziming]

More detailed description of your change
Currently we use a fixed value 3600000 as requestTimeout when creating NetworkClient in KafkaAdminClient, it's better to use the request.timeout.ms config like KafkaConsumer and KafkaProducer.

In fact, this change has no effect unless request.timeout.ms is set to a value greater than 3600000.

Summary of testing strategy (including rationale)

Committer Checklist (excluded from commit message)

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

[dengziming]

Hello @dajac, this is a small finding when comparing KafkaConsumer and KafkaAdminClient, In fact, I'm not very confident about this change. PTAL, thank you.

[OneCricketeer]

Should this be a long like the other timeouts?

[dengziming]

It seems we don't make it very clear about when to use Int and when to use Long, but request.timeout.ms is defined as Integer, the code can be found in :

.define(REQUEST_TIMEOUT_MS_CONFIG,
    Type.INT,
    30000,
    atLeast(0),
    Importance.MEDIUM,
    REQUEST_TIMEOUT_MS_DOC)

[showuon]

@dengziming , nice catch! Could we add a test for it? There are some tests in KafkaAdminClientTest that tests request timeout. So maybe you can refer to them to set a small request timeout when creating kafkaAdminClient, and then try to send a request and sleep for the request timeout value to assert there'll be timeout exception thrown. Thanks.

[dajac]

I wonder why we did this in the first place. Is it because the request timeout is handled by the admin client itself? If this is the case, it might not be a good idea to change the request timeout passed to the selector.

[showuon]

@dajac , looks like you're right. KAFKA-5324 changed the value from reqeust timeout config value to 1 hour, to have per-call timeouts. In this case, we should not change it.

[dengziming]

Yes, it's handled by the admin client itself, I will recheck this logic, thank you @dajac @showuon

[showuon]

@dengziming , I'm thinking if we should add some comment to this 1 hour setting, so that it won't confuse other developers next time. WDYT?

[dengziming]

This is a good idea @showuon, I will add some comment to explain why we use different setting here compared to KafkaConsumer and KafkaProducer.

[dengziming]

Hello @dajac @showuon, I found that we set this to a large enough value in KAFKA-5324 so that we can overwrite per-call timeout with a lager value by AbstractOptions. I updated the comments and you can have a look.

However, in KAFKA-8503(KIP-533, #8011) we change the AbstractOptions timeout from request.timeout.ms to default.api.timeout.ms, this means we no longer need to overwrite request.timeout.ms, but need to overwrite default.api.timeout.ms, so maybe we can change it back to REQUEST_TIMEOUT_MS_CONFIG, what do you think? @dajac @showuon

[github-actions]

This PR is being marked as stale since it has not had any activity in 90 days. If you
would like to keep this PR alive, please leave a comment asking for a review. If the PR has
merge conflicts, update it with the latest from the base branch.

If you are having difficulty finding a reviewer, please reach out on the [mailing list](https://kafka.apache.org/contact).

If this PR is no longer valid or desired, please feel free to close it. If no activity occurs in the next 30 days, it will be automatically closed.

[github-actions]

This PR has been closed since it has not had any activity in 120 days. If you feel like this
was a mistake, or you would like to continue working on it, please feel free to re-open the
PR and ask for a review.