Merge lp:~verterok/ubuntuone-client/fix-848224 into lp:ubuntuone-client

[Guillermo Gonzalez]

fix Tritcask.should_merge method to support missing stats for a data file.

This branch fix Tritcask.should_merge method to support missing stats for a data file, e.g: the file is 100% deletes (tombstones) there are no stats for it.

[lp:~verterok/ubuntuone-client/fix-848224]

1136. By Guillermo Gonzalez on 2011-09-15

make use of try/except/else in the get_stats calls

[Facundo Batista (facundo)]

Like it!

review: Approve

=== modified file 'tests/syncdaemon/test_tritcask.py'
--- tests/syncdaemon/test_tritcask.py	2011-08-30 17:16:49 +0000
+++ tests/syncdaemon/test_tritcask.py	2011-09-15 19:44:30 +0000
@@ -1089,6 +1089,29 @@
         db.rotate()
         self.assertTrue(db.should_merge(db._immutable))
 
+    def test_should_merge_no_stats(self):
+        """Test should_merge method without stats for a data file."""
+        db = Tritcask(self.base_dir, max_immutable_files=10)
+        # add some data
+        for j in range(10):
+            db.put(j, 'foo%d' % (j,), 'bar%s' % (j,))
+        # rotate the file to create a immutable with all the live rows
+        db.rotate()
+        # delete everything
+        for j in range(10):
+            db.delete(j, 'foo%d' % (j,))
+        # rotate the file to create an immutable with all the tombstones
+        fid = db.live_file.file_id
+        db.rotate()
+        db.shutdown()
+        # start with auto_merge=False
+        db = Tritcask(self.base_dir, auto_merge=False)
+        self.addCleanup(db.shutdown)
+        # check that we don't have any stats for the file with the tombstones
+        self.assertRaises(KeyError, db._keydir.get_stats, fid)
+        # check the should_merge works as expected
+        self.assertTrue(db.should_merge(db._immutable))
+
     def test__rotate_and_not_merge(self):
         """Test _rotate_and_merge method."""
         db = Tritcask(self.base_dir)
@@ -1629,6 +1652,12 @@
         self.assertTrue(keydir._stats[file_id] is not \
                         keydir.get_stats(file_id))
 
+    def test_get_stats_missing(self):
+        """Test get_stats with a missing file_id."""
+        keydir = Keydir()
+        file_id = DataFile._get_next_file_id()
+        self.assertRaises(KeyError, keydir.get_stats, file_id)
+
 
 class TritcaskShelfTests(BaseTestCase):
     """Tests for TritcaskShelf."""

=== modified file 'ubuntuone/syncdaemon/tritcask.py'
--- ubuntuone/syncdaemon/tritcask.py	2011-08-23 15:48:54 +0000
+++ ubuntuone/syncdaemon/tritcask.py	2011-09-15 19:44:30 +0000
@@ -549,7 +549,6 @@
         return self._stats[file_id].copy()
 
 
-
 class Tritcask(object):
     """Implementation of a bitcask-like (row_type,key)/value store.
 
@@ -640,25 +639,30 @@
         # now check the default rotation policy
         try:
             live_file_stats = self._keydir.get_stats(self.live_file.file_id)
+        except KeyError:
+            # no info for the live file
+            return False
+        else:
             return (live_file_stats['live_bytes'] / self.live_file.size) \
                     < self.dead_bytes_threshold
-        except KeyError:
-            # no info for the live file
-            return False
 
     def should_merge(self, immutable_files):
         """Check if the immutable_files should be merged."""
         if not immutable_files:
             return False
-        file_ids = sorted(immutable_files.keys())
-        live_bytes = sum([self._keydir.get_stats(file_id)['live_bytes'] \
-                          for file_id in file_ids])
+        live_bytes = 0
+        for file_id in sorted(immutable_files.keys()):
+            try:
+                stats = self._keydir.get_stats(file_id)
+            except KeyError:
+                # no stats for this data file
+                pass
+            else:
+                live_bytes += stats['live_bytes']
         total_bytes = sum([f.size for f in immutable_files.values()])
         merge_dead = (live_bytes / total_bytes) < self.dead_bytes_threshold
-        if merge_dead:
-            return True
-        # now check if we should merge using the max_immutable_files setting.
-        if len(self._immutable) > self.max_immutable_files:
+        # check if we should merge using the max_immutable_files setting.
+        if merge_dead or len(self._immutable) > self.max_immutable_files:
             return True
         # shouldn't merge.
         return False