Skip to content

in repliace env, set hosts as Array to new Mongolian#102

Open
muddydixon wants to merge 1 commit intomarcello3d:masterfrom
muddydixon:fix/replicaset
Open

in repliace env, set hosts as Array to new Mongolian#102
muddydixon wants to merge 1 commit intomarcello3d:masterfrom
muddydixon:fix/replicaset

Conversation

@muddydixon
Copy link
Copy Markdown

We have replica server info in Array

var hosts = ["localhost:27017", "localhost:27018", "localhost:27019"];

In master branch, we must set each arguments to create mongolian client to replica set, such that

var server = new Mongolian("localhost:27017", "localhost:27018", "localhost:27019");

But the number of servers are not specified in code, but are specified in config.
So in this patch, we can create mongolian client such that,

var hosts = ["localhost:27017", "localhost:27018", "localhost:27019"];
var server = new Mongolian(hosts);

@travisbot
Copy link
Copy Markdown

This pull request fails (merged 72de02d into f1a6bdc).

@travisbot
Copy link
Copy Markdown

This pull request passes (merged 72de02d into f1a6bdc).

@travisbot
Copy link
Copy Markdown

This pull request passes (merged 2ff62ea into f1a6bdc).

@marcello3d
Copy link
Copy Markdown
Owner

This solution feels a bit hacky. I think it can be simplified. What if we only handled two cases: arguments and array? Then you could have something like:

var args = arguments.length == 1 && arguments[0] instanceof Array ? arguments[0] : arguments

And the remainder of the code would be nearly identical.

Even with that, I'm not convinced this is the right solution, though. Maybe options.host should support an array instead? E.g. new Mongolian({ host:array }).

In either case, I would like to see the Mongolian constructor get simpler, not more complex.

@muddydixon
Copy link
Copy Markdown
Author

Thank you for you reply.
I consider more...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants