launchpad-reviewers team mailing list archive

[William Grant <me@xxxxxxxxxxxxxxxxxx>]

William Grant has proposed merging lp:~wgrant/launchpad/amputate-more-ztm into lp:launchpad.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)

For more details, see:
https://code.launchpad.net/~wgrant/launchpad/amputate-more-ztm/+merge/75894

This branch rips out half of the remaining public ZopelessTransactionManager methods. conn, registerSynch and unregisterSynch were shims still used in only one or two places. set_isolation_level was used only in a test, so has been turned into a function in the test module.
-- 
https://code.launchpad.net/~wgrant/launchpad/amputate-more-ztm/+merge/75894
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~wgrant/launchpad/amputate-more-ztm into lp:launchpad.

=== modified file 'cronscripts/librarian-gc.py'
--- cronscripts/librarian-gc.py	2010-04-27 19:48:39 +0000
+++ cronscripts/librarian-gc.py	2011-09-18 11:10:29 +0000
@@ -17,9 +17,11 @@
 import _pythonpath
 import logging
 
+from canonical.config import config
+from canonical.database.sqlbase import ISOLATION_LEVEL_AUTOCOMMIT
+from canonical.launchpad.database.librarian import LibraryFileAlias
+from canonical.launchpad.interfaces.lpstorm import IStore
 from canonical.librarian import librariangc
-from canonical.database.sqlbase import ISOLATION_LEVEL_AUTOCOMMIT
-from canonical.config import config
 from lp.services.scripts.base import LaunchpadCronScript
 
 
@@ -57,14 +59,16 @@
                 help="Skip expiring aliases with an expiry date in the past."
                 )
 
-
     def main(self):
         librariangc.log = self.logger
 
         if self.options.loglevel <= logging.DEBUG:
             librariangc.debug = True
 
-        conn = self.txn.conn()
+        # XXX wgrant 2011-09-18 bug=853066: Using Storm's raw connection
+        # here is wrong. We should either create our own or use
+        # Store.execute or cursor() and the transaction module.
+        conn = IStore(LibraryFileAlias)._connection._raw_connection
 
         # Refuse to run if we have significant clock skew between the
         # librarian and the database.

=== modified file 'lib/canonical/database/ftests/script_isolation.py'
--- lib/canonical/database/ftests/script_isolation.py	2010-07-16 08:54:42 +0000
+++ lib/canonical/database/ftests/script_isolation.py	2011-09-18 11:10:29 +0000
@@ -41,13 +41,6 @@
 check()
 txn.uninstall()
 
-# We run the checks twice to ensure that both methods of setting the
-# isolation level stick across transaction boundaries.
 txn = initZopeless(isolation=ISOLATION_LEVEL_SERIALIZABLE)
 check()
 txn.uninstall()
-
-txn = initZopeless()
-txn.set_isolation_level(ISOLATION_LEVEL_SERIALIZABLE)
-check()
-

=== modified file 'lib/canonical/database/ftests/test_isolation.py'
--- lib/canonical/database/ftests/test_isolation.py	2011-09-06 05:18:33 +0000
+++ lib/canonical/database/ftests/test_isolation.py	2011-09-18 11:10:29 +0000
@@ -12,19 +12,24 @@
 from textwrap import dedent
 import unittest
 
+import transaction
+
 from canonical.database.sqlbase import (
     cursor, ISOLATION_LEVEL_AUTOCOMMIT, ISOLATION_LEVEL_DEFAULT,
     ISOLATION_LEVEL_READ_COMMITTED, ISOLATION_LEVEL_SERIALIZABLE,
-    connect)
+    connect, ZopelessTransactionManager)
 from canonical.testing.layers import LaunchpadZopelessLayer
 
 
+def set_isolation_level(isolation):
+    user = ZopelessTransactionManager._dbuser
+    ZopelessTransactionManager.uninstall()
+    ZopelessTransactionManager.initZopeless(dbuser=user, isolation=isolation)
+
+
 class TestIsolation(unittest.TestCase):
     layer = LaunchpadZopelessLayer
 
-    def setUp(self):
-        self.txn = LaunchpadZopelessLayer.txn
-
     def getCurrentIsolation(self, con=None):
         if con is None:
             cur = cursor()
