launchpad-reviewers team mailing list archive

[Stuart Bishop <stuart.bishop@xxxxxxxxxxxxx>]

Review: Approve

Looks good. One of the comments is bogus, and one of my old ones missing a word.

Diff comments:

> === modified file 'lib/lp/services/librarianserver/librariangc.py'
> --- lib/lp/services/librarianserver/librariangc.py	2014-12-16 10:54:15 +0000
> +++ lib/lp/services/librarianserver/librariangc.py	2015-01-14 05:41:13 +0000
> @@ -616,7 +616,7 @@
>      removed_count = 0
>      content_id = next_wanted_content_id = -1
>  
> -    hex_content_id_re = re.compile('^[0-9a-f]{8}$')
> +    hex_content_id_re = re.compile('^([0-9a-f]{8})(\.migrated)?$')
>      ONE_DAY = 24 * 60 * 60
>  
>      for dirpath, dirnames, filenames in os.walk(
> @@ -627,8 +627,7 @@
>              dirnames.remove('incoming')
>          if 'lost+found' in dirnames:
>              dirnames.remove('lost+found')
> -        filenames = set(fn for fn in filenames
> -                        if not fn.endswith('.migrated'))
> +        filenames = set(filenames)
>          filenames.discard('librarian.pid')
>          filenames.discard('librarian.log')
>  
> @@ -660,12 +659,13 @@
>          for filename in filenames:
>              path = os.path.join(dirpath, filename)
>              hex_content_id = ''.join(path.split(os.sep)[-4:])
> -            if hex_content_id_re.search(hex_content_id) is None:
> +            match = hex_content_id_re.search(hex_content_id)
> +            if match is None:
>                  log.warning(
>                      "Ignoring invalid path %s" % path)
>                  continue
>  
> -            content_id = int(hex_content_id, 16)
> +            content_id = int(match.groups()[0], 16)
>  
>              while (next_wanted_content_id is not None
>                      and content_id > next_wanted_content_id):
> 
> === modified file 'lib/lp/services/librarianserver/tests/test_gc.py'
> --- lib/lp/services/librarianserver/tests/test_gc.py	2014-12-16 10:51:41 +0000
> +++ lib/lp/services/librarianserver/tests/test_gc.py	2015-01-14 05:41:13 +0000
> @@ -19,6 +19,7 @@
>  import sys
>  import tempfile
>  
> +from contextlib import contextmanager
>  from sqlobject import SQLObjectNotFound
>  from swiftclient import client as swiftclient
>  import transaction
> @@ -437,6 +438,25 @@
>                  len(results), 0, 'Too many results %r' % (results,)
>                  )
>  
> +    @contextmanager
> +    def librariangc_thinking_it_is_tomorrow(self):
> +        org_time = librariangc.time
> +        org_utcnow = librariangc._utcnow
> +
> +        def tomorrow_time():
> +            return org_time() + 24 * 60 * 60 + 1
> +
> +        def tomorrow_utcnow():
> +            return datetime.utcnow() + timedelta(days=1, seconds=1)
> +
> +        try:
> +            librariangc.time = tomorrow_time
> +            librariangc._utcnow = tomorrow_utcnow
> +            yield
> +        finally:
> +            librariangc.time = org_time
> +            librariangc._utcnow = org_utcnow
> +
>      def test_deleteUnwantedFiles(self):
>          self.ztm.begin()
>          cur = cursor()
> @@ -473,22 +493,8 @@
>  
>          # To test removal does occur when we want it to, we need to trick
>          # the garbage collector into thinking it is tomorrow.
> -        org_time = librariangc.time
> -        org_utcnow = librariangc._utcnow
> -
> -        def tomorrow_time():
> -            return org_time() + 24 * 60 * 60 + 1
> -
> -        def tomorrow_utcnow():
> -            return datetime.utcnow() + timedelta(days=1, seconds=1)
> -
> -        try:
> -            librariangc.time = tomorrow_time
> -            librariangc._utcnow = tomorrow_utcnow
> +        with self.librariangc_thinking_it_is_tomorrow():
>              librariangc.delete_unwanted_files(self.con)
> -        finally:
> -            librariangc.time = org_time
> -            librariangc._utcnow = org_utcnow
>  
>          self.failIf(self.file_exists(content_id))
>  
> @@ -578,18 +584,10 @@
>  
>          # To test this function raises an excption when it should,
>          # the garbage collector into thinking it is tomorrow.

