Refactor multiple writeConcern helpers into a single `applyWriteConcern' method
#1680
Conversation
| target.retryWrites = true; | ||
| } | ||
|
|
||
| if (options.w != null || options.j != null || options.fsync != null) { |
There was a problem hiding this comment.
Under this behavior, if the user overrides either w, j, or fsync, we will ignore the entire default write concern.
For example if the collection has a writeConcern with w and j defined, and the user specifies a new value for w, our new writeConcern will only have w, not j.
Is that expected behavior?
Also, should the user be able to specify a new writeConcern value for wtimeout only?
There was a problem hiding this comment.
I agree with you, but at the same time this isn't new behavior, rather a merging of existing writeConcern methods. I've created this ticket to track that: https://jira.mongodb.org/browse/NODE-1412
| @@ -256,7 +257,8 @@ var UnorderedBulkOperation = function(topology, collection, options) { | |||
| : 1000; | |||
|
|
|||
| // Get the write concern | |||
| var writeConcern = common.writeConcern(shallowClone(options), collection, options); | |||
| var writeConcern = applyWriteConcern(shallowClone(options), { collection: collection }, options); | |||
There was a problem hiding this comment.
Will we use a retryWrites value if it is assigned here?
There was a problem hiding this comment.
that's a good point, but at the same time we never did (retryWrites didn't used to be part of the old writeConcern for bulk, it was only in the one in Collection). Truly shows the extent how this fragmentation as hurt us, and also how correct my note is in the function (e.g. setting retryWrites needs to be set in a much better place)
writeConcern has been supported since 2.6, we only need to check for server support in aggregation's $out in which case support was deferred to 3.4
We presently have many similar methods laying around the codebase to deal with applying a write concern to some command. This has become a problem during transactions development because we need to make some decisions about whether to apply based on transaction status - and requiring changing this logic in seven locations is error-prone, and outright annoying.