Unnecessary fetch of metadata in DELETE's mkstemp context?

So if we can pull that out, then we could refactor the code a bit to:

  * Keep extension names only in the DiskFile class
  * Hide put the simple "with mkstemp/put()" expressions in a simple routine to abstract the behavior

An example of the kind of refactoring that could be done can be found at the end of this post. Note that we also notice that tmppath does not have to be exposed to the caller, only fd does, and only in the case of a PUT.

Why would we want to do all this?

It will allow the Gluster code base an easier way to add a temporary file performance improvement without having to patch the object server code, just replace the DiskFile class.

And for that matter, it would be nice if DiskFile was referenced by a method of the ObjectController (like container and account brokers are by AccountController and ContainerController) so that we can just subclass ObjectController to replace DiskFile behaviors instead of monkey-patching it.

Thanks for indulging me.

diff --git a/swift/obj/server.py b/swift/obj/server.py
index c654d80..f8fe3b7 100755
--- a/swift/obj/server.py
+++ b/swift/obj/server.py
@@ -118,6 +118,7 @@ class DiskFile(object):
             path, device, storage_directory(DATADIR, partition, name_hash))
         self.device_path = os.path.join(path, device)
         self.tmpdir = os.path.join(path, device, 'tmp')
+ self.tmppath = None
         self.logger = logger
         self.metadata = {}
         self.meta_file = None
@@ -278,30 +279,31 @@ class DiskFile(object):
         """Contextmanager to make a temporary file."""
         if not os.path.exists(self.tmpdir):
             mkdirs(self.tmpdir)
- fd, tmppath = mkstemp(dir=self.tmpdir)
+ fd, self.tmppath = mkstemp(dir=self.tmpdir)
         try:
- yield fd, tmppath
+ yield fd
         finally:
             try:
                 os.close(fd)
             except OSError:
                 pass
+ tmppath, self.tmppath = self.tmppath, None
             try:
                 os.unlink(tmppath)
             except OSError:
                 pass

- def put(self, fd, tmppath, metadata, extension='.data'):
+ def put(self, fd, metadata, extension='.data'):
         """
         Finalize writing the file on disk, and renames it from the temp file to
         the real location. This should be called after the data has been
         written to the temp file.

- :params fd: file descriptor of the temp file
- :param tmppath: path to the temporary file being used
+ :param fd: file descriptor of the temp file
         :param metadata: dictionary of metadata to be written
         :param extension: extension to be used when making the file
         """
+ assert self.tmppath is not None
         metadata['name'] = self.name
         timestamp = normalize_timestamp(metadata['X-Timestamp'])
         write_metadata(fd, metadata)
@@ -309,9 +311,20 @@ class DiskFile(object):
             self.drop_cache(fd, 0, int(metadata['Content-Length']))
         tpool.execute(fsync, fd)
         invalidate_hash(os.path.dirname(self.datadir))
- renamer(tmppath, os.path.join(self.datadir, timestamp + extension))
+ renamer(self.tmppath, os.path.join(self.datadir, timestamp + extension))
         self.metadata = metadata

+ def put_metadata(self, metadata, tombstone=False):
+ """
+ Short hand for putting metadata to .meta and .ts files.
+
+ :param metadata: dictionary of metadata to be written
+ :param tombstone: whether or not we are writing a tombstone
+ """
+ extension = '.ts' if tombstone else '.meta'
+ with file.mkstemp() as fd:
+ file.put(fd, metadata, extension=extension)
+
     def unlinkold(self, timestamp):
         """
         Remove any older versions of the object file. Any file that has an
@@ -565,8 +578,7 @@ class ObjectController(object):
             if old_delete_at:
                 self.delete_at_update('DELETE', old_delete_at, account,
                                       container, obj, request.headers, device)
- with file.mkstemp() as (fd, tmppath):
- file.put(fd, tmppath, metadata, extension='.meta')
+ file.put_metadata(metadata)
         return HTTPAccepted(request=request)

     @public
@@ -600,7 +612,7 @@ class ObjectController(object):
         etag = md5()
         upload_size = 0
         last_sync = 0
- with file.mkstemp() as (fd, tmppath):
+ with file.mkstemp() as fd:
             if 'content-length' in request.headers:
                 try:
                     fallocate(fd, int(request.headers['content-length']))
@@ -654,7 +666,7 @@ class ObjectController(object):
                     self.delete_at_update(
                         'DELETE', old_delete_at, account, container, obj,
                         request.headers, device)
- file.put(fd, tmppath, metadata)
+ file.put(fd, metadata)
         file.unlinkold(metadata['X-Timestamp'])
         if not orig_timestamp or \
                 orig_timestamp < request.headers['x-timestamp']:
@@ -821,12 +833,11 @@ class ObjectController(object):
         metadata = {
             'X-Timestamp': request.headers['X-Timestamp'], 'deleted': True,
         }
- with file.mkstemp() as (fd, tmppath):
- old_delete_at = int(file.metadata.get('X-Delete-At') or 0)
- if old_delete_at:
- self.delete_at_update('DELETE', old_delete_at, account,
- container, obj, request.headers, device)
- file.put(fd, tmppath, metadata, extension='.ts')
+ old_delete_at = int(file.metadata.get('X-Delete-At') or 0)
+ if old_delete_at:
+ self.delete_at_update('DELETE', old_delete_at, account,
+ container, obj, request.headers, device)
+ file.put_metadata(metadata, tomestone=True)
         file.unlinkold(metadata['X-Timestamp'])
         if not orig_timestamp or \
                 orig_timestamp < request.headers['x-timestamp']:

Sounds like a plan, but it's a bit hard to read in this Launchpad Q&A thing. Can you just submit it as a code patch set to Swift (Gerrit review)? You'll probably get a lot more eyes on it that way too.

Working do submit the code, just waiting for Contributor approval.

Out for review at: https://review.openstack.org/#/c/16185/

This question was expired because it remained in the 'Needs information' state without activity for the last 15 days.