@@ -38,11 +43,11 @@
         self.failUnlessEqual(self.getCurrentIsolation(), 'read committed')
 
     def test_default2(self):
-        self.txn.set_isolation_level(ISOLATION_LEVEL_DEFAULT)
+        set_isolation_level(ISOLATION_LEVEL_DEFAULT)
         self.failUnlessEqual(self.getCurrentIsolation(), 'read committed')
 
     def test_autocommit(self):
-        self.txn.set_isolation_level(ISOLATION_LEVEL_AUTOCOMMIT)
+        set_isolation_level(ISOLATION_LEVEL_AUTOCOMMIT)
         # There is no actual 'autocommit' mode in PostgreSQL. psycopg
         # implements this feature by using read committed isolation and
         # issuing commit() statements after every query.
@@ -50,49 +55,46 @@
 
         # So we need to confirm we are actually in autocommit mode
         # by seeing if we an roll back
-        con = self.txn.conn()
-        cur = con.cursor()
+        cur = cursor()
         cur.execute(
             "SELECT COUNT(*) FROM Person WHERE homepage_content IS NULL")
         self.failIfEqual(cur.fetchone()[0], 0)
         cur.execute("UPDATE Person SET homepage_content=NULL")
-        con.rollback()
-        cur = con.cursor()
+        transaction.abort()
+        cur = cursor()
         cur.execute(
             "SELECT COUNT(*) FROM Person WHERE homepage_content IS NOT NULL")
         self.failUnlessEqual(cur.fetchone()[0], 0)
 
     def test_readCommitted(self):
-        self.txn.set_isolation_level(ISOLATION_LEVEL_READ_COMMITTED)
+        set_isolation_level(ISOLATION_LEVEL_READ_COMMITTED)
         self.failUnlessEqual(self.getCurrentIsolation(), 'read committed')
 
     def test_serializable(self):
-        self.txn.set_isolation_level(ISOLATION_LEVEL_SERIALIZABLE)
+        set_isolation_level(ISOLATION_LEVEL_SERIALIZABLE)
         self.failUnlessEqual(self.getCurrentIsolation(), 'serializable')
 
     def test_commit(self):
         # Change the isolation level
         self.failUnlessEqual(self.getCurrentIsolation(), 'read committed')
-        self.txn.set_isolation_level(ISOLATION_LEVEL_SERIALIZABLE)
+        set_isolation_level(ISOLATION_LEVEL_SERIALIZABLE)
         self.failUnlessEqual(self.getCurrentIsolation(), 'serializable')
 
-        con = self.txn.conn()
-        cur = con.cursor()
+        cur = cursor()
         cur.execute("UPDATE Person SET homepage_content=NULL")
-        con.commit()
+        transaction.commit()
         cur.execute("UPDATE Person SET homepage_content='foo'")
         self.failUnlessEqual(self.getCurrentIsolation(), 'serializable')
 
     def test_rollback(self):
         # Change the isolation level
         self.failUnlessEqual(self.getCurrentIsolation(), 'read committed')
-        self.txn.set_isolation_level(ISOLATION_LEVEL_SERIALIZABLE)
+        set_isolation_level(ISOLATION_LEVEL_SERIALIZABLE)
         self.failUnlessEqual(self.getCurrentIsolation(), 'serializable')
 
-        con = self.txn.conn()
-        cur = con.cursor()
+        cur = cursor()
         cur.execute("UPDATE Person SET homepage_content=NULL")
-        con.rollback()
+        transaction.abort()
         self.failUnlessEqual(self.getCurrentIsolation(), 'serializable')
 
     def test_script(self):
@@ -109,8 +111,6 @@
                 read committed
                 serializable
                 serializable
