launchpad-reviewers team mailing list archive

[William Grant <me@xxxxxxxxxxxxxxxxxx>]

William Grant has proposed merging lp:~wgrant/launchpad/librarian-swift-unify into lp:launchpad with lp:~wgrant/launchpad/librarian-missing-500 as a prerequisite.

Commit message:
Drop the pre-Swift librarian codepaths. The Swift bits are battle-tested now.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)

For more details, see:
https://code.launchpad.net/~wgrant/launchpad/librarian-swift-unify/+merge/281371

Drop the pre-Swift librarian codepaths. The Swift bits are battle-tested now.

The feature flag check is now in LibrarianStorage.open.
-- 
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~wgrant/launchpad/librarian-swift-unify into lp:launchpad.

=== modified file 'lib/lp/services/librarianserver/storage.py'
--- lib/lp/services/librarianserver/storage.py	2015-01-09 07:36:08 +0000
+++ lib/lp/services/librarianserver/storage.py	2015-12-27 04:05:56 +0000
@@ -20,6 +20,7 @@
 from lp.services.database import write_transaction
 from lp.services.database.interfaces import IStore
 from lp.services.database.postgresql import ConnectionString
+from lp.services.features import getFeatureFlag
 from lp.services.librarianserver import swift
 
 
@@ -82,35 +83,37 @@
 
     @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))
-
-        # First, try and stream the file from Swift.
-        container, name = swift.swift_location(fileid)
-        swift_connection = swift.connection_pool.get()
-        try:
-            headers, chunks = yield deferToThread(
-                swift_connection.get_object,
-                container, name, resp_chunk_size=self.CHUNK_SIZE)
-            swift_stream = TxSwiftStream(swift_connection, chunks)
-            defer.returnValue(swift_stream)
-        except swiftclient.ClientException as x:
-            if x.http_status == 404:
-                swift.connection_pool.put(swift_connection)
-            else:
+        if getFeatureFlag('librarian.swift.enabled'):
+            # 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))
+
+            # First, try and stream the file from Swift.
+            container, name = swift.swift_location(fileid)
+            swift_connection = swift.connection_pool.get()
+            try:
+                headers, chunks = yield deferToThread(
+                    swift_connection.get_object,
+                    container, name, resp_chunk_size=self.CHUNK_SIZE)
+                swift_stream = TxSwiftStream(swift_connection, chunks)
+                defer.returnValue(swift_stream)
+            except swiftclient.ClientException as x:
+                if x.http_status == 404:
+                    swift.connection_pool.put(swift_connection)
+                else:
+                    self.swift_download_fails += 1
+                    log.err(x)
+            except Exception as x:
                 self.swift_download_fails += 1
                 log.err(x)
-        except Exception as x:
-            self.swift_download_fails += 1
-            log.err(x)
+            # If Swift failed, for any reason, fall through to try and
+            # stream the data from disk. In particular, files cannot be
+            # found in Swift until librarian-feed-swift.py has put them
+            # in there.
 
-        # If Swift failed, for any reason, try and stream the data from
-        # disk. In particular, files cannot be found in Swift until
-        # librarian-feed-swift.py has put them in there.
         path = self._fileLocation(fileid)
         if os.path.exists(path):
             defer.returnValue(open(path, 'rb'))

=== modified file 'lib/lp/services/librarianserver/web.py'
--- lib/lp/services/librarianserver/web.py	2015-12-27 04:05:56 +0000
+++ lib/lp/services/librarianserver/web.py	2015-12-27 04:05:56 +0000
@@ -25,7 +25,6 @@
     read_transaction,
     write_transaction,
     )
-from lp.services.features import getFeatureFlag
 from lp.services.librarian.client import url_path_quote
 from lp.services.librarian.utils import guess_librarian_encoding
 
@@ -118,17 +117,11 @@
 
         token = request.args.get('token', [None])[0]
         path = request.path
