I'm happy to do that once we have an agreement.
On Tue, May 8, 2018 at 8:34 AM, Flavio Junqueira <[email protected]> wrote: > It might be a good idea to document whatever we end up doing. > > -Flavio > > > On 8 May 2018, at 17:22, Andor Molnar <[email protected]> wrote: > > > > "If refactoring is necessary to address the issue, then it should be part > > of the PR." > > > > Agreed. I think it is and it also makes things a lot more simpler, so > it's > > easier to review. > > > > "I'm not sure what kind of confirmation you are after here. Could you > > elaborate, please?" > > > > I'm just wondering what could have been the reason for caching host > > addresses in StaticHostProvider in the first place. > > > > "The other solution, if I remember enough of it, tried to avoid resolving > > unnecessarily, but perhaps the DNS client cache is enough..." > > > > That's exactly my point: what JDK is doing internally is perfectly fine > for > > us and we don't need extra logic here. > > > > "Could you elaborate on this point, please? What exactly do you mean with > > this statement?" > > > > See my previous point. Caching is already done in all DNS servers in the > > chain and also there's also caching in JDK already, which means by > calling > > getByName() you don't necessarily fire a DNS request, only when the TTL > is > > expired. > > > > Andor > > > > > > > > > > On Tue, May 8, 2018 at 8:12 AM, Flavio Junqueira <[email protected]> wrote: > > > >> Hi Andor, > >> > >> Thanks for your work on addressing the issue. > >> > >>> On 8 May 2018, at 16:06, Andor Molnar <[email protected]> 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 <[email protected]> > 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 <[email protected]> > >> 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 <[email protected]> > >>>>> 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 > >>>>>> > >>>>>> > >>>>>> > >>>>>> > >>>>> > >>>> > >> > >> > >