-                serializable
-                serializable
                 """))
 
     def test_connect(self):

=== modified file 'lib/canonical/database/ftests/test_zopelesstransactionmanager.txt'
--- lib/canonical/database/ftests/test_zopelesstransactionmanager.txt	2011-06-09 10:50:25 +0000
+++ lib/canonical/database/ftests/test_zopelesstransactionmanager.txt	2011-09-18 11:10:29 +0000
@@ -78,41 +78,4 @@
     >>> c.execute("SHOW transaction_isolation")
     >>> print c.fetchone()[0]
     serializable
-
-It is also possible to change the isolation level while leaving the
-remaining settings as is:
-
-    >>> from canonical.database.sqlbase import (
-    ...     ISOLATION_LEVEL_READ_COMMITTED)
-    >>> ztm.set_isolation_level(ISOLATION_LEVEL_READ_COMMITTED)
-    >>> c = cursor()
-    >>> c.execute("SHOW transaction_isolation")
-    >>> print c.fetchone()[0]
-    read committed
     >>> ztm.uninstall()
-
-We can also register synchronizers with the transaction manager.
-
-    >>> class TestSynchonizer:
-    ...     def newTransaction(self, txn):
-    ...         print "New transaction started."
-    ...
-    ...     def beforeCompletion(self, txn):
-    ...         print "Transaction will be completed."
-    ...
-    ...     def afterCompletion(self, txn):
-    ...         print "Transaction completed."
-    >>>
-    >>> synchronizer = TestSynchonizer()
-    >>> ztm.registerSynch(synchronizer)
-    >>> ztm.begin()
-    New transaction started.
-    >>> ztm.commit()
-    Transaction will be completed.
-    Transaction completed.
-
-The synchonizer can be unregistered again.
-
-    >>> ztm.unregisterSynch(synchronizer)
-    >>> ztm.begin()
-    >>> ztm.commit()

=== modified file 'lib/canonical/database/sqlbase.py'
--- lib/canonical/database/sqlbase.py	2011-09-07 23:26:54 +0000
+++ lib/canonical/database/sqlbase.py	2011-09-18 11:10:29 +0000
@@ -370,31 +370,6 @@
         cls._reset_stores()
         cls._installed = None
 
-    @classmethod
-    def set_isolation_level(cls, isolation):
-        """Set the transaction isolation level.
-
-        Level can be one of ISOLATION_LEVEL_AUTOCOMMIT,
-        ISOLATION_LEVEL_READ_COMMITTED or
-        ISOLATION_LEVEL_SERIALIZABLE. As changing the isolation level
-        must be done before any other queries are issued in the
-        current transaction, this method automatically issues a
-        rollback to ensure this is the case.
-        """
-        assert cls._installed is not None, (
-            "ZopelessTransactionManager not installed")
-        cls.uninstall()
-        cls.initZopeless(cls._dbuser, isolation)
-
-    @staticmethod
-    def conn():
-        store = _get_sqlobject_store()
-        # Use of the raw connection will not be coherent with Storm's
-        # cache.
-        connection = store._connection
-        connection._ensure_connected()
-        return connection._raw_connection
-
     @staticmethod
     def begin():
         """Begin a transaction."""
@@ -410,16 +385,6 @@
         """Abort the current transaction."""
         transaction.abort()
 
-    @staticmethod
-    def registerSynch(synch):
-        """Register an ISynchronizer."""
-        transaction.manager.registerSynch(synch)
-
-    @staticmethod
-    def unregisterSynch(synch):
-        """Unregister an ISynchronizer."""
-        transaction.manager.unregisterSynch(synch)
-
 
 def clear_current_connection_cache():
     """Clear SQLObject's object cache. SQLObject compatibility - DEPRECATED.

=== modified file 'lib/lp/soyuz/tests/test_processaccepted.py'
--- lib/lp/soyuz/tests/test_processaccepted.py	2011-08-12 05:23:13 +0000
+++ lib/lp/soyuz/tests/test_processaccepted.py	2011-09-18 11:10:29 +0000
@@ -9,6 +9,7 @@
 from debian.deb822 import Changes
 from optparse import OptionValueError
 from testtools.matchers import LessThan
+import transaction
 
 from canonical.config import config
 from canonical.launchpad.webapp.errorlog import ErrorReportingUtility
@@ -167,7 +168,7 @@
         self.layer.txn.commit()
         self.layer.switchDbUser(self.dbuser)
         synch = UploadCheckingSynchronizer()
-        script.txn.registerSynch(synch)
+        transaction.manager.registerSynch(synch)
         script.main()
         self.assertThat(len(uploads), LessThan(synch.commit_count))