launchpad-reviewers team mailing list archive

[William Grant <me@xxxxxxxxxxxxxxxxxx>]

Review: Approve code



Diff comments:

> === modified file 'cronscripts/librarian-feed-swift.py'
> --- cronscripts/librarian-feed-swift.py	2014-01-07 10:06:44 +0000
> +++ cronscripts/librarian-feed-swift.py	2014-10-15 13:17:24 +0000
> @@ -43,6 +43,7 @@
>              swift.to_swift(
>                  self.logger, self.options.start, self.options.end,
>                  self.options.remove)
> +        self.logger.info('Done')
>  
>  
>  if __name__ == '__main__':
> 
> === modified file 'lib/lp/services/librarianserver/storage.py'
> --- lib/lp/services/librarianserver/storage.py	2014-10-03 17:22:52 +0000
> +++ lib/lp/services/librarianserver/storage.py	2014-10-15 13:17:24 +0000
> @@ -61,6 +61,10 @@
>      files.
>      """
>  
> +    # Class variables storing some metrics.
> +    swift_download_attempts = 0
> +    swift_download_fails = 0
> +
>      def __init__(self, directory, library):
>          self.directory = directory
>          self.library = library
> @@ -78,6 +82,13 @@
>  
>      @defer.inlineCallbacks
>      def open(self, fileid):
> +        # Log our attempt.
> +        self.swift_download_attempts += 1
> +
> +        if self.swift_download_attempts % 1000 == 0:
> +            log.msg('{} Swift download attempts, {} failures'.format(
> +                self.swift_download_attempts, self.swift_download_fails))

This will log 1000 requests and 999 fails even if they've all failed.

> +
>          # First, try and stream the file from Swift.
>          container, name = swift.swift_location(fileid)
>          swift_connection = swift.connection_pool.get()
> @@ -88,9 +99,11 @@
>              swift_stream = TxSwiftStream(swift_connection, chunks)
>              defer.returnValue(swift_stream)
>          except swiftclient.ClientException as x:
> +            self.swift_download_fails += 1
>              if x.http_status != 404:
>                  log.err(x)

Is a 404 really a fail?

>          except Exception as x:
> +            self.swift_download_fails += 1
>              log.err(x)
>  
>          # If Swift failed, for any reason, try and stream the data from
> 
> === modified file 'lib/lp/services/librarianserver/swift.py'
> --- lib/lp/services/librarianserver/swift.py	2014-10-03 17:22:52 +0000
> +++ lib/lp/services/librarianserver/swift.py	2014-10-15 13:17:24 +0000
> @@ -150,9 +150,14 @@
>  def _put(log, swift_connection, lfc_id, container, obj_name, fs_path):
>      fs_size = os.path.getsize(fs_path)
>      fs_file = HashStream(open(fs_path, 'rb'))
> +
>      db_md5_hash = ISlaveStore(LibraryFileContent).get(
>          LibraryFileContent, lfc_id).md5
>  
> +    assert hasattr(fs_file, 'tell') and hasattr(fs_file, 'seek'), '''
> +        File not rewindable
> +        '''
> +
>      if fs_size <= MAX_SWIFT_OBJECT_SIZE:
>          swift_md5_hash = swift_connection.put_object(
>              container, obj_name, fs_file, fs_size)
> @@ -294,6 +299,7 @@
>      """Read a file while calculating a checksum as we go."""
>      def __init__(self, stream, hash_factory=hashlib.md5):
>          self._stream = stream
> +        self.hash_factory = hash_factory
>          self.hash = hash_factory()
>  
>      def read(self, size=-1):
> @@ -304,6 +310,11 @@
>      def tell(self):
>          return self._stream.tell()
>  
> +    def seek(self, offset):
> +        """Seek to offset, and reset the hash."""
> +        self.hash = self.hash_factory()
> +        return self._stream.seek(offset)
> +
>  
>  class ConnectionPool:
>      MAX_POOL_SIZE = 10
> 
> === modified file 'lib/lp/services/librarianserver/tests/test_swift.py'
> --- lib/lp/services/librarianserver/tests/test_swift.py	2014-01-07 08:02:15 +0000
> +++ lib/lp/services/librarianserver/tests/test_swift.py	2014-10-15 13:17:24 +0000
> @@ -6,6 +6,7 @@
>  __metaclass__ = type
>  
>  from cStringIO import StringIO
> +import hashlib
>  import os.path
>  import time
>  
> @@ -21,7 +22,7 @@
>  from lp.services.librarianserver.storage import LibrarianStorage
>  from lp.services.log.logger import BufferLogger
>  from lp.testing import TestCase
> -from lp.testing.layers import LaunchpadZopelessLayer, LibrarianLayer
> +from lp.testing.layers import BaseLayer, LaunchpadZopelessLayer, LibrarianLayer
>  from lp.testing.swift.fixture import SwiftFixture
>  
>  from lp.services.librarianserver import swift
> @@ -245,3 +246,53 @@
>  
>          # Our object round tripped
>          self.assertEqual(obj1 + obj2 + obj3, expected_content)
> +
> +
> +class TestHashStream(TestCase):
> +    layer = BaseLayer
> +
> +    def test_read(self):
> +        empty_md5 = 'd41d8cd98f00b204e9800998ecf8427e'
> +        s = swift.HashStream(StringIO('make me a coffee'))
> +        self.assertEqual(s.hash.hexdigest(), empty_md5)
> +        data = s.read()
> +        self.assertEqual(data, 'make me a coffee')
> +        self.assertEqual(s.hash.hexdigest(),
> +                         '17dfd3e9f99a2260552e898406c696e9')
> +
> +    def test_partial_read(self):
> +        empty_sha1 = 'da39a3ee5e6b4b0d3255bfef95601890afd80709'
> +        s = swift.HashStream(StringIO('make me another coffee'), hashlib.sha1)
> +        self.assertEqual(s.hash.hexdigest(), empty_sha1)
> +        chunk = s.read(4)
> +        self.assertEqual(chunk, 'make')
> +        self.assertEqual(s.hash.hexdigest(),
> +                         '5821eb27d7b71c9078000da31a5a654c97e401b9')
> +        chunk = s.read()
> +        self.assertEqual(chunk, ' me another coffee')
> +        self.assertEqual(s.hash.hexdigest(),
> +                         '8c826e573016ce05f3968044f82507b46fd2aa93')
> +
> +    def test_tell(self):
> +        s = swift.HashStream(StringIO('hurry up with that coffee'))
> +        self.assertEqual(s.tell(), 0)
> +        s.read(4)
> +        self.assertEqual(s.tell(), 4)
> +
> +    def test_seek(self):
> +        s = swift.HashStream(StringIO('hurry up with that coffee'))
> +        s.seek(0)
> +        self.assertEqual(s.tell(), 0)
> +        s.seek(6)
> +        self.assertEqual(s.tell(), 6)
> +        chunk = s.read()
> +        self.assertEqual(chunk, 'up with that coffee')
> +        self.assertEqual(s.hash.hexdigest(),
> +                         '0687b12af46824e3584530c5262fed36')
> +
> +        # Seek also must reset the hash.
> +        s.seek(2)
> +        chunk = s.read(3)
> +        self.assertEqual(chunk, 'rry')
> +        self.assertEqual(s.hash.hexdigest(),
> +                         '35cd51ccd493b67542201d20b6ed7db9')
> 


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