-        if getFeatureFlag('librarian.swift.enabled'):
-            deferred = deferToThread(
-                self._getFileAlias_swift, self.aliasID, token, path)
-            deferred.addCallback(
-                self._cb_getFileAlias_swift, filename, request)
-        else:
-            deferred = deferToThread(
-                self._getFileAlias, self.aliasID, token, path)
-            deferred.addCallback(
-                    self._cb_getFileAlias, filename, request
-                    )
+        deferred = deferToThread(
+            self._getFileAlias, self.aliasID, token, path)
+        deferred.addCallback(
+                self._cb_getFileAlias, filename, request
+                )
         deferred.addErrback(self._eb_getFileAlias)
         return util.DeferredResource(deferred)
 
@@ -137,15 +130,6 @@
         try:
             alias = self.storage.getFileAlias(aliasID, token, path)
             return (alias.contentID, alias.filename,
-                alias.mimetype, alias.date_created, alias.restricted)
-        except LookupError:
-            raise NotFound
-
-    @write_transaction
-    def _getFileAlias_swift(self, aliasID, token, path):
-        try:
-            alias = self.storage.getFileAlias(aliasID, token, path)
-            return (alias.contentID, alias.filename,
                 alias.mimetype, alias.date_created, alias.content.filesize,
                 alias.restricted)
         except LookupError:
@@ -162,43 +146,9 @@
         else:
             return fourOhFour
 
+    @defer.inlineCallbacks
     def _cb_getFileAlias(
             self,
-            (dbcontentID, dbfilename, mimetype, date_created, restricted),
-            filename, request
-            ):
-        # Return a 404 if the filename in the URL is incorrect. This offers
-        # a crude form of access control (stuff we care about can have
-        # unguessable names effectively using the filename as a secret).
-        if dbfilename.encode('utf-8') != filename:
-            log.msg(
-                "404: dbfilename.encode('utf-8') != filename: %r != %r"
-                % (dbfilename.encode('utf-8'), filename))
-            return fourOhFour
-
-        if self.storage.hasFile(dbcontentID):
-            # XXX: Brad Crittenden 2007-12-05 bug=174204: When encodings are
-            # stored as part of a file's metadata this logic will be replaced.
-            encoding, mimetype = guess_librarian_encoding(filename, mimetype)
-            # Set our caching headers. Public Librarian files can be
-            # cached forever, while private ones mustn't be at all.
-            request.setHeader(
-                'Cache-Control',
-                'max-age=31536000, public'
-                if not restricted else 'max-age=0, private')
-            return File(
-                mimetype, encoding, date_created,
-                self.storage._fileLocation(dbcontentID))
-        elif self.upstreamHost is not None:
-            return proxy.ReverseProxyResource(self.upstreamHost,
-                                              self.upstreamPort, request.path)
-        else:
-            raise AssertionError(
-                "Content %d missing from storage." % dbcontentID)
-
-    @defer.inlineCallbacks
-    def _cb_getFileAlias_swift(
-            self,
             (dbcontentID, dbfilename, mimetype, date_created, size,
                 restricted),
             filename, request
@@ -217,7 +167,7 @@
             # XXX: Brad Crittenden 2007-12-05 bug=174204: When encodings are
             # stored as part of a file's metadata this logic will be replaced.
             encoding, mimetype = guess_librarian_encoding(filename, mimetype)
-            file = File_swift(mimetype, encoding, date_created, stream, size)
+            file = File(mimetype, encoding, date_created, stream, size)
             assert file.exists
             # Set our caching headers. Public Librarian files can be
             # cached forever, while private ones mustn't be at all.
@@ -242,28 +192,6 @@
     isLeaf = True
 
     def __init__(
-        self, contentType, encoding, modification_time, *args, **kwargs):
-        # Have to convert the UTC datetime to POSIX timestamp (localtime)
-        offset = datetime.utcnow() - datetime.now()
-        local_modification_time = modification_time - offset
-        self._modification_time = time.mktime(
-            local_modification_time.timetuple())
-        static.File.__init__(self, *args, **kwargs)
-        self.type = contentType
-        self.encoding = encoding
-
-    def getModificationTime(self):
-        """Override the time on disk with the time from the database.
-
-        This is used by twisted to set the Last-Modified: header.
-        """
-        return self._modification_time
-
-
-class File_swift(static.File):
-    isLeaf = True
-
-    def __init__(
         self, contentType, encoding, modification_time, stream, size):
         # Have to convert the UTC datetime to POSIX timestamp (localtime)
         offset = datetime.utcnow() - datetime.now()