Code review comment for lp:~danilo/launchpad/bug-408718

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Am 14.09.2009 11:35, Данило Шеган schrieb:
> = Bug #408717 =
>
> Note: this is a cherrypick candidate so is kept as simple as possible.
> It's critical for Launchpad Translations (poimport is blocked on this).

Thanks for taking this on so quickly and finding a solution. If Stuart
thinks it's a great idea, who am I to argue? ;-)

 review approve

Just two comments below.

> === modified file 'lib/canonical/config/schema-lazr.conf'
> --- lib/canonical/config/schema-lazr.conf 2009-09-08 15:16:33 +0000
> +++ lib/canonical/config/schema-lazr.conf 2009-09-14 09:12:58 +0000
> @@ -1429,6 +1429,10 @@
> storm_cache: generational
> storm_cache_size: 500
>
> +# Statement timeout (in seconds), limited to super-fast-imports query.
> +# Set to 'timeout' to make it timeout every time (for tests).
> +statement_timeout: 300
> +
> [processmail]
> # The database user which will be used by this process.
> # datatype: string
>
> === modified file 'lib/lp/translations/utilities/tests/test_superfastimports.py'
> --- lib/lp/translations/utilities/tests/test_superfastimports.py 2009-07-17 02:25:09 +0000
> +++ lib/lp/translations/utilities/tests/test_superfastimports.py 2009-09-14 09:12:58 +0000
> @@ -5,8 +5,9 @@
>
> import unittest
>
> +from canonical.config import config
> from canonical.testing import ZopelessDatabaseLayer
> -from lp.testing import TestCaseWithFactory, verifyObject

Thanks for cleaning this.

> +from lp.testing import TestCaseWithFactory
> from lp.translations.utilities.translation_import import (
> ExistingPOFileInDatabase)
> from lp.translations.utilities.translation_common_format import (
> @@ -67,6 +68,27 @@
> self.assertFalse(cached_file.isAlreadyImportedTheSame(message_data))
> self.assertFalse(cached_file.isAlreadyTranslatedTheSame(message_data))
>
> + def test_query_timeout(self):
> + # Test that super-fast-imports doesn't cache anything when it hits
> + # a timeout.
> +
> + # Override the config option to force a timeout error.
> + new_config = ("""
> + [poimport]
> + statement_timeout: timeout
> + """)
> + config.push('super_fast_timeout', new_config)
> +
> + # Add a message that would otherwise be cached (see other tests).
> + current_message = self.factory.makeTranslationMessage(
> + pofile=self.pofile, is_imported=False)
> + message_data = self.getTranslationMessageData(current_message)
> + cached_file = ExistingPOFileInDatabase(self.pofile)
> + self.assertFalse(cached_file.isAlreadyTranslatedTheSame(message_data))
> +
> + # Restore the old configuration.
> + config.pop('super_fast_timeout')
> +
>
> def test_suite():
> return unittest.TestLoader().loadTestsFromName(__name__)
>
> === modified file 'lib/lp/translations/utilities/translation_import.py'
> --- lib/lp/translations/utilities/translation_import.py 2009-08-05 13:11:00 +0000
> +++ lib/lp/translations/utilities/translation_import.py 2009-09-14 09:12:58 +0000
> @@ -18,8 +18,13 @@
>
> from operator import attrgetter
>
> +import transaction
> +
> +from canonical.config import config
> from canonical.database.sqlbase import cursor, quote
>
> +from storm.exceptions import TimeoutError
> +
> from canonical.cachedproperty import cachedproperty
> from lp.registry.interfaces.person import (
> IPersonSet,
> @@ -119,7 +124,6 @@
> # Pre-fill self.messages and self.imported with data.
> self._fetchDBRows()
>
> -
> def _fetchDBRows(self):
> msgstr_joins = [
> "LEFT OUTER JOIN POTranslation pt%d "
> @@ -168,8 +172,32 @@
> TranslationTemplateItem.sequence,
> TranslationMessage.potemplate NULLS LAST
> ''' % substitutions
> +
> cur = cursor()
> - cur.execute(sql)
> + try:
> + # XXX 2009-09-14 DaniloSegan (bug #408718):
> + # this statement causes postgres to eat the diskspace
> + # from time to time. Let's wrap it up in a timeout.
> + timeout = config.poimport.statement_timeout
> +
> + # We have to commit what we've got so far or we'll lose
> + # it when we hit TimeoutError.
> + transaction.commit()
> +
> + if timeout == 'timeout':
> + # This is used in tests.
> + query = "SELECT pg_sleep(2)"
> + timeout = '1s'
> + else:
> + timeout = 1000 * int(timeout)
> + query = sql
> + cur.execute("SET statement_timeout to %s" % quote(timeout))
> + cur.execute(query)
> + except TimeoutError:
> + # Restart the transaction and return empty SelectResults.
> + transaction.abort()
> + transaction.begin()
> + cur.execute("SELECT 1 WHERE 1=0")

As you mentioned on IRC, you should reset the statement_timeout back to
what it was (if that is not done by abort() ?). Please extend the test
to check that, too.

Also, could you consider using EmptyResultSet instead of such useless
queries, please? ;)

> rows = cur.fetchall()
>
> assert TranslationConstants.MAX_PLURAL_FORMS == 6, (
>

Thanks!
Henning

-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.9 (GNU/Linux)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org

iEYEARECAAYFAkquGY8ACgkQBT3oW1L17ijQYQCg4/O+WFfckuMG/Y3ug+/jLpEP
t6UAniuipfgdRR2r0bS3bfuxQWd2tCse
=i+ew
-----END PGP SIGNATURE-----