Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

User defined mongo connection options API for #6958 #7277

Merged
merged 1 commit into from Jun 23, 2016

Conversation

dburles
Copy link
Contributor

@dburles dburles commented Jun 23, 2016

For #6958

I noticed the API has changed for a bunch of the existing options used in mongo_driver.js such as { safe: true } now replaced by: https://docs.mongodb.com/manual/core/replica-set-write-concern/ but that is a bit out of scope for this PR. Maybe something to be looked at though.

// set it for replSet, it will be ignored if we're not using a replSet.
mongoOptions.server.poolSize = options.poolSize;
mongoOptions.replSet.poolSize = options.poolSize;
mongoOptions.poolSize = options.poolSize;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Has this poolSize option changed? If so we should probably fix this

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Only in how you pass it in, the new mongo driver takes care of where it needs to go

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does the old code work though? It seems surprising that this passed our testing of the new driver..

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh right yeah, well as far as I've looked into things it seems like legacy options will work just fine, though I'm not totally sure what happens with safe, I think it may just be ignored as it's the default on later Mongo, but I could be wrong.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe remove this from the PR then, I think it muddies the waters a little

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure thing

@tmeasday
Copy link
Contributor

Looking good @dburles

@dburles
Copy link
Contributor Author

dburles commented Jun 23, 2016

For reference here's the relevant docs for the newer options http://mongodb.github.io/node-mongodb-native/2.1/reference/connecting/connection-settings/

@dburles dburles changed the title first pass at custom mongo connection options api for #6958 User defined mongo connection options API for #6958 Jun 23, 2016
@dburles
Copy link
Contributor Author

dburles commented Jun 23, 2016

Seems pretty good now https://github.com/meteor/meteor/pull/7277/files

/**
* @summary Allows for user specified connection options
* @locus Server
* @param {Object} options User specified Mongo connection options
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we add a link to the mongo docs so people know what these options are?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

@benjamn benjamn merged commit c87fcaa into meteor:release-1.4 Jun 23, 2016
@dburles
Copy link
Contributor Author

dburles commented Jun 25, 2016

Just a heads up, this was definitely merged, but for some reason now it's not included in the release-1.4 branch

@tmeasday
Copy link
Contributor

Any idea what happened here @benjamn?

benjamn added a commit that referenced this pull request Jun 26, 2016
User-defined Mongo.setConnectionOptions API for #6958.
@benjamn
Copy link
Contributor

benjamn commented Jun 27, 2016

Not sure why it disappeared, but I've cherry-picked it back onto release-1.4: 2654fe6

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.

None yet

3 participants