Who wrote that rubbish comment? EENGLISH.

> -        org_time = librariangc.time
> -
> -        def tomorrow_time():
> -            return org_time() + 24 * 60 * 60 + 1
> -
> -        try:
> -            librariangc.time = tomorrow_time
> +        with self.librariangc_thinking_it_is_tomorrow():
>              self.assertRaises(
>                  Exception, librariangc.confirm_no_clock_skew, (self.con,)
>                  )
> -        finally:
> -            librariangc.time = org_time
>  
>  
>  class TestDiskLibrarianGarbageCollection(
> @@ -603,6 +601,39 @@
>          path = librariangc.get_file_path(content_id)
>          os.unlink(path)
>  
> +    def test_delete_unwanted_files_handles_migrated(self):
> +        # Files that have been uploaded to Swift have ".migrated"
> +        # appended to their names. These are treated just like the
> +        # original file, ignoring the extension.
> +        switch_dbuser('testadmin')
> +        content = 'foo'
> +        lfa = LibraryFileAlias.get(self.client.addFile(
> +            'foo.txt', len(content), StringIO(content), 'text/plain'))
> +        id_aborted = lfa.contentID
> +        # Roll back the database changes, leaving the file on disk.
> +        transaction.abort()
> +
> +        lfa = LibraryFileAlias.get(self.client.addFile(
> +            'bar.txt', len(content), StringIO(content), 'text/plain'))
> +        transaction.commit()
> +        id_committed = lfa.contentID
> +
> +        switch_dbuser(config.librarian_gc.dbuser)
> +
> +        # Now, we will move the directory containing the trash somewhere else
> +        # and make a symlink to it.

This comment doesn't match the code.

> +        path_aborted = librariangc.get_file_path(id_aborted)
> +        os.rename(path_aborted, path_aborted + '.migrated')
> +
> +        path_committed = librariangc.get_file_path(id_committed)
> +        os.rename(path_committed, path_committed + '.migrated')
> +
> +        with self.librariangc_thinking_it_is_tomorrow():
> +            librariangc.delete_unwanted_files(self.con)
> +
> +        self.assertFalse(os.path.exists(path_aborted + '.migrated'))
> +        self.assertTrue(os.path.exists(path_committed + '.migrated'))
> +
>      def test_deleteUnwantedFilesIgnoresNoise(self):
>          # Directories with invalid names in the storage area are
>          # ignored. They are reported as warnings though.
> @@ -616,8 +647,10 @@
>          # Long non-hexadecimal number
>          noisedir3_path = os.path.join(config.librarian_server.root, '11.bak')
>  
> -        # A file we will migrate to Swift using the --rename feed-swift option.
> -        migrated_path = librariangc.get_file_path(8769786) + '.migrated'
> +        # A file with the ".migrated" suffix has migrated to Swift but
> +        # may still be removed from disk as unwanted by the GC. Other
> +        # suffixes are unknown and stay around.
> +        migrated_path = librariangc.get_file_path(8769786) + '.noise'
>  
>          try:
>              os.mkdir(noisedir1_path)
> @@ -636,16 +669,8 @@
>  
>              # Pretend it is tomorrow to ensure the files don't count as
>              # recently created, and run the delete_unwanted_files process.
> -            org_time = librariangc.time
> -
> -            def tomorrow_time():
> -                return org_time() + 24 * 60 * 60 + 1
> -
> -            try:
> -                librariangc.time = tomorrow_time
> +            with self.librariangc_thinking_it_is_tomorrow():
>                  librariangc.delete_unwanted_files(self.con)
> -            finally:
> -                librariangc.time = org_time
>  
>              # None of the rubbish we created has been touched.
>              self.assert_(os.path.isdir(noisedir1_path))
> 


-- 
https://code.launchpad.net/~wgrant/launchpad/bug-1410592/+merge/246389
Your team Launchpad code reviewers is subscribed to branch lp:launchpad.