KAFKA-5293. Do not apply exponential backoff if users have overridden… #3174

cmccabe · 2017-05-31T02:16:31Z

… reconnect.backoff.ms

ijuma

Thanks for the PR. Left a couple of comments. Also: don't we have to change the Connect defaults?

ijuma · 2017-05-31T02:18:50Z

clients/src/main/java/org/apache/kafka/common/config/AbstractConfig.java

+     * @param reconnectBackoffMax   The name of the configuraiton key for maximum reconnect backoff.
+     * @return                      The new values which have been set as described in postProcessParsedConfig.
+     */
+    protected Map<String, Object> postProcessReconnectBackoffConfigs(Map<String, Object> parsedValues,


Hmm, we shouldn't add this method here as AbstractConfig is a generic class that shouldn't know about specific configs like reconnectBackoff. We could add this method to CommonClientConfigs perhaps?

Well... it does allow you to parameterize the key names, so it is generic in that sense.

Unfortunately, we need access to AbstractConfig#originals, which is currently private. And I think one or two other private things in AbstractConfig. So we can't put this in a utility at the moment.... but at least it is protected, so it is not exposed to end-users and we could change it later.

originals() is public:

public Map<String, Object> originals() {

It's not generic if reconnectBackoff and reconnectBackoffMax are mentioned. :) Also, AbstractConfig is meant to be subclassed, so protected methods are public API and cannot be changed.

OK. I'll move it to CommonClientConfigs.

ijuma · 2017-05-31T02:19:29Z

streams/src/main/java/org/apache/kafka/streams/StreamsConfig.java

@@ -439,7 +439,7 @@
                    RECONNECT_BACKOFF_MS_DOC)
            .define(RECONNECT_BACKOFF_MAX_MS_CONFIG,
                    ConfigDef.Type.LONG,
-                    50L,
+                    1000L,


@guozhangwang @dguy Is this change OK?

I'm not sure of the original motivation behind setting it to 50. But if we are going to define it as 1000 i think it might be better to remove this and reconnect_backoff_min_ms_config definitations from here as they are the same as what is set in the ProducerConfig and ConsumerConfig, so it seem pretty pointless overriding them

When Dana implemented exponential backoff, she disabled it for Streams and Connect. I don't think there's a good reason not to enable it for those two and hence this PR. In the Streams case, I think those two configs are actually used for the StreamsKafkaClient.

asfbot · 2017-05-31T03:13:51Z

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/kafka-pr-jdk7-scala2.11/4606/
Test PASSed (JDK 7 and Scala 2.11).

asfbot · 2017-05-31T03:47:45Z

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/kafka-pr-jdk8-scala2.12/4591/
Test PASSed (JDK 8 and Scala 2.12).

asfbot · 2017-05-31T17:20:18Z

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/kafka-pr-jdk8-scala2.12/4637/
Test FAILed (JDK 8 and Scala 2.12).

asfbot · 2017-05-31T17:22:09Z

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/kafka-pr-jdk7-scala2.11/4652/
Test FAILed (JDK 7 and Scala 2.11).

asfbot · 2017-05-31T23:36:45Z

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/kafka-pr-jdk7-scala2.11/4680/
Test PASSed (JDK 7 and Scala 2.11).

asfbot · 2017-05-31T23:43:12Z

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/kafka-pr-jdk8-scala2.12/4665/
Test FAILed (JDK 8 and Scala 2.12).

cmccabe · 2017-06-01T00:32:19Z

retest this please

asfbot · 2017-06-01T01:48:38Z

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/kafka-pr-jdk8-scala2.12/4677/
Test PASSed (JDK 8 and Scala 2.12).

asfbot · 2017-06-01T01:49:58Z

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/kafka-pr-jdk7-scala2.11/4693/
Test PASSed (JDK 7 and Scala 2.11).

ijuma

Thanks for the PR, LGTM. I had just a couple of minor comments. We can merge after we decide if we should do something about them or not.

ijuma · 2017-05-31T19:20:41Z

clients/src/main/java/org/apache/kafka/clients/CommonClientConfigs.java

+        HashMap<String, Object> rval = new HashMap<>();
+        if (config.originals().containsKey(reconnectBackoffMaxKey))
+            return rval;
+        if (!config.originals().containsKey(reconnectBackoffKey))


Nit: maybe we should include this in the previous check

ijuma · 2017-06-01T15:45:32Z

clients/src/main/java/org/apache/kafka/clients/CommonClientConfigs.java

+     * @param config                    The config object.
+     * @param parsedValues              The parsedValues as provided to postProcessParsedConfig.
+     * @param reconnectBackoffKey       The name of the configuraiton key for reconnect backoff.
+     * @param reconnectBackoffMaxKey    The name of the configuraiton key for maximum reconnect backoff.


Since RECONNECT_BACKOFF_MS_CONFIG and RECONNECT_BACKOFF_MAX_MS_CONFIG are defined in this class too, is it really useful to pass the key and value from every usage? It seems like every one of them passes the same configs.

asfbot · 2017-06-01T18:36:54Z

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/kafka-pr-jdk7-scala2.11/4737/
Test FAILed (JDK 7 and Scala 2.11).

asfbot · 2017-06-01T18:37:05Z

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/kafka-pr-jdk8-scala2.12/4721/
Test FAILed (JDK 8 and Scala 2.12).

asfbot · 2017-06-01T20:03:01Z

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/kafka-pr-jdk8-scala2.12/4723/
Test PASSed (JDK 8 and Scala 2.12).

… reconnect.backoff.ms

ijuma

Thanks for the PR, LGTM. Merging to trunk and 0.11.0.

… reconnect.backoff.ms Author: Colin P. Mccabe <cmccabe@confluent.io> Reviewers: Ismael Juma <ismael@juma.me.uk> Closes #3174 from cmccabe/KAFKA-5293 (cherry picked from commit b3036c5) Signed-off-by: Ismael Juma <ismael@juma.me.uk>

asfbot · 2017-06-01T20:38:56Z

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/kafka-pr-jdk7-scala2.11/4739/
Test PASSed (JDK 7 and Scala 2.11).

asfbot · 2017-06-01T21:13:21Z

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/kafka-pr-jdk8-scala2.12/4729/
Test PASSed (JDK 8 and Scala 2.12).

asfbot · 2017-06-01T21:13:32Z

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/kafka-pr-jdk7-scala2.11/4745/
Test PASSed (JDK 7 and Scala 2.11).

ijuma reviewed May 31, 2017

View reviewed changes

cmccabe force-pushed the KAFKA-5293 branch from 41b1839 to a5c93b1 Compare May 31, 2017 17:01

cmccabe force-pushed the KAFKA-5293 branch from a5c93b1 to 43813c2 Compare May 31, 2017 22:05

ijuma approved these changes Jun 1, 2017

View reviewed changes

cmccabe force-pushed the KAFKA-5293 branch from 43813c2 to a91a427 Compare June 1, 2017 18:25

cmccabe force-pushed the KAFKA-5293 branch from a91a427 to 55b0556 Compare June 1, 2017 18:58

KAFKA-5293. Do not apply exponential backoff if users have overridden…

027ee00

… reconnect.backoff.ms

cmccabe force-pushed the KAFKA-5293 branch from 55b0556 to 027ee00 Compare June 1, 2017 20:11

ijuma approved these changes Jun 1, 2017

View reviewed changes

asfgit closed this in b3036c5 Jun 1, 2017

cmccabe deleted the KAFKA-5293 branch May 20, 2019 19:02

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

KAFKA-5293. Do not apply exponential backoff if users have overridden… #3174

KAFKA-5293. Do not apply exponential backoff if users have overridden… #3174

cmccabe commented May 31, 2017

ijuma left a comment

ijuma May 31, 2017

cmccabe May 31, 2017

ijuma May 31, 2017

cmccabe May 31, 2017

ijuma May 31, 2017

dguy May 31, 2017

ijuma May 31, 2017

asfbot commented May 31, 2017

asfbot commented May 31, 2017

asfbot commented May 31, 2017

asfbot commented May 31, 2017

asfbot commented May 31, 2017

asfbot commented May 31, 2017

cmccabe commented Jun 1, 2017

asfbot commented Jun 1, 2017

asfbot commented Jun 1, 2017

ijuma left a comment

ijuma May 31, 2017

ijuma Jun 1, 2017

asfbot commented Jun 1, 2017

asfbot commented Jun 1, 2017

asfbot commented Jun 1, 2017

ijuma left a comment

asfbot commented Jun 1, 2017

asfbot commented Jun 1, 2017

asfbot commented Jun 1, 2017

KAFKA-5293. Do not apply exponential backoff if users have overridden… #3174

KAFKA-5293. Do not apply exponential backoff if users have overridden… #3174

Conversation

cmccabe commented May 31, 2017

ijuma left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

asfbot commented May 31, 2017

asfbot commented May 31, 2017

asfbot commented May 31, 2017

asfbot commented May 31, 2017

asfbot commented May 31, 2017

asfbot commented May 31, 2017

cmccabe commented Jun 1, 2017

asfbot commented Jun 1, 2017

asfbot commented Jun 1, 2017

ijuma left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

asfbot commented Jun 1, 2017

asfbot commented Jun 1, 2017

asfbot commented Jun 1, 2017

ijuma left a comment

Choose a reason for hiding this comment

asfbot commented Jun 1, 2017

asfbot commented Jun 1, 2017

asfbot commented Jun 1, 2017