MONGOID-5758: Add Mongoid.reconnect_clients and improve forking webserver documentation #5808
Conversation
- Add Mongoid.reconnect_clients (analogous to Mongoid.disconnect_clients). The reason for adding this is to simply web server hooks (see added docs.) - Corrects the @return in the docs for disconnect_clients. Also added specs for the existing behavior. - Updates documentation related to web server forking.
johnnyshields
changed the title
|
FYI I've verified the documentation and changes here in my production Puma app. |
lib/mongoid.rb
Outdated
| # @example Reconnect all active clients. | ||
| # Mongoid.reconnect_clients | ||
| # | ||
| # @return [ Array<Mongo::Client> ] The driver clients. |
There was a problem hiding this comment.
Hey @johnnyshields -- here and elsewhere, do you think there's value in actually returning the list of clients? I don't believe the current return value is intentional (or even useful). I think I'd rather leave the documented return value as true (or at least "truthy"), rather than be committed to always returning a list of clients here.
There was a problem hiding this comment.
@jamis the documentation should match what the method does, and today the method returns the list of clients. It is certainly fine to raise another PR and change the return value, I doubt anyone is relying on this behavior today.
There was a problem hiding this comment.
You're not wrong, Johnny. However, it's also fair to say that the method returns truthy, which is what the documentation today (poorly) implies. I much prefer a truthy return value, than telling the world to expect it to return a list of clients. There are other ways to get the list of clients, and I'd really rather this not be one of them.
There was a problem hiding this comment.
OK so would you like me to change the method return behavior of both disconnect (existing) and reconnect (new) in this PR?
There was a problem hiding this comment.
reconnect should mirror the behavior of disconnect. I'm honestly more worried about the documentation (and the contract it exposes) than the actual implementation, but as long as we're not promising clients more than we want to deliver (e.g. exposing the client list as a return value here), I'm okay.
There was a problem hiding this comment.
@jamis this is done--both disconnect and reconnect now return true.
…rver documentation (mongodb#5808) * This PR does the following: - Add Mongoid.reconnect_clients (analogous to Mongoid.disconnect_clients). The reason for adding this is to simply web server hooks (see added docs.) - Corrects the @return in the docs for disconnect_clients. Also added specs for the existing behavior. - Updates documentation related to web server forking. * Fix method name * More terse syntax * Preserve old return type * Update configuration.txt * Update configuration.txt
This PR does the following:
Mongoid.reconnect_clientsmethod analogous toMongoid.disconnect_clients. The reason for adding this is to simplify web server hooks (see added docs.)@returnin the docs fordisconnect_clients. Also added specs which cover the existing behavior.This will hopefully help others avoid issues we saw in our production.
See also: mongodb/mongo-ruby-driver#2856