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.
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.
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.
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?
> The host is the wrong variable to use.
> 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. 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?
- 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
