Issue 4

[werkshy]

Fix for issue #4, single commit on top of previous pull request.

[danielhfrank]

Can we not import * here? We're pretty strict here at Bitly about explicit imports

[werkshy]

That must be an IDE setting. I will find it and kill it.

[danielhfrank]

So, I actually think that we should be leaving the lookupd response logic as it was. We never really want to disconnect from a nsqd if it is absent from a lookupd response, because the correct client behavior is to connect to the union of all the responses from the lookupds that it queries, in case existence of a producer has not propagated to all the lookupds. We also guard against attempting to make duplicate connections by checking if a connection already exists inside the connectToNsqd method, using the putIfAbsent method of the ConcurrentHashMap of connections. I didn't get a chance to look at the test code added, but I imagine it can be tweaked a little to ensure that we are behaving correctly. Let me know if you have any questions, thanks!

[werkshy]

Ah, I missed this in the old version, which makes a world of difference:

if(stored != null){

return;
}

I see your point about not removing a connection just because a single
lookupd does not contain the producer. The spec for drivers that Matt was
working on suggests that servers /should/ be disconnected if removed from
the lookupd responses. Perhaps we should query all lookupds at once to
create the union of all producers, then decide which need to be
added/dropped. I could adapt my patch to do that if you agree.

Andy

On Tue, Apr 16, 2013 at 5:04 PM, Dan Frank [email protected] wrote:

So, I actually think that we should be leaving the lookupd response logic
as it was. We never really want to disconnect from a nsqd if it is absent
from a lookupd response, because the correct client behavior is to connect
to the union of all the responses from the lookupds that it queries, in
case existence of a producer has not propagated to all the lookupds. We
also guard against attempting to make duplicate connections by checking if
a connection already exists inside the connectToNsqd method, using the
putIfAbsent method of the ConcurrentHashMap of connections. I didn't get
a chance to look at the test code added, but I imagine it can be tweaked a
little to ensure that we are behaving correctly. Let me know if you have
any questions, thanks!

—
Reply to this email directly or view it on GitHubhttps://github.com//pull/5#issuecomment-16471677
.

[danielhfrank]

Hi @werkshy , trying to do a little summer cleaning around here. I think there's some valuable stuff in this PR, but as I said above I don't think that the lookupd logic should be changed. I may spend some time pulling out some changes from here individually, so keep your eyes peeled

[mreiferson]

👻