Hi Andor, Thanks for your work on addressing the issue.
> On 8 May 2018, at 16:06, Andor Molnar <an...@cloudera.com> wrote: > > Hi, > > Updating this thread, because the PR is still being review on GitHub. > > So, the reason why I refactored the original behaviour of > StaticHostProvider is that I believe that it's trying to do something which > is not its responsibility. If refactoring is necessary to address the issue, then it should be part of the PR. Otherwise, it is better to refactor in a separate PR. > Please tell me if there's a good historical > reason for that. I'm not sure what kind of confirmation you are after here. Could you elaborate, please? > > My approach is giving the user the following to options: > 1- Use static IP addresses, if you don't want to deal with DNS resolution > at all - we guarantee that no DNS logic will involved in this case at all. Sounds fine. > 2- Use DNS hostnames if you have a reliable DNS service for resolution > (with HA, secondary servers, backups, etc.) - we must use DNS in the right > way in this case e.g. do NOT cache IP address for a longer period that DNS > server allows to and re-resolve after TTL expries, because it's mandatory > by protocol. Sounds fine. > > My 2 cents here: > - the fix which was originally posted for re-resolution is a workaround and > doesn't satisfy the requirement for #2, The other solution, if I remember enough of it, tried to avoid resolving unnecessarily, but perhaps the DNS client cache is enough to avoid the unnecessary round-trips. This observation actually brings me to the next point: > - the solution is already built-in in JDK and DNS clients in the right way Could you elaborate on this point, please? What exactly do you mean with this statement? > - can't see a reason why we shouldn't use that > > I checked this in some other projects as well and found very similar > approach in hadoop-common's SecurityUtil.java. It has 2 built-in plugins > for that: > - Standard resolver uses java's built-in getByName(). > - Qualified resolver still uses getByName(), but adds some logic to avoid > incorrect re-resolutions and reverse IP lookups. > > Please let me know your thoughts. > > Regards, > Andor > > > > > > > On Tue, Mar 6, 2018 at 8:12 AM, Andor Molnar <an...@cloudera.com> wrote: > >> Hi Abe, >> >> Unfortunately we haven't got any feedback yet. What do you think of >> implementing Option #3? >> >> Regards, >> Andor >> >> >> On Thu, Feb 22, 2018 at 6:06 PM, Andor Molnar <an...@cloudera.com> wrote: >> >>> Did anybody happen to take a quick look by any chance? >>> >>> I don't want to push this too hard, because I know it's a time consuming >>> topic to think about, but this is a blocker in 3.5 which has been hanging >>> around for a while and any feedback would be extremely helpful to close it >>> quickly. >>> >>> Thanks, >>> Andor >>> >>> >>> >>> On Mon, Feb 19, 2018 at 12:18 PM, Andor Molnar <an...@cloudera.com> >>> wrote: >>> >>>> Hi all, >>>> >>>> We need more eyes and brains on the following PR: >>>> >>>> https://github.com/apache/zookeeper/pull/451 >>>> >>>> I added a comment few days ago about the way we currently do DNS name >>>> resolution in this class and a suggestion on how we could simplify things a >>>> little bit. We talked about it with Abe Fine, but we're a little bit unsure >>>> and cannot get a conclusion. It would be extremely handy to get more >>>> feedback from you. >>>> >>>> To add some colour to it, let me elaborate on the situation here: >>>> >>>> In general, the task that StaticHostProvider does is to get a list of >>>> potentially unresolved InetSocketAddress objects, resolve them and iterate >>>> over the resolved objects by calling next() method. >>>> >>>> *Option #1 (current logic)* >>>> - Resolve addresses with getAllByName() which returns a list of IP >>>> addresses associated with the address. >>>> - Cache all these IP's, shuffle them and iterate over. >>>> - If client is unable to connect to an IP, remove all IPs from the list >>>> which the original servername was resolved to and re-resolve it. >>>> >>>> *Option #2 (getByName())* >>>> - Resolve address with getByName() instead which returns only the first >>>> IP address of the name, >>>> - Do not cache IPs, >>>> - Shuffle the *names* and resolve with getByName() *every time* when >>>> next() is called, >>>> - JDK's built-in caching will prevent name servers from being flooded >>>> and will do the re-resolution automatically when cache expires, >>>> - Names with multiple IPs will be handled by DNS servers which (if >>>> configured properly) return IPs in different order - this is called DNS >>>> Round Robin -, so getByName() will return different IP on each call. >>>> >>>> *Options #3* >>>> - There's a small problem with option#2: if DNS server is not configured >>>> properly and handles the round-robin case in a way that it always return >>>> the IP list in the same order, getByName() will never return the next ip, >>>> - In order to overcome that, use getAllByName() instead, shuffle the >>>> list and return the first IP. >>>> >>>> All feedback if much appreciated. >>>> >>>> Thanks, >>>> Andor >>>> >>>> >>>> >>>> >>> >>
