Code review comment for lp:~allenap/storm/oneiric-admin-shutdown-bug-871596

Revision history for this message
Gavin Panella (allenap) wrote :

> Thanks for working on this!
>
> So the only time you ever saw the 57P01 error was with pgbouncer? That is
> interesting as in the past we often never saw the error codes at all (hence
> the string matching).

Yeah, I don't fully understand what was happening there.

>
> The code seems good. Just a few suggestions and points for discussion:
>
> - Rather than check just for 57P01, I think we might as well check for the
> following list:
>
> pg_connection_failure_codes = frozenset([
> '08006', # CONNECTION FAILURE
> '08001', # SQLCLIENT UNABLE TO ESTABLISH SQLCONNECTION
> '08004', # SQLSERVER REJECTED ESTABLISHMENT OF SQLCONNECTION
> '53300', # TOO MANY CONNECTIONS
> '57000', # OPERATOR INTERVENTION
> '57014', # QUERY CANCELED
> '57P01', # ADMIN SHUTDOWN
> '57P02', # CRASH SHUTDOWN
> '57P03', # CANNOT CONNECT NOW
> ])

Tip top, done.

>
> - Should we document installing python-pgbouncer using the Python egg from
> pypi rather than checking out the Bazaar branch and using a symlink hack?
>
> - Should python-fixtures be an optional dependency? Maybe if it gets used
> elsewhere.
>
> - Do we want python-pgbouncer and fixtures as optional dependencies for
> running the PostgreSQL tests? I understand not needing the environment setup
> for testing every database backend, but there seems little point for allowing
> people to run a subset of the tests for a particular DB backend.

I've switched everything to use setuptools, and have defined all the
test dependencies in setup.py. I've switched make check over to use
setuptools' testing support, and I've also made the ./test script use
the eggs that setuptools downloads. There's also an additional develop
Makefile target that downloads the test dependencies without running
the tests.

This means you can do make check from a fresh branch of Storm and run
*all* the tests without further intervention (other than doing the
one-off package installations and database set-up). This has already
shown me that there's another Django disconnection test failure that I
need to address.

« Back to merge proposal