KAFKA-8503: Ignore retries config if a custom timeout is provided by huxihx · Pull Request #6913 · apache/kafka

huxihx · 2019-06-11T08:02:47Z

https://issues.apache.org/jira/browse/KAFKA-8503

When custom timeout is provided, retries config could be ignored for individual APIs in KafkaAdminClient. Tweaked code path for Call.fail to pass NPathComplexity check.

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)

huxihx · 2019-06-12T01:42:28Z

@hachikuji Please review this patch. Thanks.

huxihx · 2019-09-06T07:37:59Z

retest this please.

huxihx · 2019-09-12T05:24:01Z

@hachikuji Could you take some time to review this patch? Thanks.

hachikuji · 2019-10-15T00:30:00Z

@huxihx I wrote this KIP: https://cwiki.apache.org/confluence/display/KAFKA/KIP-533%3A+Add+default+api+timeout+to+AdminClient. Once this is approved, we can complete this work.

hachikuji · 2019-11-27T07:42:58Z

@huxihx KIP-533 has been approved: https://cwiki.apache.org/confluence/display/KAFKA/KIP-533%3A+Add+default+api+timeout+to+AdminClient. Would you like to update this patch with the proposal?

https://issues.apache.org/jira/browse/KAFKA-8503 The KIP-533 implementation.

huxihx · 2019-11-29T23:56:06Z

@hachikuji Please review this patch. Thanks.

hachikuji

Thanks. Left a few comments. It would be helpful to have a basic unit or integration test.

hachikuji · 2019-12-03T23:01:16Z

                                                           + "than 1/3 of that value. It can be adjusted even lower to control the expected time for normal rebalances.";

+    public static final String DEFAULT_API_TIMEOUT_MS_CONFIG = "default.api.timeout.ms";
+    public static final String DEFAULT_API_TIMEOUT_MS_DOC = "Specifies the timeout (in milliseconds) for client APIs that could block. " +


We might need to generalize this description a little bit. The AdminClient APIs do not block.

Simply dropping could block is a good option?

Yeah, that sounds fine.

hachikuji · 2019-12-03T23:04:14Z

     */
-    public Integer timeoutMs() {
-        return timeoutMs;
+    public Integer apiTimeoutMs() {


These changes are incompatible; we need to keep the existing API. I think it's fine to update the javadoc.

hachikuji · 2019-12-03T23:16:06Z

-        this.defaultTimeoutMs = config.getInt(AdminClientConfig.REQUEST_TIMEOUT_MS_CONFIG);
+        this.defaultTimeoutMs = config.getInt(AdminClientConfig.DEFAULT_API_TIMEOUT_MS_CONFIG);
+        // Normally, the default api timeout should be equal or larger than the request timeout.
+        this.requestTimeoutMs = Math.min(defaultTimeoutMs, config.getInt(AdminClientConfig.REQUEST_TIMEOUT_MS_CONFIG));


So our options here are either to raise an error to the user or adjust one of the configurations. Since default.api.timeout.ms is a new configuration, it is possible that a user has explicitly provided a request.timeout.ms which conflicts with the default default.api.timeout.ms. I think the logic should be something like the following:

If a default.api.timeout.ms has been explicitly specified, raise an error if it conflicts with request.timeout.ms.

If no default.api.timeout.ms has been configured, then set its value as the max of the default and request.timeout.ms. Also we should probably log a warning.

Otherwise, use the provided values for both configurations.

huxihx · 2019-12-06T09:20:03Z

retest this please.

hachikuji

Thanks for the updates. Left one more comment. Note we still need test a test case which verifies basic timeout behavior.

hachikuji · 2019-12-07T23:20:12Z

                }
                Call call = calls.remove(0);
-                int timeoutMs = calcTimeoutMsRemainingAsInt(now, call.deadlineMs);
+                int timeoutMs = Math.min(requestTimeoutMs, calcTimeoutMsRemainingAsInt(now, call.deadlineMs));


In addition to passing through the timeout to createRequest, we also need to pass it through in newClientRequest. Only some of the APIs support an explicit request timeout. This appears to have been a pre-existing bug.

huxihx · 2019-12-11T09:33:13Z

testHandleTimeout which is now disabled due to flaky failures seems to be a good starting point for testing the timeout behavior. Do we need a test case which should cover the new-added config or the request timeout or even both?

hachikuji · 2019-12-12T01:59:34Z

Mainly we need a test case which verifies that the config is actually respected. We don't have to do this for every API. I would say pick one and verify 1) that the default api timeout works, and 2) that it can be overridden by the Options.

huxihx · 2019-12-13T01:01:30Z

retest this please.

huxihx · 2019-12-16T07:06:09Z

retest this please.

huxihx · 2019-12-17T01:55:29Z

retest this please.

huxihx · 2019-12-19T07:28:46Z

retest this please.

hachikuji · 2019-12-30T19:31:24Z

@huxihx I opened a PR into your branch with some of the missing test cases. Please take a look: huxihx#1.

hachikuji · 2020-01-27T22:37:27Z

@huxihx Do you have time to follow up here? I would like to get this merged for 2.5 if possible.

hachikuji · 2020-01-30T23:30:50Z

@huxihx Closing this. We will merge #8011 with co-author attribution. Thanks for the contribution!

This PR implements `default.api.timeout.ms` as documented by KIP-533. This is a rebased version of #6913 with some additional test cases and small cleanups. Reviewers: David Arthur <mumrah@gmail.com> Co-authored-by: huxi <huxi_2b@hotmail.com>

huxihx force-pushed the KAFKA-8503 branch from 2f98cbe to 23ef8a9 Compare September 6, 2019 02:18

huxihx added 2 commits November 28, 2019 15:54

rebase KafkaAdminClient.java

e10a12d

KAFKA-8503: Ignore retries config if a custom timeout is provided

9c6856e

https://issues.apache.org/jira/browse/KAFKA-8503 The KIP-533 implementation.

huxihx force-pushed the KAFKA-8503 branch from 5a98175 to 9c6856e Compare November 29, 2019 05:57

hachikuji reviewed Dec 3, 2019

View reviewed changes

addressed Jason's comments

b84363e

hachikuji reviewed Dec 7, 2019

View reviewed changes

test case added

4f61615

hachikuji mentioned this pull request Jan 28, 2020

KAFKA-8503; Add default api timeout to AdminClient (KIP-533) #8011

Merged

3 tasks

hachikuji closed this Jan 30, 2020

Conversation

huxihx commented Jun 11, 2019

Committer Checklist (excluded from commit message)

Uh oh!

huxihx commented Jun 12, 2019

Uh oh!

huxihx commented Sep 6, 2019

Uh oh!

huxihx commented Sep 12, 2019

Uh oh!

hachikuji commented Oct 15, 2019

Uh oh!

hachikuji commented Nov 27, 2019

Uh oh!

huxihx commented Nov 29, 2019

Uh oh!

hachikuji left a comment

Choose a reason for hiding this comment

Uh oh!

hachikuji Dec 3, 2019

Choose a reason for hiding this comment

Uh oh!

huxihx Dec 4, 2019

Choose a reason for hiding this comment

Uh oh!

hachikuji Dec 4, 2019

Choose a reason for hiding this comment

Uh oh!

hachikuji Dec 3, 2019

Choose a reason for hiding this comment

Uh oh!

hachikuji Dec 3, 2019

Choose a reason for hiding this comment

Uh oh!

huxihx commented Dec 6, 2019

Uh oh!

hachikuji left a comment

Choose a reason for hiding this comment

Uh oh!

hachikuji Dec 7, 2019

Choose a reason for hiding this comment

Uh oh!

huxihx commented Dec 11, 2019

Uh oh!

hachikuji commented Dec 12, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

huxihx commented Dec 13, 2019

Uh oh!

huxihx commented Dec 16, 2019

Uh oh!

huxihx commented Dec 17, 2019

Uh oh!

huxihx commented Dec 19, 2019

Uh oh!

hachikuji commented Dec 30, 2019

Uh oh!

hachikuji commented Jan 27, 2020

Uh oh!

hachikuji commented Jan 30, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

hachikuji commented Dec 12, 2019 •

edited

Loading