Launchpad itself

Merge lp:~stub/launchpad/librarian-gc into lp:launchpad

  1. librarian-gc
  2. Merge into devel
Proposed by Stuart Bishop
Status: Merged
Merged at revision: not available
Proposed branch: lp:~stub/launchpad/librarian-gc
Merge into: lp:launchpad
Diff against target: 120 lines
2 files modified
lib/canonical/librarian/ftests/test_gc.py (+53/-29)
lib/canonical/librarian/librariangc.py (+1/-0)
To merge this branch: bzr merge lp:~stub/launchpad/librarian-gc
Reviewer Review Type Date Requested Status
Michael Nelson (community) code Approve
Review via email: mp+12695@code.launchpad.net

This proposal supersedes a proposal from 2009-09-30.

To post a comment you must log in.
Revision history for this message
Stuart Bishop (stub) wrote : Posted in a previous version of this proposal

Per Bug #437084, the librarian garbage collector is still failing on production. This is the fix.

Revision history for this message
Jeroen T. Vermeulen (jtv) wrote : Posted in a previous version of this proposal

Test please.

review: Needs Fixing
Revision history for this message
Stuart Bishop (stub) wrote :

Per Bug #437084, the librarian garbage collector is still failing on production. This is the fix.

Now with test case.

Revision history for this message
Michael Nelson (michael.nelson) wrote :

> Per Bug #437084, the librarian garbage collector is still failing on
> production. This is the fix.
>
> Now with test case.

Thanks stub - nicely tested.

r=me (Teeny style-guide questions below.)

> === modified file 'lib/canonical/librarian/ftests/test_gc.py'
> --- lib/canonical/librarian/ftests/test_gc.py 2009-09-26 09:06:13 +0000
> +++ lib/canonical/librarian/ftests/test_gc.py 2009-10-01 07:09:13 +0000
> @@ -15,6 +15,7 @@
> from pytz import utc
> from sqlobject import SQLObjectNotFound
> from storm.locals import SQL, AutoReload
> +import transaction
> from zope.component import getUtility
>
> from canonical.config import config
> @@ -560,6 +561,21 @@
> self.assert_(os.path.exists(noisefile2_path))
> self.assert_(os.path.exists(noisefile3_path))
>
> + def test_deleteUnwantedFilesBug437084(self):

Just looking at https://dev.launchpad.net/TestsStyleGuide#Python Test Cases
should this be:

       def test_deleteUnwantedFiles_bug437084(self):

or similar?

> + # There was a bug where delete_unwanted_files() would die
> + # if the last file found on disk was unwanted.
> + self.layer.switchDbUser(dbuser='testadmin')
> + content='foo'
> + self.client.addFile(
> + 'foo.txt', len(content), StringIO(content), 'text/plain',
> + )

I always get confused on this one, but I think you've used the tuple
style here, rather than the multiline function call style? That is,
I the closing ')' should be on the previous line and without the extra
comma? See

https://dev.launchpad.net/PythonStyleGuide#Multiline function calls

> + # Roll back the database changes, leaving the file on disk.
> + transaction.abort()

Nice!

