Transactions + assorted improvements #27

Merged
merged 5 commits into from Aug 20, 2012

Projects

None yet

3 participants

@aleksj
aleksj commented Aug 19, 2012
  • support for transactions using the TransactionChain
  • general refactoring and code simplification around new_cursor
  • a more graceful protocol based on timeouts for attempting reconnection to DB servers (it used to flood the logs with reconnects)
  • some typo-fixing and formatting
@FSX
Owner
FSX commented Aug 20, 2012

I'll review it once I get a chance. Once it cools down (the weather) I'll be able to do some stuff. _

@FSX FSX commented on the diff Aug 20, 2012
momoko/clients.py
@@ -26,6 +26,9 @@ class BlockingClient(object):
def __init__(self, settings):
self._pool = BlockingPool(**settings)
+ def __del__(self):
@FSX
FSX Aug 20, 2012 Owner

Hasn't been added to AsyncClient.

@aleksj
aleksj Aug 20, 2012

I tried it, but it doesn't work. There might be a cyclic relationship somewhere, you might know this better?

@FSX FSX commented on the diff Aug 20, 2012
momoko/pools.py
"""
if len(self._pool) > self.max_conn:
- raise PoolError('connection pool exausted')
- conn = AsyncConnection(self._ioloop)
-
- if new_cursor_args:
- new_cursor_args.append(conn)
- callbacks = [
- partial(self._pool.append, conn),
- partial(self.new_cursor, *new_cursor_args)]
+ self._clean_pool()
+ if len(self._pool) > self.max_conn:
+ raise PoolError('connection pool exhausted')
+ timeout = self._last_reconnect + .25 # 1/4 second delay between reconnection
+ timenow = time.time()
+ if timenow > timeout or len(self._pool) <= self.min_conn:
@FSX
FSX Aug 20, 2012 Owner

I don't understand this line. len(self._pool) <= self.min_conn is ok. There's no need to limit the connection that are needed to reach the minimum, but what about timenow > timeout? Are there cases when this is true? W

hy would the creation of connections be delayed? To run a batch of queries the same amount of connections are needed and sometimes new connections need to be created for this.

@aleksj
aleksj Aug 20, 2012

Imagine that a connection goes down (it happened in my testing). Within seconds, there were megabytes of log entries and a barrage of reconnection attempts. I'm not saying the timeout should be very long, but it's good practice. I'm quite open to other protocols, this one is about as simple as one can go.

@FSX
FSX Aug 20, 2012 Owner

Hmm, I didn't know that happens. Do you also know the cause of the
reconnection attempts? Maybe the connection times out.

I implemented a maximum amount of 5 attempts to get a new connection
in the rewrite (located in this repo in the rewrite branch).

@aleksj
aleksj Aug 20, 2012

I'd prefer progressively increasing timeouts (up to a maximum, say 30s) - rather than timing out and having to restart the server.

@FSX
FSX Aug 20, 2012 Owner

But only for connections that are lost. Not for connections that are requested by execute/callproc/chain/batch.

I'll do some research about keeping connections alive. Sockets can time out and if that's prevented there's no need for a complicated reconnection mechanism.

@aleksj
aleksj Aug 20, 2012

What if the DB server goes down? One has to reconnect, and
socket-level logic won't do.

@FSX
FSX Aug 20, 2012 Owner

Is it possible to record the exceptions in your log? It sounds like your using Momoko in a production environment and that would give us a better view on what's happening.

    if connection is not None:
        try:
            connection.cursor(function, function_args, callback, cursor_kwargs)
            return
        except (DatabaseError, InterfaceError) as e:  # Recover from lost connection
            logging.warning('Requested connection was closed; exception: %r'  % e)
            self._pool.remove(connection)

Would be cool. :D

@FSX
FSX Aug 20, 2012 Owner

That's true. The reconnect mechanism shouldn't be completely gone. I was just wondering about why reconnect would happen.

The the pool cleaner could also be used for checking the health of the connections.

@aleksj
aleksj Aug 20, 2012

You want me to push this change into my pull request? Any other changes?

@FSX
FSX Aug 20, 2012 Owner

Hmm, no. I'll merge it and test it.

@FSX FSX merged commit 3e487d0 into FSX:master Aug 20, 2012
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment