FIX #103 - Implementing discovered servers API #143
Conversation
| @@ -797,6 +801,23 @@ Client.prototype.processInbound = function() { | |||
| if (client.checkTLSMismatch() === true) { | |||
| return; | |||
| } | |||
|
|
|||
| // Always try to read the connect_urls from info | |||
| if(client.info.connect_urls && client.info.connect_urls.length > 0) { | |||
There was a problem hiding this comment.
First, indentation looks strange.
Second, are you not going to do that for any protocol message received by the client? Looks to me that you should ensure that protocol being processed is an INFO here.
Also, not sure if you support randomization, but what the other clients do is randomize (if allowed) the array or connect_urls prior to add the URLs to the server pool.
There was a problem hiding this comment.
I preserved the indentation that was in the file :)
The block we are in is only handling INFO messages. See https://sourcegraph.com/github.com/nats-io/node-nats@b5db73ee06428471568a730c3b0ff7f907fa6943/-/blob/lib/nats.js#L798:1
There was a problem hiding this comment.
Even with this link, I am sorry but it looks horrible. It is very difficult to know what is in which block.
lib/nats.js
Outdated
| client.info.connect_urls.forEach(function(server) { | ||
| var u = 'nats://' + server; | ||
| if(known.indexOf(u) === -1) { | ||
| client.addServer(u); |
There was a problem hiding this comment.
Does this do the right thing with randomizing or not the list?
There was a problem hiding this comment.
Just to be clear, we don't want to randomize the pool when we add things after the initial connect. We can randomize the received URLs before adding to the pool, but not randomize the entire pool after initial connect.
test/dyncluster.js
Outdated
| }); | ||
| }); | ||
|
|
||
| it('setting protocol zero doesnt update', function(done) { |
There was a problem hiding this comment.
Why do we want to expose and let client decide? Is that how we do it in other clients, they can suppress if they want to?
test/dyncluster.js
Outdated
| // add two servers | ||
| process.nextTick(function () { | ||
| var others = nsc.add_member_with_delay([port + 1, port + 2], route_port, 250, function () { | ||
| should(others.length).be.equal(2); |
There was a problem hiding this comment.
We should write test to check randomize etc. And not randomize.
aricart
force-pushed
the
discovered-servers
branch
from
dec737a
to
175d828
Compare
|
@kozlovic @derekcollison I have put in the changes. |
|
LGTM, let's have @kozlovic give a +1 as well. |
lib/nats.js
Outdated
| }); | ||
|
|
||
| if(toAdd.length > 0) { | ||
| shuffle(toAdd); |
There was a problem hiding this comment.
If I am not mistaken, this client has a noRandomize option. If so, you should shuffle the incoming array only if noRandomize === false.
Added test Extended nats_server_control to allow easy creation of a cluster Bumped versions
aricart
force-pushed
the
discovered-servers
branch
from
de189da
to
6fd8397
Compare
|
@kozlovic I made the shuffling of the discovered servers honor noRandomize, and added a related test. |
|
LGTM. Not sure why we get so many coveralls report (I deleted some from this page). |
|
There were some git errors during checkout for one of the builds.
/a
On May 5, 2017 at 1:15:27 PM, Ivan Kozlovic (notifications@github.com) wrote:
LGTM. Not sure why we get so many coveralls report (I deleted some from
this page).
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
<#143 (comment)>, or mute
the thread
<https://github.com/notifications/unsubscribe-auth/AA_DEFFvQu3xvemwEV4HUR5W0H8vQR1qks5r24NegaJpZM4NRIGg>
.
|
Extended nats_server_control to allow easy creation of a cluster
Bumped versions