Extensions Docs Server: Efficient MemcacheFileSystem

chrome/common/extensions/docs/server2/memcache_file_system_test.py

Index: chrome/common/extensions/docs/server2/memcache_file_system_test.py
diff --git a/chrome/common/extensions/docs/server2/memcache_file_system_test.py b/chrome/common/extensions/docs/server2/memcache_file_system_test.py
index 487b180f2fba74535515d0e46e74c4395f38f86a..da67d8d94948594541165ecb472f00a695387691 100755
--- a/chrome/common/extensions/docs/server2/memcache_file_system_test.py
+++ b/chrome/common/extensions/docs/server2/memcache_file_system_test.py
@@ -8,16 +8,47 @@ import unittest
 
 import appengine_memcache as memcache
 from appengine_memcache import AppEngineMemcache
+from file_system import FileSystem
+from future import Future
 from local_file_system import LocalFileSystem
 from memcache_file_system import MemcacheFileSystem
 
-class LocalFileSystemTest(unittest.TestCase):
+class _FakeFileSystem(FileSystem):
+  def __init__(self):
+    self.stat_value = 0
+    self.fetch_tracker = False
+    self.stat_fetch_tracker = False

[not at google - send to devlin, 2012/08/13 05:34:15]

what's the difference between fetch_tracker and stat_fetch_tracker? seems like
it should be read_fetch_tracker vs stat_fetch_tracker.

That said, a more common idiom would be to count calls to the methods, providing
a Reset method. Like read_counter and stat_counter.

Another idiom I've seen (and quite like) is combining the resetting with the
querying. That reduces the chance of forgetting to call Reset and also the
verbosity of the tests.

So like, make read/stat counter private, then have

def CheckAndReset(self, read_count=0, stat_count=0):
  try:
    return (self._read_count == read_count and
            self._stat_count == stat_count)
  finally:
    self._read_count = 0
    self._stat_count = 0

then tests can call

self.assertTrue(fake_fs.CheckAndReset(read_count=1, stat_count=0))

etc.

[cduvall, 2012/08/13 19:45:45]

Done.

+
+  def Stat(self, path):
+    self.stat_fetch_tracker = True
+    return self.StatInfo(
+        self.stat_value,
+        dict((path + str(i), self.stat_value) for i in range(5)))
+
+  def Read(self, paths, binary=False):
+    if paths:

[not at google - send to devlin, 2012/08/13 05:34:15]

Is this so that if read is called with an empty object, it doesn't trigger
fetched?

Sounds fishy. Why does it happen? We should fix the calling classes.

[cduvall, 2012/08/13 19:45:45]

Done.

+      self.fetch_tracker = True
+    return Future(value= dict((path, path) for path in paths))

[not at google - send to devlin, 2012/08/13 05:34:15]

nit: extra space after value=

[cduvall, 2012/08/13 19:45:45]

Done.

+
+class MemcacheFileSystemTest(unittest.TestCase):
   def setUp(self):
     self._memcache = AppEngineMemcache('')
     self._file_system = MemcacheFileSystem(
         LocalFileSystem(os.path.join('test_data', 'file_system')),
         self._memcache)
 
+  def _SetCacheItem(self, key, value, stat):

[not at google - send to devlin, 2012/08/13 05:34:15]

would prefer this be SetReadCacheItem since there's SetStatCacheItem (ditto
delete).

[cduvall, 2012/08/13 19:45:45]

Done.

