11684: ArvadosFile.flush() now check if it is the only owner of a bufferblock before...

On commit_all(), always check if the owner attribute is an instance of ArvadosFile before calling flush()
Fixed a couple tests that were mocking bufferblock.owner so that they work with this new behavior.

diff --git a/sdk/python/arvados/arvfile.py b/sdk/python/arvados/arvfile.py
index 2c653d7f09333a7e02621f8647e52c6bcace740e..a1d87a498ef81304ad6266793292263f8ff4479c 100644 (file)
--- a/sdk/python/arvados/arvfile.py
+++ b/sdk/python/arvados/arvfile.py
@@ -799,7 +799,7 @@ class _BlockManager(object):
             if v.state() != _BufferBlock.COMMITTED and v.owner:
                 # Ignore blocks with a list of owners, as if they're not in COMMITTED
                 # state, they're already being committed asynchronously.
-                if not isinstance(v.owner, list):
+                if isinstance(v.owner, ArvadosFile):
                     v.owner.flush(sync=False)
 
         with self.lock:
@@ -824,7 +824,7 @@ class _BlockManager(object):
                     # of repacking small blocks, so don't delete it when flushing
                     # its owners, just do it after flushing them all.
                     for owner in v.owner:
-                        owner.flush(sync=True, delete_bufferblock=False)
+                        owner.flush(sync=True)
                     self.delete_bufferblock(k)
 
     def block_prefetch(self, locator):
@@ -1129,7 +1129,7 @@ class ArvadosFile(object):
         return len(data)
 
     @synchronized
-    def flush(self, sync=True, num_retries=0, delete_bufferblock=True):
+    def flush(self, sync=True, num_retries=0):
         """Flush the current bufferblock to Keep.
 
         :sync:
@@ -1155,8 +1155,10 @@ class ArvadosFile(object):
                         self.parent._my_block_manager().commit_bufferblock(bb, sync=True)
                     to_delete.add(s.locator)
                     s.locator = bb.locator()
-            if delete_bufferblock:
-                for s in to_delete:
+            for s in to_delete:
+                # Don't delete the bufferblock if it's owned by many files. It'll be
+                # deleted after all of its owners are flush()ed.
+                if self.parent._my_block_manager().get_bufferblock(s).owner is self:
                     self.parent._my_block_manager().delete_bufferblock(s)
 
         self.parent.notify(MOD, self.parent, self.name, (self, self))

diff --git a/sdk/python/tests/test_arvfile.py b/sdk/python/tests/test_arvfile.py
index a8009130551fe3b0ecf0d74aa3ebfe834eb59ad6..d241f73b0c8bbcbc9b37bb38421c0c485e95f7ae 100644 (file)
--- a/sdk/python/tests/test_arvfile.py
+++ b/sdk/python/tests/test_arvfile.py
@@ -832,7 +832,7 @@ class BlockManagerTest(unittest.TestCase):
         mockkeep = mock.MagicMock()
         with arvados.arvfile._BlockManager(mockkeep) as blockmanager:
             bufferblock = blockmanager.alloc_bufferblock()
-            bufferblock.owner = mock.MagicMock()
+            bufferblock.owner = mock.MagicMock(spec=arvados.arvfile.ArvadosFile)
             def flush(sync=None):
                 blockmanager.commit_bufferblock(bufferblock, sync)
             bufferblock.owner.flush.side_effect = flush
@@ -863,7 +863,7 @@ class BlockManagerTest(unittest.TestCase):
         mockkeep.put.side_effect = arvados.errors.KeepWriteError("fail")
         with arvados.arvfile._BlockManager(mockkeep) as blockmanager:
             bufferblock = blockmanager.alloc_bufferblock()
-            bufferblock.owner = mock.MagicMock()
+            bufferblock.owner = mock.MagicMock(spec=arvados.arvfile.ArvadosFile)
             def flush(sync=None):
                 blockmanager.commit_bufferblock(bufferblock, sync)
             bufferblock.owner.flush.side_effect = flush