You are viewing a plain text version of this content. The canonical link for it is here.
Posted to jira@kafka.apache.org by GitBox <gi...@apache.org> on 2022/02/10 13:10:19 UTC

[GitHub] [kafka] dengziming opened a new pull request #11746: MINOR: Use timeout config in KafkaAdminClient

dengziming opened a new pull request #11746:
URL: https://github.com/apache/kafka/pull/11746


   *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)
   


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [kafka] dengziming commented on pull request #11746: MINOR: Use timeout config in KafkaAdminClient

Posted by GitBox <gi...@apache.org>.
dengziming commented on pull request #11746:
URL: https://github.com/apache/kafka/pull/11746#issuecomment-1037750296


   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`, so maybe we can change it back to `REQUEST_TIMEOUT_MS_CONFIG`, what do you think? @dajac @showuon 


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [kafka] showuon commented on pull request #11746: MINOR: Use timeout config in KafkaAdminClient

Posted by GitBox <gi...@apache.org>.
showuon commented on pull request #11746:
URL: https://github.com/apache/kafka/pull/11746#issuecomment-1036126819


   @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?


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [kafka] OneCricketeer commented on a change in pull request #11746: MINOR: Use timeout config in KafkaAdminClient

Posted by GitBox <gi...@apache.org>.
OneCricketeer commented on a change in pull request #11746:
URL: https://github.com/apache/kafka/pull/11746#discussion_r803897385



##########
File path: clients/src/main/java/org/apache/kafka/clients/admin/KafkaAdminClient.java
##########
@@ -524,7 +524,7 @@ static KafkaAdminClient createInternal(AdminClientConfig config, TimeoutProcesso
                 config.getLong(AdminClientConfig.RECONNECT_BACKOFF_MAX_MS_CONFIG),
                 config.getInt(AdminClientConfig.SEND_BUFFER_CONFIG),
                 config.getInt(AdminClientConfig.RECEIVE_BUFFER_CONFIG),
-                (int) TimeUnit.HOURS.toMillis(1),
+                config.getInt(AdminClientConfig.REQUEST_TIMEOUT_MS_CONFIG),

Review comment:
       Should this be a long like the other timeouts? 




-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [kafka] dengziming closed pull request #11746: MINOR: Use timeout config in KafkaAdminClient

Posted by GitBox <gi...@apache.org>.
dengziming closed pull request #11746:
URL: https://github.com/apache/kafka/pull/11746


   


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [kafka] dengziming commented on a change in pull request #11746: MINOR: Use timeout config in KafkaAdminClient

Posted by GitBox <gi...@apache.org>.
dengziming commented on a change in pull request #11746:
URL: https://github.com/apache/kafka/pull/11746#discussion_r804334534



##########
File path: clients/src/main/java/org/apache/kafka/clients/admin/KafkaAdminClient.java
##########
@@ -524,7 +524,7 @@ static KafkaAdminClient createInternal(AdminClientConfig config, TimeoutProcesso
                 config.getLong(AdminClientConfig.RECONNECT_BACKOFF_MAX_MS_CONFIG),
                 config.getInt(AdminClientConfig.SEND_BUFFER_CONFIG),
                 config.getInt(AdminClientConfig.RECEIVE_BUFFER_CONFIG),
-                (int) TimeUnit.HOURS.toMillis(1),
+                config.getInt(AdminClientConfig.REQUEST_TIMEOUT_MS_CONFIG),

Review comment:
       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)
   ```




-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [kafka] showuon commented on pull request #11746: MINOR: Use timeout config in KafkaAdminClient

Posted by GitBox <gi...@apache.org>.
showuon commented on pull request #11746:
URL: https://github.com/apache/kafka/pull/11746#issuecomment-1036034155


   @dajac , looks like you're right. [KAFKA-5324](https://issues.apache.org/jira/browse/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. 


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [kafka] dengziming edited a comment on pull request #11746: MINOR: Use timeout config in KafkaAdminClient

Posted by GitBox <gi...@apache.org>.
dengziming edited a comment on pull request #11746:
URL: https://github.com/apache/kafka/pull/11746#issuecomment-1037750296


   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 


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [kafka] dengziming commented on pull request #11746: MINOR: Use timeout config in KafkaAdminClient

Posted by GitBox <gi...@apache.org>.
dengziming commented on pull request #11746:
URL: https://github.com/apache/kafka/pull/11746#issuecomment-1034906293


   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.


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [kafka] dengziming edited a comment on pull request #11746: MINOR: Use timeout config in KafkaAdminClient

Posted by GitBox <gi...@apache.org>.
dengziming edited a comment on pull request #11746:
URL: https://github.com/apache/kafka/pull/11746#issuecomment-1037750296


   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 


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [kafka] dajac commented on pull request #11746: MINOR: Use timeout config in KafkaAdminClient

Posted by GitBox <gi...@apache.org>.
dajac commented on pull request #11746:
URL: https://github.com/apache/kafka/pull/11746#issuecomment-1036029927


   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.


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [kafka] dengziming commented on pull request #11746: MINOR: Use timeout config in KafkaAdminClient

Posted by GitBox <gi...@apache.org>.
dengziming commented on pull request #11746:
URL: https://github.com/apache/kafka/pull/11746#issuecomment-1037750296


   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`, so maybe we can change it back to `REQUEST_TIMEOUT_MS_CONFIG`, what do you think? @dajac @showuon 


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [kafka] dengziming commented on pull request #11746: MINOR: Use timeout config in KafkaAdminClient

Posted by GitBox <gi...@apache.org>.
dengziming commented on pull request #11746:
URL: https://github.com/apache/kafka/pull/11746#issuecomment-1036847160


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


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [kafka] dengziming commented on pull request #11746: MINOR: Use timeout config in KafkaAdminClient

Posted by GitBox <gi...@apache.org>.
dengziming commented on pull request #11746:
URL: https://github.com/apache/kafka/pull/11746#issuecomment-1036037690


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


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org