-
Notifications
You must be signed in to change notification settings - Fork 73
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
Add Write Concern support #108
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM in general, just one quick comment
if ('undefined' !== typeof concern.w) this.options.w = concern.w; | ||
if ('undefined' !== typeof concern.wtimeout) this.options.wtimeout = concern.wtimeout; | ||
} else { | ||
this.options.w = 'm' === concern ? 'majority' : concern; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure I like supporting 'm' as a shortcut for 'majority'. Is there some precedent for this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
mquery ReadPreference .read()
support abbrevation.
https://github.com/aheckmann/mquery/blob/45c4bd104613da47828df13d4aec12c324525c53/lib/mquery.js#L1583-L1596
In my previous request, I make readConcern()
also support abbrevation. m
= majority
https://github.com/aheckmann/mquery/blob/45c4bd104613da47828df13d4aec12c324525c53/lib/mquery.js#L1640-L1653
So I make writeConcern()
to accept 'm' as shortcut as well.
I doubt if it is ok to support m
in write concern too. If the tag name happens to be m
, it will be replaced with majority
.
For consistency, maybe we should make a breaking change to not support m
as alias of majority
in read concern either? Only read preference accept alias names.
In read concern, l
=local
, lz
= linearizable
. I think lz
is not good abbreviation but I can't find good alternative.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah you're right, this is more consistent with the readConcern and read APIs, let's merge this
Add write concern helpers
w writeConcern
j
wtimeout wTimeout
The special case is that write concern accepts
object
and it will parse the object to the underlying options correctly. Also,.w('m')
is alias of.w('majority')
. Mongoose does not has this implementation.Add
r
as alias ofreadConcern
as
w
is forwriteConcern
Add
setReadPreference
as alias ofread
mquery
use.read()
to set read preference. However,node-mongodb-native
has different meaningIMO, this method is for internal use. I think it's okay to overwrite this behavior.
node-mongodb-native
uses setReadPreference to specify read preference.Add
hint
testAdd findOneAndDelete alias of findOneAndRemove