+    self._memcache.Set(key, (value, stat), memcache.MEMCACHE_FILE_SYSTEM_READ)
+
+  def _SetStatCacheItem(self, key, value):
+    self._memcache.Set(key, value, memcache.MEMCACHE_FILE_SYSTEM_STAT)
+
+  def _DeleteCacheItem(self, key):
+    self._memcache.Delete(key, memcache.MEMCACHE_FILE_SYSTEM_READ)
+
+  def _DeleteStatCacheItem(self, key):
+    self._memcache.Delete(key, memcache.MEMCACHE_FILE_SYSTEM_STAT)
+
   def testReadFiles(self):
     expected = {
       'test1.txt': 'test1\n',
@@ -35,11 +66,60 @@ class LocalFileSystemTest(unittest.TestCase):
     self.assertEqual(expected,
                      sorted(self._file_system.ReadSingle('list/')))
     expected.remove('file0.html')
-    self._memcache.Set('list/',
-                       (expected, self._file_system.Stat('list/')),
-                       memcache.MEMCACHE_FILE_SYSTEM_READ)
+    self._SetCacheItem('list/', expected, self._file_system.Stat('list/'))
     self.assertEqual(expected,
                      sorted(self._file_system.ReadSingle('list/')))
 
+  def testCaching(self):
+    fake_fs = _FakeFileSystem()
+    self._file_system = MemcacheFileSystem(fake_fs, self._memcache)

[not at google - send to devlin, 2012/08/13 05:34:15]

if you find yourself doing this, sounds like _file_system shouldn't be a member
variable.

[cduvall, 2012/08/13 19:45:45]

Done.

+    self.assertEqual('bob', self._file_system.ReadSingle('bob'))
+    self.assertTrue(fake_fs.fetch_tracker)
+    fake_fs.fetch_tracker = False
+
+    # Test resource is not re-fetched.

[not at google - send to devlin, 2012/08/13 05:34:15]

"Resource has been memcached, so test resource is not re-fetched."

[cduvall, 2012/08/13 19:45:45]

Done.

+    self.assertEqual('bob', self._file_system.ReadSingle('bob'))
+    self.assertFalse(fake_fs.fetch_tracker)
+
+    # Test if the Stat version is the same the resource is not re-fetched.
+    self._DeleteStatCacheItem('bob')
+    self.assertEqual('bob', self._file_system.ReadSingle('bob'))
+    self.assertFalse(fake_fs.fetch_tracker)
+
+    # Test if there is a newer version, the resource is re-fetched.
+    self._DeleteStatCacheItem('bob')
+    fake_fs.stat_value += 1
+    self.assertEqual('bob', self._file_system.ReadSingle('bob'))
+    self.assertTrue(fake_fs.fetch_tracker)
+    fake_fs.fetch_tracker = False
+
+    # Test directory and subdirectory stats are cached.
+    self.assertEqual('bob/', self._file_system.ReadSingle('bob/'))
+    fake_fs.stat_fetch_tracker = False
+    self.assertEqual('bob/bob1', self._file_system.ReadSingle('bob/bob1'))
+    self.assertFalse(fake_fs.stat_fetch_tracker)
+    self.assertEqual('bob/', self._file_system.ReadSingle('bob/'))
+    self.assertFalse(fake_fs.stat_fetch_tracker)
+
+    # Test a more recent parent directory doesn't force a refetch of children.
+    self.assertEqual('bob/bob0', self._file_system.ReadSingle('bob/bob0'))
+    self.assertEqual('bob/bob1', self._file_system.ReadSingle('bob/bob1'))
+    self.assertEqual('bob/bob2', self._file_system.ReadSingle('bob/bob2'))
+    self._SetStatCacheItem('bob/', 10)
+    fake_fs.fetch_tracker = False
+    self.assertEqual('bob/bob0', self._file_system.ReadSingle('bob/bob0'))
+    self.assertEqual('bob/bob1', self._file_system.ReadSingle('bob/bob1'))
+    self.assertEqual('bob/bob2', self._file_system.ReadSingle('bob/bob2'))
+    self.assertFalse(fake_fs.fetch_tracker)
+
+    self._DeleteStatCacheItem('bob/bob0')
+    fake_fs.fetch_tracker = False
+    self.assertEqual('bob/bob0', self._file_system.ReadSingle('bob/bob0'))
+    self.assertFalse(fake_fs.fetch_tracker)
+    fake_fs.stat_fetch_tracker = False
+    self.assertEqual('bob/bob0', self._file_system.ReadSingle('bob/bob0'))
+    self.assertFalse(fake_fs.stat_fetch_tracker)
+    self.assertFalse(fake_fs.fetch_tracker)
+
 if __name__ == '__main__':
   unittest.main()