From dev-return-69506-archive-asf-public=cust-asf.ponee.io@zookeeper.apache.org Tue May 8 21:09:00 2018 Return-Path: X-Original-To: archive-asf-public@cust-asf.ponee.io Delivered-To: archive-asf-public@cust-asf.ponee.io Received: from mail.apache.org (hermes.apache.org [140.211.11.3]) by mx-eu-01.ponee.io (Postfix) with SMTP id 33A5918063B for ; Tue, 8 May 2018 21:09:00 +0200 (CEST) Received: (qmail 33602 invoked by uid 500); 8 May 2018 19:08:59 -0000 Mailing-List: contact dev-help@zookeeper.apache.org; run by ezmlm Precedence: bulk List-Help: List-Unsubscribe: List-Post: List-Id: Reply-To: dev@zookeeper.apache.org Delivered-To: mailing list dev@zookeeper.apache.org Received: (qmail 33585 invoked by uid 99); 8 May 2018 19:08:58 -0000 Received: from pnap-us-west-generic-nat.apache.org (HELO spamd1-us-west.apache.org) (209.188.14.142) by apache.org (qpsmtpd/0.29) with ESMTP; Tue, 08 May 2018 19:08:58 +0000 Received: from localhost (localhost [127.0.0.1]) by spamd1-us-west.apache.org (ASF Mail Server at spamd1-us-west.apache.org) with ESMTP id 17746CB2B0 for ; Tue, 8 May 2018 19:08:58 +0000 (UTC) X-Virus-Scanned: Debian amavisd-new at spamd1-us-west.apache.org X-Spam-Flag: NO X-Spam-Score: 1.899 X-Spam-Level: * X-Spam-Status: No, score=1.899 tagged_above=-999 required=6.31 tests=[DKIM_SIGNED=0.1, DKIM_VALID=-0.1, DKIM_VALID_AU=-0.1, HTML_MESSAGE=2, RCVD_IN_DNSWL_NONE=-0.0001, SPF_PASS=-0.001] autolearn=disabled Authentication-Results: spamd1-us-west.apache.org (amavisd-new); dkim=pass (2048-bit key) header.d=cloudera.com Received: from mx1-lw-us.apache.org ([10.40.0.8]) by localhost (spamd1-us-west.apache.org [10.40.0.7]) (amavisd-new, port 10024) with ESMTP id uAMVFID-udbH for ; Tue, 8 May 2018 19:08:56 +0000 (UTC) Received: from mail-ot0-f181.google.com (mail-ot0-f181.google.com [74.125.82.181]) by mx1-lw-us.apache.org (ASF Mail Server at mx1-lw-us.apache.org) with ESMTPS id DB4835FAD2 for ; Tue, 8 May 2018 19:08:55 +0000 (UTC) Received: by mail-ot0-f181.google.com with SMTP id 15-v6so22538251otn.12 for ; Tue, 08 May 2018 12:08:55 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=cloudera.com; s=google; h=mime-version:in-reply-to:references:from:date:message-id:subject:to; bh=66nukE4ihLbof6W/pYFC32hfT7lPy8xR5v/AY3j0ATY=; b=FZdJJnlHE/q9bXkjAGzaoz9cvpBx2iVKpP/9w93gJuQaivvAjJkndsXxe0V8I+22ad NEA7jX6sCiN8744IKpD4npIxVjtuMYqFSDH2u/VbzdlNjIrvtZ17i3R0MLqGSvRqTSbg Jz4l7LVnoPErbzOOsAwz6cZdavxT6Gm0EdQzFs7LH98PE6mbozl1++tITy7Ee8b03NJ+ eXpyYeoFWCyCoES7TOKi5KVYQq6L0ohNsT1AEzbOKTtTCuGRWZlnwwMOm9CPgBhhKIsB 2LEZ87oCGsKrGo+ogGmE/ZcdbYExF/S650d0LfswUk1EJIJpBDIDcTNqHVLbNm0576vf d7zA== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:in-reply-to:references:from:date :message-id:subject:to; bh=66nukE4ihLbof6W/pYFC32hfT7lPy8xR5v/AY3j0ATY=; b=RmDQ1ZPMDHGDmCB91+8ajvp1I4QtQ4Mv2ffD83QdXy0/PJUqJ23ShTaxpAIBUxvkFZ RNTP/7b+Kc8YjoPkrEfb+WIPZouGcxNCo4Tci27i1kkeo6Bp1MPmh6qmk4wOH5ywBgZQ 3bzhvIzbW5gTT4ngmDHenJfVIUatFNluefwaMXHZIha7/8ZUKLGrgXD+uCZZBhA7rHnP eQlavxIisyZLJK6mTk2s3DZJ+J0nKQWDXU5NqcQHc29XF3CSQ97fqc7mwYfDSfcsWuFK GwJLweKY/P8KN72Gcfc4qK1tuUEia43knEpnrQjZomejJUDPuz8wnGfFibUyv1mQjX+6 6PYw== X-Gm-Message-State: ALQs6tCMu+uIDCQuh9MrPfmqWHKm7m/1CGbsrlE4TERWahwS2npRSh2y 3xy5KdPfFeiwb1UtO2sOwurDZPdnoBXrhv59SFVzSm9/ X-Google-Smtp-Source: AB8JxZrmyaW/qkPXmshnWRNSxNO/kCO0Fef7OTz7epttXG476EI9Pt/Tk2fg3diFcdmDdSOdgtqd2pXRLstdtC8qwxc= X-Received: by 2002:a9d:9b8:: with SMTP id q53-v6mr10695393otd.162.1525806529312; Tue, 08 May 2018 12:08:49 -0700 (PDT) MIME-Version: 1.0 Received: by 2002:a9d:fc3:0:0:0:0:0 with HTTP; Tue, 8 May 2018 12:08:48 -0700 (PDT) In-Reply-To: <11875FB1-4445-4C47-974C-8A1A9F2B20B6@apache.org> References: <4ABFF6D3-35FA-4FF9-8CB1-E82A0C2AB1E4@apache.org> <11875FB1-4445-4C47-974C-8A1A9F2B20B6@apache.org> From: Andor Molnar Date: Tue, 8 May 2018 12:08:48 -0700 Message-ID: Subject: Re: Name resolution in StaticHostProvider To: dev@zookeeper.apache.org Content-Type: multipart/alternative; boundary="0000000000002d2289056bb68457" --0000000000002d2289056bb68457 Content-Type: text/plain; charset="UTF-8" I'm happy to do that once we have an agreement. On Tue, May 8, 2018 at 8:34 AM, Flavio Junqueira wrote: > It might be a good idea to document whatever we end up doing. > > -Flavio > > > On 8 May 2018, at 17:22, Andor Molnar 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 wrote: > > > >> Hi Andor, > >> > >> Thanks for your work on addressing the issue. > >> > >>> On 8 May 2018, at 16:06, Andor Molnar 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 > 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 > >> 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 > >>>>> 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 > >>>>>> > >>>>>> > >>>>>> > >>>>>> > >>>>> > >>>> > >> > >> > > --0000000000002d2289056bb68457--