Hi team, Anyone else has thoughts about whether we should use checked or unchecked exception here?
Thanks, Andor On Thu, May 10, 2018 at 8:19 AM, Andor Molnar <[email protected]> wrote: > Interesting idea. What difference you think it could make comparing to > checked? > > > > On Wed, May 9, 2018 at 8:01 AM, Flavio Junqueira <[email protected]> wrote: > >> I'm actually now wondering whether we should be using an unchecked >> exception instead. A lot of things have changed with exception handling >> since we wrote this code base initially. An unchecked exception would >> actually match better my current mental model of what that signature should >> look like. >> >> -Flavio >> >> > On 9 May 2018, at 16:44, Flavio Junqueira <[email protected]> wrote: >> > >> > I like the idea of indicating to the application that there is >> something wrong with the list of servers so that it has a chance to look >> into it. With the current code in `ClientCnxn`, we will log at warn level >> and hope that someone sees it, but we are not really stopping the client. >> Throwing might actually be an improvement as it will output a log message, >> but I'm now wondering if we should propagate it all the way to the >> application. Responding to myself, one reason for not doing it is that it >> is not a fatal error unless no server can be resolved. >> > >> > -Flavio >> > >> >> 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. Please tell me if there's a good historical >> >> reason for that. >> >> >> >> 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. >> >> 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. >> >> >> >> 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 solution is already built-in in JDK and DNS clients in the right >> way >> >> - 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 >> >>>>> >> >>>>> >> >>>>> >> >>>>> >> >>>> >> >>> >> > >> >> >
