On Dec 2, 11:17 am, Chuck Remes <[email protected]> wrote: > I've been experimenting with the sharding features in Sequel and ran > across a use-case in our system that isn't covered. > > Due to the nature of the keys we are using to shard, we do not always > know all possible shard keys in advance. The Sequel#connect call > requires the :servers hash to be complete; there isn't any mechanism > to modify it after calling #connect.
True. > I know it is possible to call Sequel::ConnectionPool#disconnect and > then reconnect with a modified :servers hash, but I am troubled by the > documentation for #disconnect. > > > Removes all connection currently available on all servers, > > optionally yielding each connection to the given block. This method > > has the effect of disconnecting from the database, assuming that no > > connections are currently being used. > > In a threaded system the call to #disconnect might always orphan some > connections because they are in use by another thread elsewhere. A new > call to #connect would (???) throw a Sequel::DatabaseConnectionError. > If I am wrong about this, I would love to be corrected. No. Only available connections are disconnected. Any active connections will not be disconnected, and will be returned to the pool when they are no longer active. So while disconnect is safe and doesn't orphan connections, it also only completely disconnects from the database if there are no active connections to it. > If I am right about this race condition, then I would like to propose > a small modification to the API. Here is a (untested) monkeypatch that > would add this functionality. I put a '+' next to each line containing > new code so it is easy to see (I am not showing a few lines of deleted > code). Not a big deal, but in the future, please include a full diff, preferably as a pastie so it is easy to apply. > If I were to create tests for this and submit as a patch, would this > kind of functionality be accepted? Is there a superior way to add a > new sharding server to the connection pool at runtime that I have > missed? No, your implementation is generally sound, but I see a few issues: 1) The add_server_key internals should be protected by a @mutex.synchronize block, otherwise it is vulnerable to a race condition. 2) You need to make sure that the :default server always exists. 3) You need to protect against adding an already present server, otherwise add_server with an already existing key will orphan connections. You can probably modify the database's options to have an existing server key use new options, but you can't modify the connection pool keys. 4) Database#add_server uses :server instead of :servers as the option key, and I would probably have it accept a hash instead of multiple options. I would also change ConnectionPool#add_server to add_servers and have it take an array of symbols. If you fix those issues and add documentation and specs, I'll include this as long as it passes the test suite. Your post got me thinking about changes to ConnectionPool#disconnect: 1) Start accepting an options hash, for a :servers option, for only disconnecting those servers. 2) Have it start checking allocated connections, marking the threads with a flag. When the connection is released, the flag is checked, and if present, instead of the connection being returned to the pool, it is disconnected. This would allow calling disconnect even when there are active connections, knowing that any active connections will be disconnected as soon as they are finished. This would also allow add_server to change existing servers, by disconnecting, and having new connections go to the new server. I'd like to get some feedback on those ideas. Thanks, Jeremy -- You received this message because you are subscribed to the Google Groups "sequel-talk" group. To post to this group, send email to [email protected]. To unsubscribe from this group, send email to [email protected]. For more options, visit this group at http://groups.google.com/group/sequel-talk?hl=en.
