launchpad-reviewers team mailing list archive

[William Grant <me@xxxxxxxxxxxxxxxxxx>]

Review: Approve code

Given the sensitivity of this change I wouldn't mind seeing a quick test.

Diff comments:

> === modified file 'cronscripts/librarian-feed-swift.py'
> --- cronscripts/librarian-feed-swift.py	2014-10-22 10:00:03 +0000
> +++ cronscripts/librarian-feed-swift.py	2014-12-16 09:25:02 +0000
> @@ -23,9 +23,12 @@
>              "-i", "--id", action="append", dest="ids", default=[],
>              metavar="CONTENT_ID", help="Migrate a single file")
>          self.parser.add_option(
> -            "-r", "--remove", action="store_true", default=False,
> +            "--remove", action="store_true", default=False,
>              help="Remove files from disk after migration (default: False)")
>          self.parser.add_option(
> +            "--rename", action="store_true", default=False,
> +            help="Rename files on disk after migration (default: False)")
> +        self.parser.add_option(
>              "-s", "--start", action="store", type=int, default=None,
>              dest="start", metavar="CONTENT_ID",
>              help="Migrate files starting from CONTENT_ID")
> @@ -39,22 +42,33 @@
>              help="Migrate files up to and including CONTENT_ID")
>  
>      def main(self):
> +        if self.options.rename and self.options.remove:
> +            self.parser.error("Cannot both remove and rename")
> +        elif self.options.rename:
> +            remove = swift.rename
> +        elif self.options.remove:
> +            remove = os.unlink
> +        else:
> +            remove = None
> +
>          if self.options.start_since:
>              self.options.start = ISlaveStore(LibraryFileContent).execute("""
>                  SELECT MAX(id) FROM LibraryFileContent
>                  WHERE datecreated < current_timestamp at time zone 'UTC'
>                      - CAST(%s AS INTERVAL)
>                  """, (unicode(self.options.start_since),)).get_one()[0]
> +
>          if self.options.ids and (self.options.start or self.options.end):
>              self.parser.error(
>                  "Cannot specify both individual file(s) and range")
> +
>          elif self.options.ids:
>              for lfc in self.options.ids:
> -                swift.to_swift(self.logger, lfc, lfc, self.options.remove)
> +                swift.to_swift(self.logger, lfc, lfc, remove)
> +
>          else:
> -            swift.to_swift(
> -                self.logger, self.options.start, self.options.end,
> -                self.options.remove)
> +            swift.to_swift(self.logger, self.options.start,
> +                           self.options.end, remove)
>          self.logger.info('Done')
>  
>  
> 
> === modified file 'lib/lp/services/librarianserver/librariangc.py'
> --- lib/lp/services/librarianserver/librariangc.py	2014-10-22 19:52:40 +0000
> +++ lib/lp/services/librarianserver/librariangc.py	2014-12-16 09:25:02 +0000
> @@ -433,7 +433,7 @@
>              "SELECT COALESCE(max(id),0) FROM UnreferencedLibraryFileAlias")
>          self.max_id = cur.fetchone()[0]
>          log.debug(
> -            "%d unreferenced LibraryFileContent to remove." % self.max_id)
> +            "%d unreferenced LibraryFileAlias to remove." % self.max_id)
>          con.commit()
>  
>      def isDone(self):
> @@ -627,7 +627,8 @@
>              dirnames.remove('incoming')
>          if 'lost+found' in dirnames:
>              dirnames.remove('lost+found')
> -        filenames = set(filenames)
> +        filenames = set([fn for fn in filenames
> +                         if not fn.endswith('.migrated'])

That list comprehension might as well be a generator expression.

>          filenames.discard('librarian.pid')
>          filenames.discard('librarian.log')
>  
> 
> === modified file 'lib/lp/services/librarianserver/swift.py'
> --- lib/lp/services/librarianserver/swift.py	2014-10-15 07:52:40 +0000
> +++ lib/lp/services/librarianserver/swift.py	2014-12-16 09:25:02 +0000
> @@ -29,13 +29,13 @@
>  ONE_DAY = 24 * 60 * 60
>  
>  
> -def to_swift(log, start_lfc_id=None, end_lfc_id=None, remove=False):
> +def to_swift(log, start_lfc_id=None, end_lfc_id=None, remove_func=False):
>      '''Copy a range of Librarian files from disk into Swift.
>  
>      start and end identify the range of LibraryFileContent.id to
>      migrate (inclusive).
>  
> -    If remove is True, files are removed from disk after being copied into
> +    If remove_func is set, it is called for every file after being copied into
>      Swift.
>      '''
>      swift_connection = connection_pool.get()
> @@ -45,7 +45,7 @@
>          start_lfc_id = 1
>      if end_lfc_id is None:
>          # Maximum id capable of being stored on the filesystem - ffffffff
> -        end_lfc_id = 4294967295
> +        end_lfc_id = 0xffffffff
>  
>      log.info("Walking disk store {0} from {1} to {2}, inclusive".format(
>          fs_root, start_lfc_id, end_lfc_id))
> @@ -143,8 +143,15 @@
>                      lfc, container, obj_name))
>                  _put(log, swift_connection, lfc, container, obj_name, fs_path)
>  
> -            if remove:
> -                os.unlink(fs_path)
> +            if remove_func:
> +                remove_func(fs_path)
> +
> +
> +def rename(path):
> +    # It would be nice to move the file out of the tree entirely, but we
> +    # need to keep the backup on the same filesystem as the original
> +    # file.
> +    os.rename(path, path + '.migrated')
>  
>  
>  def _put(log, swift_connection, lfc_id, container, obj_name, fs_path):
> 


-- 
https://code.launchpad.net/~stub/launchpad/trivial/+merge/244833
Your team Launchpad code reviewers is subscribed to branch lp:launchpad.