On 4 February 2016 at 20:42, Sam Ruby <[email protected]> wrote: > On Thu, Feb 4, 2016 at 3:15 PM, sebb <[email protected]> wrote: >> On 4 February 2016 at 19:45, Sam Ruby <[email protected]> wrote: >>> On Thu, Feb 4, 2016 at 12:55 PM, sebb <[email protected]> wrote: >>>> On 4 February 2016 at 15:31, Sam Ruby <[email protected]> wrote: >>>>> Commit 60f24d0cab030cf6d64ce75c23d05b1ac33bdda7: >>>>> if all hosts are down, give up >>>>> >>>>> Branch: refs/heads/master >>>>> Author: Sam Ruby <[email protected]> >>>>> Committer: Sam Ruby <[email protected]> >>>>> Pusher: rubys <[email protected]> >>>>> >>>>> ------------------------------------------------------------ >>>>> lib/whimsy/asf/ldap.rb | ++ -- >>>>> ------------------------------------------------------------ >>>>> 4 changes: 2 additions, 2 deletions. >>>>> ------------------------------------------------------------ >>>>> >>>>> >>>>> diff --git a/lib/whimsy/asf/ldap.rb b/lib/whimsy/asf/ldap.rb >>>>> index 0376c64..f73646b 100644 >>>>> --- a/lib/whimsy/asf/ldap.rb >>>>> +++ b/lib/whimsy/asf/ldap.rb >>>>> @@ -73,7 +73,7 @@ def self.puppet_ldapservers >>>>> >>>>> # connect to LDAP >>>>> def self.connect >>>>> - loop do >>>>> + host.length.times do >>>> >>>> -1 >>>> >>>> The original code works fine. >>>> >>>> The new loop tries to loop some 37 times (depending on the size of the >>>> host name) and then fails with >>>> >>>> iteration reached an end (StopIteration) >>>> >>>> I think the original code works rather better. >>> >>> Can you explain how this works for me? >>> >>> What I see is a "loop do...end" with the only exit being on a success, >>> and the code at the end of this method being unreachable. >> >> loop/end works in conjunction with next_host which is an enum. >> enum.next generates StopIteration which is specifically caught by loop/end. >> >> http://ruby-doc.org/core-1.9.3/StopIteration.html > > That feels a bit too tightly coupled to me. Having one method's > implementation depend on another method's implementation and all that.
Seems reasonable to me - it offers plenty of flexibility in designing loops as the getter can be anywhere in the loop. Otherwise the getter has to be part of the condition checking at the start or end. > But be that as it may, I missed the fact that once you run out of your > enumeration, it is game over. That doesn't work well with long > running applications, like the board agenda. 37 failures and it is > time to restart the server. In fact it's about 7 failures (or however many are in the host list) The loop maximum was 37 (approx) but it will only try each host once. > That doesn't work for me. > >>> I will say that whether there are 4 hosts or 50 hosts, trying exactly >>> 37 times (depending on the length of an unrelated string) seems kinda >>> weird. >> >> It does it 37 or so times because host holds the host URL, and that is >> roughly its length. > > That seems totally random. > I could understand trying each host once. Which is what it does. > Or trying each host twice. Or trying something like [hosts.length, > 3].min times. But trying a number of times based on a url length? See above, 37 approx is the *max* loop count. I think you misunderstood me earlier. >> The host is the wrong variable to use. Hence the peculiar loop count. >> But so is the hosts array wrong, because that will always return the >> original number of hosts, even if some have already been used up and >> we are resuming the loop. >> The loop will then fall off the end of the enum, generating a >> StopIteration, which is not caught by the x.length.times do loop. > > I would rather next_host continuously provide hosts. OK, but this is a different design. My intention was to ensure that hosts that have already failed are not tried again. This makes sense for all the apps I have worked on. However I agree that does not sit well with long-running apps. > When it runs out > of a batch, it starts a new set. I was too quick in scanning the code > and presumed that's what the @he ||= was doing. > > Perhaps something like the following instead: > > @he = hosts.dup unless @he and not @he.empty? > @he.pop > > The net effect is that it would try each host in turn. If there later > is a failure, it will start over and loop back around if necessary > until it finds a host that works. > > Thoughts? The host re-use behaviour should be selectable. There's no point looping round all the hosts again for the one-shot apps that are going to be run again soon. If all the hosts have failed, the likelihood is that they will do so again if tried again immediately. So just give up and hope the fault has cleared for the next run. For long-running apps, the situation is a bit different. It's fine if each host is tried multiple times, so long as that happens over a long period. However there needs to be a guard against repeatedly trying all the hosts in rapid succession. So I don't think simply resetting the list will do; there needs to be a time delay. Nor does it make sense to always pause before restarting. Perhaps it would work to store the time when the list was last started, and add a wait if necessary before restarting. This would stop endless rapid recycling, but would still not be ideal behaviour e.g. if the external network was down. Is there a stage when even the long-running apps need to give up? And if so, how is that detected? Note: there was no auto retry at all until recently, so what do the long-running apps do currently? Do they try to recover from LDAP queries? If so, it seems more likely that the current design (StopIteration generated and not caught by connect) is less likely to play well with any recovery. So whatever the case, I think the change needs to be reverted. We can then improve the host re-use strategy for the long-running apps. > - Sam Ruby > >>> And failing with a StopIteration is less desirable than producing a >>> clean message, such as "Failed to connect to any LDAP host". I can >>> see the advantage of raising an exception over returning nil, but I >>> would prefer a more meaningful message. >> >> It only *fails* with StopIteration with the updated code. >> That is because the loop count is too large, so next_host is called >> more times than there are hosts. >> >> With the previous code, next_host is also called one extra time, but >> the generated StopIteration is caught by the loop/do, as designed. >> >> http://ruby-doc.org/core-1.9.3/StopIteration.html >> >> Try defining single failing URL in your .whimsy file and you will see >> what I mean. >> >>> - Sam Ruby >>> >>> >>>>> host = next_host >>>>> Wunderbar.info "Connecting to LDAP server: #{host}" >>>>> >>>>> @@ -97,8 +97,8 @@ def self.connect >>>>> Wunderbar.warn "Error connecting to LDAP server #{host}: " + >>>>> re.message + " (continuing)" >>>>> end >>>>> - >>>>> end >>>>> + >>>>> Wunderbar.error "Failed to connect to any LDAP host" >>>>> return nil >>>>> end
