Fix DNS cache logic and update default from 1s to 60s #1491

dacjames · 2017-10-27T22:09:14Z

To my eyes, the dns caching logic was not fully implemented.

This converts to using rd_clock() and sets the rkb_t_rsal_last timestamp after the value is loaded.

I also adjust the default dns cache time to 60s. 1s is very low in practice; when you have a large number of clients, a low DNS caching time leads to a flood of DNS requests when a broker is restarted. Could split out the config change if necessary

edenhill

The fix looks good, but the default change needs to go

edenhill · 2017-10-28T07:29:27Z

src/rdkafka_broker.c

@@ -596,17 +596,18 @@ rd_kafka_broker_send (rd_kafka_broker_t *rkb, rd_slice_t *slice) {
 static int rd_kafka_broker_resolve (rd_kafka_broker_t *rkb) {
 	const char *errstr;

+	rd_ts_t now = rd_clock();


No need for temp variable here, only used once.

If that’s your preference. As someone who recently spent a lot of time reading librdkafka from a place of relative ignorance, more temporary variables would have been very helpful.

edenhill · 2017-10-28T07:35:09Z

src/rdkafka_conf.c

@@ -284,7 +284,7 @@ static const struct rd_kafka_property rd_kafka_properties[] = {
 	  _RK(broker_addr_ttl),
 	  "How long to cache the broker address resolving "
          "results (milliseconds).",
-	  0, 86400*1000, 1*1000 },
+	  0, 86400*1000, 60*1000 },


DNS caching is performed by the resolver (typically libc) based on the records TTL setting, adding another layer of caching is inadvisable and will slow down the propagation of DNS record changes.

The initial default of broker.address.ttl was actually 5 minutes, but was decreased to 1s for this reason:
a0aff23
#494

The current default of 1s provides a bit of dampening, rather than caching.
A user that wants to dampen further can increase the configuration property.

edenhill · 2017-10-28T07:37:12Z

src/rdkafka_broker.c

@@ -624,6 +625,8 @@ static int rd_kafka_broker_resolve (rd_kafka_broker_t *rkb) {
                                             "Failed to resolve '%s': %s",
                                             rkb->rkb_nodename, errstr);
 			return -1;
+		} else {
+			rkb->rkb_t_rsal_last = rd_clock();


use 8 whitespace tabs.

Do you mean to use tabs instead of spaces? The block above was inconsistent (the rd_kafka_broker_fail call is indented with spaces) so I wasn't sure what to do.

librdkafka has mixed indents due to weird historical reasons, but all new code, and any changes to existing code, should use 8 whitespaces for tabs.

dacjames · 2017-10-29T20:29:41Z

DNS caching is performed by the resolver (typically libc) based on the records TTL setting.

That’s not true. In general, libc does not perform DNS caching and major Linux distributions do not enable it by default.

I’ll remove it if you want but the current default is bad and will overload DNS servers during broker maintenance once you have a few hundred clients.

A more robust form of backoff when restablishing the broker connection may be a better solution to the DNS flooding problem, though. Even with a longer DNS cache, the current retry loop makes decommissioning a broker from a large, live system impossible.

dacjames · 2017-10-30T16:52:02Z

@edenhill Applied changes per your review.

Tried to fix the whitespace but ending up confirming that it is already using tabs.

tabs

edenhill

small pr technicalities, then we're good to go!

edenhill · 2017-11-06T23:29:32Z

src/rdkafka_broker.c

@@ -624,6 +625,8 @@ static int rd_kafka_broker_resolve (rd_kafka_broker_t *rkb) {
                                             "Failed to resolve '%s': %s",
                                             rkb->rkb_nodename, errstr);
 			return -1;
+		} else {
+			rkb->rkb_t_rsal_last = rd_clock();


librdkafka has mixed indents due to weird historical reasons, but all new code, and any changes to existing code, should use 8 whitespaces for tabs.

edenhill · 2017-11-06T23:30:00Z

src/rdkafka_conf.c

@@ -284,7 +284,7 @@ static const struct rd_kafka_property rd_kafka_properties[] = {
 	  _RK(broker_addr_ttl),
 	  "How long to cache the broker address resolving "
          "results (milliseconds).",
-	  0, 86400*1000, 1*1000 },


avoid unneccessary changes (for git blame to be convenient)

edenhill · 2017-11-06T23:30:30Z

CONFIGURATION.md

@@ -27,7 +27,7 @@ socket.receive.buffer.bytes              |  *  | 0 .. 100000000  |             0
 socket.keepalive.enable                  |  *  | true, false     |         false | Enable TCP keep-alives (SO_KEEPALIVE) on broker sockets <br>*Type: boolean*
 socket.nagle.disable                     |  *  | true, false     |         false | Disable the Nagle algorithm (TCP_NODELAY). <br>*Type: boolean*
 socket.max.fails                         |  *  | 0 .. 1000000    |             3 | Disconnect from broker when this number of send failures (e.g., timed out requests) is reached. Disable with 0. NOTE: The connection is automatically re-established. <br>*Type: integer*
-broker.address.ttl                       |  *  | 0 .. 86400000   |          1000 | How long to cache the broker address resolving results (milliseconds). <br>*Type: integer*
+broker.address.ttl                       |  *  | 0 .. 86400000   |         60000 | How long to cache the broker address resolving results (milliseconds). <br>*Type: integer*


this file was not re-generated after you changed the default back so it still contains the original change.
Remove from PR

dacjames · 2017-11-07T02:26:19Z

@edenhill I think this resolves said technicalities.

edenhill · 2017-11-07T15:45:06Z

Big thanks for this!

Fix DNS cache logic and update default from 1s to 60s

6a8e107

edenhill suggested changes Oct 28, 2017

View reviewed changes

Daniel Woodlins added 2 commits October 30, 2017 09:45

Remove local now variable

9762c42

Revert broker.address.ttl default to 1s

d1cc661

edenhill suggested changes Nov 6, 2017

View reviewed changes

Address style issues for PR

4de4b97

dacjames force-pushed the feature/dns-cache branch from 631cf94 to 4de4b97 Compare November 7, 2017 02:24

edenhill approved these changes Nov 7, 2017

View reviewed changes

edenhill merged commit 2dae1f3 into confluentinc:master Nov 7, 2017

dacjames deleted the feature/dns-cache branch November 7, 2017 18:59

akhi3030 pushed a commit to akhi3030/librdkafka that referenced this pull request Nov 13, 2017

Fix DNS cache logic broker.address.ttl (confluentinc#1491, @dacjames)

8ffde73

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix DNS cache logic and update default from 1s to 60s #1491

Fix DNS cache logic and update default from 1s to 60s #1491

dacjames commented Oct 27, 2017

edenhill left a comment

edenhill Oct 28, 2017

dacjames Oct 29, 2017

edenhill Oct 28, 2017

edenhill Oct 28, 2017

dacjames Oct 29, 2017

edenhill Nov 6, 2017

dacjames commented Oct 29, 2017

dacjames commented Oct 30, 2017

edenhill left a comment

edenhill Nov 6, 2017

edenhill Nov 6, 2017

edenhill Nov 6, 2017

dacjames commented Nov 7, 2017

edenhill commented Nov 7, 2017

Fix DNS cache logic and update default from 1s to 60s #1491

Fix DNS cache logic and update default from 1s to 60s #1491

Conversation

dacjames commented Oct 27, 2017

edenhill 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

dacjames commented Oct 29, 2017

dacjames commented Oct 30, 2017

edenhill 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

dacjames commented Nov 7, 2017

edenhill commented Nov 7, 2017