> +
> + self.layer.switchDbUser(config.librarian_gc.dbuser)
> +
> + # This should cope.
> + librariangc.delete_unwanted_files(self.con)
>
> def test_cronscript(self):
> script_path = os.path.join(
>
> === modified file 'lib/canonical/librarian/librariangc.py'
> --- lib/canonical/librarian/librariangc.py 2009-09-26 08:43:07 +0000
> +++ lib/canonical/librarian/librariangc.py 2009-10-01 07:09:13 +0000
> @@ -578,6 +578,7 @@
> next_wanted_content_id = get_next_wanted_content_id()
>
> if (config.librarian_server.upstream_host is None
> + and next_wanted_content_id is not None
> and next_wanted_content_id < content_id):
> log.error(
> "LibraryFileContent %d exists in the database but "

And I asked:

<noodles775> stub: hi! Just looking at your MP... would it be worth in addition creating a separate condition to log a warning for this situation where the file exists on disk but not in the db?
<stub> That is actually normal - client.addFile(foo,...); transaction.abort() leaves a file on disk
<noodles775> OK - I assumed that the transaction is only aborted in abnormal circumstances.
<stub> Nah - anytime a web request needs to be retried after uploading a file for instance.
<noodles775> k. great.

--
Michael

review: Approve (code)
Revision history for this message
Stuart Bishop (stub) wrote :

> > + def test_deleteUnwantedFilesBug437084(self):
>
> Just looking at https://dev.launchpad.net/TestsStyleGuide#Python Test Cases
> should this be:
>
> def test_deleteUnwantedFiles_bug437084(self):
>
> or similar?

As per IRC, test_delete_unwanted_files_bug437084().

> > + # There was a bug where delete_unwanted_files() would die
> > + # if the last file found on disk was unwanted.
> > + self.layer.switchDbUser(dbuser='testadmin')
> > + content='foo'
> > + self.client.addFile(
> > + 'foo.txt', len(content), StringIO(content), 'text/plain',
> > + )
>
> I always get confused on this one, but I think you've used the tuple
> style here, rather than the multiline function call style? That is,
> I the closing ')' should be on the previous line and without the extra
> comma? See
>
> https://dev.launchpad.net/PythonStyleGuide#Multiline function calls

Yup. Copy and paste glitch. Fixed.

Thanks :)

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
=== modified file 'lib/canonical/librarian/ftests/test_gc.py'
--- lib/canonical/librarian/ftests/test_gc.py 2009-09-26 09:06:13 +0000
+++ lib/canonical/librarian/ftests/test_gc.py 2009-10-01 08:05:25 +0000
@@ -5,6 +5,7 @@
55
6__metaclass__ = type6__metaclass__ = type
77
8import shutil
8import sys9import sys
9import os10import os
10from subprocess import Popen, PIPE, STDOUT11from subprocess import Popen, PIPE, STDOUT
@@ -15,6 +16,7 @@
15from pytz import utc16from pytz import utc
16from sqlobject import SQLObjectNotFound17from sqlobject import SQLObjectNotFound
17from storm.locals import SQL, AutoReload18from storm.locals import SQL, AutoReload
19import transaction
18from zope.component import getUtility20from zope.component import getUtility
1921
20from canonical.config import config22from canonical.config import config
@@ -529,37 +531,59 @@
529 # Long non-hexadecimal number531 # Long non-hexadecimal number
530 noisedir3_path = os.path.join(config.librarian_server.root, '11.bak')532 noisedir3_path = os.path.join(config.librarian_server.root, '11.bak')
531533
532 os.mkdir(noisedir1_path)
533 os.mkdir(noisedir2_path)
534 os.mkdir(noisedir3_path)
535
536 # Files in the noise directories.
537 noisefile1_path = os.path.join(noisedir1_path, 'abc')
538 noisefile2_path = os.path.join(noisedir2_path, 'def')
539 noisefile3_path = os.path.join(noisedir2_path, 'ghi')
540 open(noisefile1_path, 'w').write('hello')
541 open(noisefile2_path, 'w').write('there')
542 open(noisefile3_path, 'w').write('testsuite')
543
544 # Pretend it is tomorrow to ensure the files don't count as
545 # recently created, and run the delete_unwanted_files process.
546 org_time = librariangc.time
547 def tomorrow_time():
548 return org_time() + 24 * 60 * 60 + 1
549 try:534 try:
550 librariangc.time = tomorrow_time535 os.mkdir(noisedir1_path)
551 librariangc.delete_unwanted_files(self.con)536 os.mkdir(noisedir2_path)
537 os.mkdir(noisedir3_path)
538
539 # Files in the noise directories.
540 noisefile1_path = os.path.join(noisedir1_path, 'abc')
541 noisefile2_path = os.path.join(noisedir2_path, 'def')
542 noisefile3_path = os.path.join(noisedir2_path, 'ghi')
543 open(noisefile1_path, 'w').write('hello')
544 open(noisefile2_path, 'w').write('there')
545 open(noisefile3_path, 'w').write('testsuite')
546
547 # Pretend it is tomorrow to ensure the files don't count as
548 # recently created, and run the delete_unwanted_files process.
549 org_time = librariangc.time
550 def tomorrow_time():
551 return org_time() + 24 * 60 * 60 + 1
552 try:
553 librariangc.time = tomorrow_time
554 librariangc.delete_unwanted_files(self.con)
555 finally:
556 librariangc.time = org_time
557
558 # None of the rubbish we created has been touched.
559 self.assert_(os.path.isdir(noisedir1_path))
560 self.assert_(os.path.isdir(noisedir2_path))
561 self.assert_(os.path.isdir(noisedir3_path))
562 self.assert_(os.path.exists(noisefile1_path))
563 self.assert_(os.path.exists(noisefile2_path))
564 self.assert_(os.path.exists(noisefile3_path))
552 finally:565 finally:
553 librariangc.time = org_time566 # We need to clean this up ourselves, as the standard librarian
554567 # cleanup only removes files it knows where valid to avoid
555 # None of the rubbish we created has been touched.568 # accidents.
556 self.assert_(os.path.isdir(noisedir1_path))569 shutil.rmtree(noisedir1_path)
557 self.assert_(os.path.isdir(noisedir2_path))570 shutil.rmtree(noisedir2_path)
558 self.assert_(os.path.isdir(noisedir3_path))571 shutil.rmtree(noisedir3_path)
559 self.assert_(os.path.exists(noisefile1_path))572
560 self.assert_(os.path.exists(noisefile2_path))573 def test_delete_unwanted_files_bug437084(self):
561 self.assert_(os.path.exists(noisefile3_path))574 # There was a bug where delete_unwanted_files() would die
562575 # if the last file found on disk was unwanted.
576 self.layer.switchDbUser(dbuser='testadmin')
577 content='foo'
578 self.client.addFile(
579 'foo.txt', len(content), StringIO(content), 'text/plain')
580 # Roll back the database changes, leaving the file on disk.
581 transaction.abort()
582
583 self.layer.switchDbUser(config.librarian_gc.dbuser)
584
585 # This should cope.
586 librariangc.delete_unwanted_files(self.con)
563587
564 def test_cronscript(self):588 def test_cronscript(self):
565 script_path = os.path.join(589 script_path = os.path.join(
566590
=== modified file 'lib/canonical/librarian/librariangc.py'
--- lib/canonical/librarian/librariangc.py 2009-09-26 08:43:07 +0000
+++ lib/canonical/librarian/librariangc.py 2009-10-01 08:05:25 +0000
@@ -578,6 +578,7 @@
578 next_wanted_content_id = get_next_wanted_content_id()578 next_wanted_content_id = get_next_wanted_content_id()
579579
580 if (config.librarian_server.upstream_host is None580 if (config.librarian_server.upstream_host is None
581 and next_wanted_content_id is not None
581 and next_wanted_content_id < content_id):582 and next_wanted_content_id < content_id):
582 log.error(583 log.error(
583 "LibraryFileContent %d exists in the database but "584 "LibraryFileContent %d exists in the database but "