ZOOKEEPER-1998: Allow C client to throttle host name resolutions

[ztzg]

Some environments experience high DNS load because of the name resolutions introduced by ZOOKEEPER-1355.

This patch allows clients to set a minimum delay to observe between "routine" resolutions using a zoo_set_servers_resolution_delay API function.

An application can influence the rate of polling via its delay_ms parameter: when set to a value greater than zero, the client skips most "routine" resolutions which would have happened in a window of that many milliseconds since the last successful one.

Setting delay_ms to 0 disables the new logic, reverting to the default behavior. Setting it to -1 disables network resolutions during normal operation (but not, e.g., on connection loss).

[ztzg]

Hi @anmolnar, @eolivelli, @hanm,

As I saw Andor's "Releasing 3.6.0 - Pending Patches" thread on the ML: what do you think about the attached patch? Does the proposed solution look reasonable to you? And if it does, is that something you would like to have in an early 3.6?

Thanks,
Damien Diederen

[enixon]

The code looks good to me. Have you tested it in any production setting?

[enixon]

naming nit - "done" is technically accurate but doesn't capture the sense of an early finish. Alternatives?

[ztzg]

Hi @enixon, Thank you for the review, and sorry for my sluggishness.

The code looks good to me. Have you tested it in any production setting?

No. (A much simpler workaround has been used by the team which encountered the issue.) I'll try and see if I can get some more coverage, but it's always a bit of a chicken-and-egg situation.

> @@ -1037,13 +1071,13 @@ int update_addrs(zhandle_t *zh) zh->state = ZOO_NOTCONNECTED_STATE; } -fail: +done: naming nit - "done" is technically accurate but doesn't capture the sense of an early finish. Alternatives?

Here is what I currently see in zookeeper.c master: 6 abort X 6 done 3 error X 13 fail X 1 finish 1 next X 2 out So: 'finish' or 'out'? Cheers, -D

[ztzg]

@enixon: I have refreshed the PR, using finish rather than done as the goto label, as 1/ there is a precedent 2/ it seems to capture "the sense of an early finish" :)

The code has also been "stress-tested" in more situations (but not "continuously in production"), with me noticing 1/ no negative side-effects and 2/ reduced operation latencies.

[ztzg]

Dear @eolivelli, @anmolnar, @eolivelli, @hanm, @enixon, @symat,

This has been lingering for a while. How can we give it some momentum?

Cheers, -D

[ztzg]

Now thinking that @symat may be also interested in looking into this.

[ztzg]

Cc: @suhasdantkale and @mjeelanimsft, as PRs #1259 and #579 indicate that you are interested in the C client library—and in DNS-related issues in particular.

[symat]

I can not give a binding +1, but I like this patch. I like the unit test as well.
Might worth to also add to the python binding as well (in a separate Jira).

[ztzg]

@symat: Thank you for the review, and the "vote of confidence"!

Re zkpython: Noted. (As well as zkperl, for good measure.)

FYI, I have unrelated minor cleanups planned for zkpython—but everyone I know seems to be using Kazoo right now. Are you actually using zkpython?

[symat]

I am using zkpython only in some smoke test / performance tests, but never in production. (once I had to measure SSL performance and I added client SSL support to zkpython)

I don't know how important would be to keep zkpython up-to-date. I remember it was even suggested in the community to remove most of the contribs from the main ZooKeeper github repo and put it to some side repo, as not many active ZooKeeper developer is motivated to work on them. I think it is worth some time to ask around in the ZooKeeper mailing lists to see how many people are actually using zkpython or zkperl.

[ztzg]

zkperl: Well, I can tell you that we are using that one :) (I'll take advantage of this to ping @nkalmar about #1243 (comment), as he so nicely suggested :)

In general, I agree with your comment—and will ask—but I am at the same time wondering how many of the actual users actively follow the mailing list(s). I don't suppose we have something akin to the Debian Popularity Contest, do we?

While I wouldn't be opposed to moving "unpopular" bindings to their own repository, it would probably only make sense to do so if merge rules are somewhat relaxed—as I suspect it would otherwise be even more difficult to meet the "two PMC approvals" threshold.

In any case: I am willing to be automatically marked as a reviewer for (at least) the zkperl and zkpython "contribs." Do we have such a mechanism? I see that GitHub implements some such mechanisms (1, 2), but I'm not sure how applicable they are to our case. Never hesitate to ping me manually!

(Cc: @eolivelli.)

[ztzg]

Hi @anmolnar, @eolivelli, @hanm,

May I draw your attention to this patch once more? It needs a second review—good or bad—to move forward.

Cheers, -D

[eolivelli]

LGTM

[eolivelli]

Now @symat +1 is binding so the patch is good to go.
I will merge during the weekend

[ztzg]

Awesome, thanks!

[ztzg]

retest this please

[eolivelli]

retest maven build