Change in vdsm[master]: cache: Add caching decorator with invalidation

[Nir Soffer]

Nir Soffer has uploaded a new change for review.

Change subject: cache: Add caching decorator with invalidation
......................................................................

cache: Add caching decorator with invalidation

The new cache.memoized extends utils.memoized, adding invalidation
support.

Features added:

- An optional "validate" argument. This is a callable invoked each time
  the memoized function is called. When the callable returns False, the
  cache is invalidated.

- Memoized functions have an "invalidate" method, used to invalidate the
  cache during testing.

- file_validator - invalidates the cache when a file changes.

Example usage:

from vdsm.cache import memoized, file_validator

@memoized(file_validator('/bigfile'))
    def parse_bigfile():
        # Expensive code processing '/bigfile' contents

Change-Id: I6dd8fb29d94286e3e3a3e29b8218501cbdc5c018
Signed-off-by: Nir Soffer 
---
M lib/vdsm/Makefile.am
A lib/vdsm/cache.py
M tests/Makefile.am
A tests/cacheTests.py
4 files changed, 366 insertions(+), 0 deletions(-)

git pull ssh://gerrit.ovirt.org:29418/vdsm refs/changes/09/34709/1

diff --git a/lib/vdsm/Makefile.am b/lib/vdsm/Makefile.am
index b862e71..6f0040d 100644
--- a/lib/vdsm/Makefile.am
+++ b/lib/vdsm/Makefile.am
@@ -23,6 +23,7 @@

dist_vdsmpylib_PYTHON = \
    __init__.py \
+	cache.py \
    compat.py \
    define.py \
    exception.py \
diff --git a/lib/vdsm/cache.py b/lib/vdsm/cache.py
new file mode 100644
index 0000000..9806e40
--- /dev/null
+++ b/lib/vdsm/cache.py
@@ -0,0 +1,98 @@
+#
+# Copyright 2014 Red Hat, Inc.
+#
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 2 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program; if not, write to the Free Software
+# Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA
+#
+# Refer to the README and COPYING files for full details of the license
+#
+
+import errno
+import os
+import functools
+
+
+def memoized(validate=None):
+    """
+    Return a caching decorator supporting invalidation.
+
+    The decorator accepts an optional validate callable, called each time the
+    memoized function is called. If the validate callable return True, the
+    memoized function will use the cache. If the validate callable return
+    False, the memoized cache is cleared.
+
+    The memoized function may accept multiple positional arguments. The
+    cache store the result for each combination of arguments. Functions with
+    kwargs are not supported.
+
+    Memoized functions have an "invalidate" method, used to invalidate the
+    memoized cache during testing.
+
+    To invalidate the cache when a file changes, use the file_validator from
+    this module.
+
+    Example usage:
+
+        from vdsm.cache import memoized, file_validator
+
+        @memoized(file_validator('/bigfile'))
+        def parse_bigfile():
+            # Expensive code processing '/bigfile' contents
+
+    """
+    def decorator(f):
+        cache = {}
+
+        @functools.wraps(f)
+        def wrapper(*args):
+            if validate is not None and not validate():
+                cache.clear()
+            try:
+                value = cache[args]
+            except KeyError:
+                value = cache[args] = f(*args)
+            return value
+
+        wrapper.invalidate = cache.clear
+        return wrapper
+
+    return decorator
+
+
+class file_validator(object):
+    """
+    I'm a validator returning False when a file has changed since the last
+    validation.
+    """
+
+    UNKNOWN = 0
+    MISSING = 1
+
+    def __init__(self, path):
+        self.path = path
+        self.stats = self.UNKNOWN
+
+    def __call__(self):
+        try:
+            stats = os.stat(self.path)
+        except OSError as e:
+            if e.errno != errno.ENOENT:
+                raise
+            stats = self.MISSING
+        else:
+            stats = stats.st_ino, stats.st_size, stats.st_mtime
+        if stats != self.stats:
+            self.stats = stats
+            return False
+        return True
diff --git a/tests/Makefile.am b/tests/Makefile.am
index 36a1cdd..6fa7e64 100644
--- a/tests/Makefile.am
+++ b/tests/Makefile.am
@@ -26,6 +26,7 @@
    alignmentScanTests.py \
    blocksdTests.py \
    bridgeTests.py \
+	cacheTests.py \
    cPopenTests.py \
    capsTests.py \
    clientifTests.py \
diff --git a/tests/cacheTests.py b/tests/cacheTests.py
new file mode 100644
index 0000000..8927b39
--- /dev/null
+++ b/tests/cacheTests.py
@@ -0,0 +1,266 @@
+#
+# Copyright 2014 Red Hat, Inc.
+#
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 2 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program; if not, write to the Free Software
+# Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA
+# 02110-1301  USA
+#
+# Refer to the README and COPYING files for full details of the license
+#
+
+import os
+from vdsm.cache import memoized
+from vdsm.cache import file_validator
+from testlib import VdsmTestCase
+from testlib import namedTemporaryDir
+
+
+class Validator(object):
+    """ I'm a callable returning a boolean value (self.valid) """
+
+    def __init__(self):
+        self.valid = True
+        self.count = 0
+
+    def __call__(self):
+        self.count += 1
+        return self.valid
+
+
+class Accessor(object):
+    """ I'm recording how many times a dict was accessed. """
+
+    def __init__(self, d):
+        self.d = d
+        self.count = 0
+
+    def get(self, key):
+        self.count += 1
+        return self.d[key]
+
+
+class MemoizedTests(VdsmTestCase):
+
+    def setUp(self):
+        self.values = {'a': 0, 'b': 10, ('a',): 20, ('a', 'b'): 30}
+
+    def test_no_args(self):
+        accessor = Accessor(self.values)
+
+        @memoized()
+        def func(key):
+            return accessor.get(key)
+
+        # Fill the cache
+        self.assertEqual(func('a'), self.values['a'])
+        self.assertEqual(func('b'), self.values['b'])
+        self.assertEqual(accessor.count, 2)
+
+        # Values served now from the cache
+        self.assertEqual(func('a'), self.values['a'])
+        self.assertEqual(func('b'), self.values['b'])
+        self.assertEqual(accessor.count, 2)
+
+    def test_validation(self):
+        accessor = Accessor(self.values)
+        validator = Validator()
+
+        @memoized(validator)
+        def func(key):
+            return accessor.get(key)
+
+        # Fill the cache
+        self.assertEqual(func('a'), self.values['a'])
+        self.assertEqual(func('b'), self.values['b'])
+        self.assertEqual(accessor.count, 2)
+        self.assertEqual(validator.count, 2)
+
+        # Values served now from the cache
+        self.assertEqual(func('a'), self.values['a'])
+        self.assertEqual(func('b'), self.values['b'])
+        self.assertEqual(accessor.count, 2)
+        self.assertEqual(validator.count, 4)
+
+        # Values has changed
+        self.values['a'] += 1
+        self.values['b'] += 1
+
+        # Next call should clear the cache
+        validator.valid = False
+        self.assertEqual(func('a'), self.values['a'])
+        self.assertEqual(accessor.count, 3)
+        self.assertEqual(validator.count, 5)
+
+        # Next call should add next value to cache
+        validator.valid = True
+        self.assertEqual(func('b'), self.values['b'])
+        self.assertEqual(accessor.count, 4)
+        self.assertEqual(validator.count, 6)
+
+        # Values served now from the cache
+        self.assertEqual(func('a'), self.values['a'])
+        self.assertEqual(func('b'), self.values['b'])
+        self.assertEqual(accessor.count, 4)
+        self.assertEqual(validator.count, 8)
+
+    def test_raise_errors_in_memoized_func(self):
+        accessor = Accessor(self.values)
+        validator = Validator()
+
+        @memoized(validator)
+        def func(key):
+            return accessor.get(key)
+
+        # First run should fail, second shold fill the cache
+        self.assertRaises(KeyError, func, 'no such key')
+        self.assertEqual(func('a'), self.values['a'])
+        self.assertEqual(accessor.count, 2)
+        self.assertEqual(validator.count, 2)
+
+    def test_multiple_args(self):
+        accessor = Accessor(self.values)
+
+        @memoized()
+        def func(*args):
+            return accessor.get(args)
+
+        # Fill the cache
+        self.assertEqual(func('a'), self.values[('a',)])
+        self.assertEqual(func('a', 'b'), self.values[('a', 'b')])
+        self.assertEqual(accessor.count, 2)
+
+        # Values served now from the cache
+        self.assertEqual(func('a'), self.values[('a',)])
+        self.assertEqual(func('a', 'b'), self.values[('a', 'b')])
+        self.assertEqual(accessor.count, 2)
+
+    def test_kwargs_not_supported(self):
+        @memoized()
+        def func(a=None, b=None):
+            pass
+        self.assertRaises(TypeError, func, a=1, b=2)
+
+    def test_invalidate(self):
+        accessor = Accessor(self.values)
+
+        @memoized()
+        def func(key):
+            return accessor.get(key)
+
+        self.assertEqual(func('a'), self.values['a'])
+        self.assertEqual(accessor.count, 1)
+
+        func.invalidate()
+
+        self.assertEqual(func('a'), self.values['a'])
+        self.assertEqual(accessor.count, 2)
+
+
+class FileValidatorTests(VdsmTestCase):
+
+    def test_no_file(self):
+        with namedTemporaryDir() as tempdir:
+            path = os.path.join(tempdir, 'data')
+            validator = file_validator(path)
+
+            # Must be False so memoise call the decorated function
+            self.assertEqual(validator(), False)
+
+            # Since file state did not change, must remain True
+            self.assertEqual(validator(), True)
+
+    def test_file_created(self):
+        with namedTemporaryDir() as tempdir:
+            path = os.path.join(tempdir, 'data')
+            validator = file_validator(path)
+
+            self.assertEqual(validator(), False)
+            self.assertEqual(validator(), True)
+
+            with open(path, 'w') as f:
+                f.write('data')
+
+            self.assertEqual(validator(), False)
+            self.assertEqual(validator(), True)
+
+    def test_file_removed(self):
+        with namedTemporaryDir() as tempdir:
+            path = os.path.join(tempdir, 'data')
+            validator = file_validator(path)
+
+            with open(path, 'w') as f:
+                f.write('data')
+
+            self.assertEqual(validator(), False)
+            self.assertEqual(validator(), True)
+
+            os.unlink(path)
+
+            self.assertEqual(validator(), False)
+            self.assertEqual(validator(), True)
+
+    def test_size_changed(self):
+        with namedTemporaryDir() as tempdir:
+            path = os.path.join(tempdir, 'data')
+            validator = file_validator(path)
+            data = 'old data'
+            with open(path, 'w') as f:
+                f.write(data)
+
+            self.assertEqual(validator(), False)
+            self.assertEqual(validator(), True)
+
+            with open(path, 'w') as f:
+                f.write(data + ' new data')
+
+            self.assertEqual(validator(), False)
+            self.assertEqual(validator(), True)
+
+    def test_mtime_changed(self):
+        with namedTemporaryDir() as tempdir:
+            path = os.path.join(tempdir, 'data')
+            validator = file_validator(path)
+            data = 'old data'
+            with open(path, 'w') as f:
+                f.write(data)
+
+            self.assertEqual(validator(), False)
+            self.assertEqual(validator(), True)
+
+            # Fake timestamp change, as timestamp resolution may not be good
+            # enough when comparing changes during the test.
+            atime = mtime = os.path.getmtime(path) + 1
+            os.utime(path, (atime, mtime))
+
+            self.assertEqual(validator(), False)
+            self.assertEqual(validator(), True)
+
+    def test_ino_changed(self):
+        with namedTemporaryDir() as tempdir:
+            path = os.path.join(tempdir, 'data')
+            validator = file_validator(path)
+            data = 'old data'
+            with open(path, 'w') as f:
+                f.write(data)
+
+            self.assertEqual(validator(), False)
+            self.assertEqual(validator(), True)
+
+            tmp = path + '.tmp'
+            with open(tmp, 'w') as f:
+                f.write(data)
+            os.rename(tmp, path)
+
+            self.assertEqual(validator(), False)
+            self.assertEqual(validator(), True)

[Jenkins CI RO]

oVirt Jenkins CI Server has posted comments on this change.

Change subject: cache: Add caching decorator with invalidation
......................................................................

Patch Set 1:

Build Failed

http://jenkins.ovirt.org/job/vdsm_master_unit-tests_created/13314/ : SUCCESS

http://jenkins.ovirt.org/job/vdsm_master_pep8_gerrit/13153/ : SUCCESS

http://jenkins.ovirt.org/job/vdsm_master_unit_tests_gerrit_el/12363/ : FAILURE

[Nir Soffer]

Nir Soffer has posted comments on this change.

Change subject: cache: Add caching decorator with invalidation
......................................................................

Patch Set 1: Verified+1

Saggi Mizrahi has posted comments on this change.

Change subject: cache: Add caching decorator with invalidation
......................................................................

Patch Set 1: Code-Review-1

There is already a memoized() in utils.py.
There should only be one. Please either unify both or name them so that users know when to use either.

Also, this is infra so the module should be under /lib/vdsm/infra/ with it's tests next to it. Also add entry in the documentation index.

Just look at how zombiereaper is maintained for an example.

Further more, I understand that the term in\validate is because you are talking about the cache, but I think having:

@memoized(update_on=file_modified("/etc/passwd"))

Gives a better explanation on expected behaviour.

Saggi Mizrahi has posted comments on this change.

Change subject: cache: Add caching decorator with invalidation
......................................................................

Patch Set 1:

Just noticed that future patches unify memoized(). So just fix the other sutff.

Saggi Mizrahi has posted comments on this change.

Change subject: cache: Add caching decorator with invalidation
......................................................................

Patch Set 1:

Also have your memozied work parameterless as well.

[Nir Soffer]

Nir Soffer has posted comments on this change.

Change subject: cache: Add caching decorator with invalidation
......................................................................

Patch Set 2: Verified+1

Version 2 address Saggi comments:
- Beautify the api
- Add missing tests for memoizing function without arguments
- Replace assertEqual(value, bool) with assertTrue/False

[Jenkins CI RO]

oVirt Jenkins CI Server has posted comments on this change.

Change subject: cache: Add caching decorator with invalidation
......................................................................

Patch Set 2:

Build Failed

http://jenkins.ovirt.org/job/vdsm_master_unit-tests_created/13327/ : SUCCESS

http://jenkins.ovirt.org/job/vdsm_master_pep8_gerrit/13167/ : SUCCESS

http://jenkins.ovirt.org/job/vdsm_master_unit_tests_gerrit_el/12377/ : FAILURE

[Nir Soffer]

Nir Soffer has posted comments on this change.

Change subject: cache: Add caching decorator with invalidation
......................................................................

Patch Set 2:

Saggi, I'm not sure how do you want to move this into the infra package, so lets do this later.

Francesco Romani has posted comments on this change.

Change subject: cache: Add caching decorator with invalidation
......................................................................

Patch Set 2: Code-Review+1

(1 comment)

Nice! Added a minor comment for a possible improvement.

http://gerrit.ovirt.org/#/c/34709/2/lib/vdsm/cache.py
File lib/vdsm/cache.py:

Line 22: import os
Line 23: import functools
Line 24: 
Line 25: 
Line 26: def memoized(update=None):
The API is really nice, but I find this 'update' a too generic name.
What about 'update_on' (as Saggi suggested) or 'update_if'?
Line 27:     """
Line 28:     Return a caching decorator supporting invalidation.
Line 29: 
Line 30:     The decorator accepts an optional "update" callable, called each time the

[Nir Soffer]

Nir Soffer has posted comments on this change.

Change subject: cache: Add caching decorator with invalidation
......................................................................

Patch Set 2:

(1 comment)

http://gerrit.ovirt.org/#/c/34709/2/lib/vdsm/cache.py
File lib/vdsm/cache.py:

Line 22: import os
Line 23: import functools
Line 24: 
Line 25: 
Line 26: def memoized(update=None):
> The API is really nice, but I find this 'update' a too generic name.

update_on looks nice when you define decorate:

@memoize(update_on=file_modified())

But it is not as nice when used inside the decorator:

if update_on is not None and update_on():
        cache.clear()

I think that replacing "validate" with "update" is a good way to keep everyone happy.
Line 27:     """
Line 28:     Return a caching decorator supporting invalidation.
Line 29: 
Line 30:     The decorator accepts an optional "update" callable, called each time the

Saggi Mizrahi has posted comments on this change.

Change subject: cache: Add caching decorator with invalidation
......................................................................

Patch Set 2: Code-Review-1

(1 comment)

Please add: doc/infra/cache.rst

And make the content:

Cache
 =============

.. automodule:: vdsm.infra.cache
         :members:

Also, modify: doc/infra/index.rst

So it appropriately lists the new cache module.

Also, if you feel industrious, Add module docstring explaining how to create new validators. But it's OK if you want to skip on that last thing.

Sorry for all the extra work but we are trying to get the infra documented and discoverable so that new developers can quickly get a list of all the tools we provide.

http://gerrit.ovirt.org/#/c/34709/2/lib/vdsm/cache.py
File lib/vdsm/cache.py:

Line 22: import os
Line 23: import functools
Line 24: 
Line 25: 
Line 26: def memoized(update=None):
> update_on looks nice when you define decorate:

I'm OK with the compromise. It's a parameter so the context is always near.

In any case, you could have internally just do:

update = update_on
 del update_on
Line 27:     """
Line 28:     Return a caching decorator supporting invalidation.
Line 29: 
Line 30:     The decorator accepts an optional "update" callable, called each time the

[Nir Soffer]

Nir Soffer has posted comments on this change.

Change subject: cache: Add caching decorator with invalidation
......................................................................

Patch Set 2:

I'll add documentation and improve the docstrings in next version.

Thanks for the quick review!

[Jenkins CI RO]

oVirt Jenkins CI Server has posted comments on this change.

Change subject: cache: Add caching decorator with invalidation
......................................................................

Patch Set 3:

Build Failed

http://jenkins.ovirt.org/job/vdsm_master_unit-tests_created/13352/ : SUCCESS

http://jenkins.ovirt.org/job/vdsm_master_pep8_gerrit/13191/ : SUCCESS

http://jenkins.ovirt.org/job/vdsm_master_unit_tests_gerrit_el/12401/ : FAILURE

[Nir Soffer]

Nir Soffer has posted comments on this change.

Change subject: cache: Add caching decorator with invalidation
......................................................................

Patch Set 3: Verified+1

Version 3 changes:
- Add documentation to doc/cache.rst - should move to infra/cache.rst when the module is moved into infra package.
- Add module docstring
- Improve tests documenation
- Simplify file_modified, was keeping unneeded state (unknown stats, missing file)
- Add test for memoized(file_modified(path)) test

[Nir Soffer]

Nir Soffer has posted comments on this change.

Change subject: cache: Add caching decorator with invalidation
......................................................................

Patch Set 4: Verified+1

Version 4 refine cache module documentation so it looks better in sphinx output.

[Jenkins CI RO]

oVirt Jenkins CI Server has posted comments on this change.

Change subject: cache: Add caching decorator with invalidation
......................................................................

Patch Set 4:

Build Failed

http://jenkins.ovirt.org/job/vdsm_master_unit-tests_created/13356/ : SUCCESS

http://jenkins.ovirt.org/job/vdsm_master_pep8_gerrit/13195/ : SUCCESS

http://jenkins.ovirt.org/job/vdsm_master_unit_tests_gerrit_el/12405/ : FAILURE

Saggi Mizrahi has posted comments on this change.

Change subject: cache: Add caching decorator with invalidation
......................................................................

Patch Set 4: Code-Review-1

Really sorry for bothering you again but you didn't update 'doc/infra/index.rst' to include you module in the list.

Saggi Mizrahi has posted comments on this change.

Change subject: cache: Add caching decorator with invalidation
......................................................................

Patch Set 4:

Also noticed it's under lib/vdsm and not lib/vdsm/infra where it should be

[Nir Soffer]

Nir Soffer has posted comments on this change.

Change subject: cache: Add caching decorator with invalidation
......................................................................

Patch Set 4:

The module is currently not in the infra package, so there is no need to update doc/infra.rst.

As I mentioned before, it is not clear how do you want to move this into infra, so we should do this later.

For qemuimg module, which is the reason I added this utility, this works nicely as is, and we don't have any need to move this into the infra package.

[Federico Simoncelli]

Federico Simoncelli has posted comments on this change.

Change subject: cache: Add caching decorator with invalidation
......................................................................

Patch Set 4: Code-Review+1

Saggi, last time we spoke I remember you suggested that each subsystem should start creating/using their own libs and then the worthy common code would have been moved to infra.

According to that, and according to where the previous memoized decorator was defined I don't think that it's Nir's duty to move it to infra.

[Federico Simoncelli]

Federico Simoncelli has posted comments on this change.

Change subject: cache: Add caching decorator with invalidation
......................................................................

Patch Set 4:

(1 comment)

http://gerrit.ovirt.org/#/c/34709/4/lib/vdsm/cache.py
File lib/vdsm/cache.py:

Line 100:             stats = os.stat(self.path)
Line 101:         except OSError as e:
Line 102:             if e.errno != errno.ENOENT:
Line 103:                 raise
Line 104:             stats = None
I think in this case you should return True.
Line 105:         else:
Line 106:             stats = stats.st_ino, stats.st_size, stats.st_mtime
Line 107:         if stats != self.stats:
Line 108:             self.stats = stats

[Nir Soffer]

Nir Soffer has posted comments on this change.

Change subject: cache: Add caching decorator with invalidation
......................................................................

Patch Set 4:

(1 comment)

http://gerrit.ovirt.org/#/c/34709/4/lib/vdsm/cache.py
File lib/vdsm/cache.py:

Line 100:             stats = os.stat(self.path)
Line 101:         except OSError as e:
Line 102:             if e.errno != errno.ENOENT:
Line 103:                 raise
Line 104:             stats = None
> I think in this case you should return True.

I don't think so. file_modified should invalidate the cache when a file was modified. If the file was missing the previous call, and is still missing, there is no need to invalidate the cache, since no modification happened.

For case when missing file should invalidate the cache on each call you can use another validator. I don't see a real use case that needs this.
Line 105:         else:
Line 106:             stats = stats.st_ino, stats.st_size, stats.st_mtime
Line 107:         if stats != self.stats:
Line 108:             self.stats = stats

[Dan Kenigsberg]

Dan Kenigsberg has posted comments on this change.

Change subject: cache: Add caching decorator with invalidation
......................................................................

Patch Set 4:

(2 comments)

http://gerrit.ovirt.org/#/c/34709/4/lib/vdsm/cache.py
File lib/vdsm/cache.py:

Line 42:     """
Line 43:     Return a caching decorator supporting invalidation.
Line 44: 
Line 45:     The decorator accepts an optional "update" callable, called each time the
Line 46:     memoized function is called. If update() return True, the memoized cache is
update() return True -> update() returns True
Line 47:     invalidated. If the update is not set, the cache is never invalidated.
Line 48: 
Line 49:     The memoized function may accept multiple positional arguments. The
Line 50:     cache store the result for each combination of arguments. Functions with

Line 46:     memoized function is called. If update() return True, the memoized cache is
Line 47:     invalidated. If the update is not set, the cache is never invalidated.
Line 48: 
Line 49:     The memoized function may accept multiple positional arguments. The
Line 50:     cache store the result for each combination of arguments. Functions with
store->stores
Line 51:     kwargs are not supported.
Line 52: 
Line 53:     Memoized functions have an "invalidate" method, used to invalidate the
Line 54:     cache externally. This is required for testing memoized functions.

Vitor de Lima has posted comments on this change.

Change subject: cache: Add caching decorator with invalidation
......................................................................

Patch Set 4:

(3 comments)

http://gerrit.ovirt.org/#/c/34709/4/lib/vdsm/cache.py
File lib/vdsm/cache.py:

Line 42:     """
Line 43:     Return a caching decorator supporting invalidation.
Line 44: 
Line 45:     The decorator accepts an optional "update" callable, called each time the
Line 46:     memoized function is called. If update() return True, the memoized cache is
> update() return True -> update() returns True

Done
Line 47:     invalidated. If the update is not set, the cache is never invalidated.
Line 48: 
Line 49:     The memoized function may accept multiple positional arguments. The
Line 50:     cache store the result for each combination of arguments. Functions with

Line 46:     memoized function is called. If update() return True, the memoized cache is
Line 47:     invalidated. If the update is not set, the cache is never invalidated.
Line 48: 
Line 49:     The memoized function may accept multiple positional arguments. The
Line 50:     cache store the result for each combination of arguments. Functions with
> store->stores

Done
Line 51:     kwargs are not supported.
Line 52: 
Line 53:     Memoized functions have an "invalidate" method, used to invalidate the
Line 54:     cache externally. This is required for testing memoized functions.

Line 100:             stats = os.stat(self.path)
Line 101:         except OSError as e:
Line 102:             if e.errno != errno.ENOENT:
Line 103:                 raise
Line 104:             stats = None
> I don't think so. file_modified should invalidate the cache when a file was

Agreed, the current implementation also considers file creation and removal as update events, which sounds like a proper behavior.
Line 105:         else:
Line 106:             stats = stats.st_ino, stats.st_size, stats.st_mtime
Line 107:         if stats != self.stats:
Line 108:             self.stats = stats

[Nir Soffer]

Nir Soffer has posted comments on this change.

Change subject: cache: Add caching decorator with invalidation
......................................................................

Patch Set 5:

(6 comments)

http://gerrit.ovirt.org/#/c/34709/5/lib/vdsm/cache.py
File lib/vdsm/cache.py:

Line 44:     Return a caching decorator supporting invalidation.
Line 45: 
Line 46:     The decorator accepts an optional "update" callable, called each time the
Line 47:     memoized function is called. If update() returns True, the memoized cache
Line 48:     is invalidated. If the update is not set, the cache is never invalidated.
Wearing my reviewer hat, I see that "If the update is not set" should be "If update is not set".
Line 49: 
Line 50:     The memoized function may accept multiple positional arguments. The
Line 51:     cache stores the result for each combination of arguments. Functions with
Line 52:     kwargs are not supported.

Line 88: 
Line 89: class file_modified(object):
Line 90:     """
Line 91:     A memoized validator returning True when a file has changed since the
Line 92:     last validation.
Maybe add here that this validator assumes that the file system report that file size and mtime has changed when file contents changed.
Line 93:     """
Line 94: 
Line 95:     def __init__(self, path):
Line 96:         self.path = path

Line 115:     """
Line 116:     A memoized validator returning True when the contents of a small file have
Line 117:     changed since the last validation. Used, for example, to monitor files in
Line 118:     /sys which do not properly expose status like regular files.
Line 119:     """
I assume that by reading the contents of a small file, you save expensive computation or reading some other bigger files - right?
Line 120: 
Line 121:     def __init__(self, path):
Line 122:         self.path = path
Line 123:         self.contents = None

http://gerrit.ovirt.org/#/c/34709/5/tests/cacheTests.py
File tests/cacheTests.py:

Line 216:     def test_update_when_file_modified(self):
Line 217:         self._update_file_content_and_verify(file_modified)
Line 218: 
Line 219:     def test_update_when_file_content_changed(self):
Line 220:         self._update_file_content_and_verify(file_content_changed)
I like having tests at the top, and private helpers at the bottom.
Line 221: 
Line 222: 
Line 223: @tools.nottest
Line 224: class FilePresenceTests(VdsmTestCase):

Line 219:     def test_update_when_file_content_changed(self):
Line 220:         self._update_file_content_and_verify(file_content_changed)
Line 221: 
Line 222: 
Line 223: @tools.nottest
I don't follow, what does this do on a class?
Line 224: class FilePresenceTests(VdsmTestCase):
Line 225: 
Line 226:     validator_class = None
Line 227:

Line 275:             # No change in stats
Line 276:             self.assertFalse(validator())
Line 277: 
Line 278: 
Line 279: @tools.istest
I don't follow, what does this do on a class?
Line 280: class FileModifiedTests(FilePresenceTests):
Line 281: 
Line 282:     validator_class = file_modified
Line 283:

[Jenkins CI RO]

oVirt Jenkins CI Server has posted comments on this change.

Change subject: cache: Add caching decorator with invalidation
......................................................................

Patch Set 5:

Build Failed

http://jenkins.ovirt.org/job/vdsm_master_unit_tests_gerrit_el/12876/ : FAILURE

http://jenkins.ovirt.org/job/vdsm_master_unit-tests_created/13828/ : SUCCESS

http://jenkins.ovirt.org/job/vdsm_master_pep8_gerrit/13665/ : SUCCESS

Vitor de Lima has posted comments on this change.

Change subject: cache: Add caching decorator with invalidation
......................................................................

Patch Set 5:

(3 comments)

http://gerrit.ovirt.org/#/c/34709/5/lib/vdsm/cache.py
File lib/vdsm/cache.py:

Line 115:     """
Line 116:     A memoized validator returning True when the contents of a small file have
Line 117:     changed since the last validation. Used, for example, to monitor files in
Line 118:     /sys which do not properly expose status like regular files.
Line 119:     """
> I assume that by reading the contents of a small file, you save expensive c

Sure, it will be used in the physical CPU hotplug for monitoring the /sys/devices/system/cpu/online for changes in the number of online CPUs.
Line 120: 
Line 121:     def __init__(self, path):
Line 122:         self.path = path
Line 123:         self.contents = None

http://gerrit.ovirt.org/#/c/34709/5/tests/cacheTests.py
File tests/cacheTests.py:

Line 219:     def test_update_when_file_content_changed(self):
Line 220:         self._update_file_content_and_verify(file_content_changed)
Line 221: 
Line 222: 
Line 223: @tools.nottest
> I don't follow, what does this do on a class?

This test is a reusable "abstract" test case, it you don't flag it as "nottest" it will run and fail, since there is not a validator_class set.
Line 224: class FilePresenceTests(VdsmTestCase):
Line 225: 
Line 226:     validator_class = None
Line 227:

Line 275:             # No change in stats
Line 276:             self.assertFalse(validator())
Line 277: 
Line 278: 
Line 279: @tools.istest
> I don't follow, what does this do on a class?

This overrides the "nottest" of the base class to mark these two classes as tests that should be executed.
Line 280: class FileModifiedTests(FilePresenceTests):
Line 281: 
Line 282:     validator_class = file_modified
Line 283:

Vitor de Lima has posted comments on this change.

Change subject: cache: Add caching decorator with invalidation
......................................................................

Patch Set 5:

(3 comments)

http://gerrit.ovirt.org/#/c/34709/5/lib/vdsm/cache.py
File lib/vdsm/cache.py:

Line 44:     Return a caching decorator supporting invalidation.
Line 45: 
Line 46:     The decorator accepts an optional "update" callable, called each time the
Line 47:     memoized function is called. If update() returns True, the memoized cache
Line 48:     is invalidated. If the update is not set, the cache is never invalidated.
> Wearing my reviewer hat, I see that "If the update is not set" should be "I

Done
Line 49: 
Line 50:     The memoized function may accept multiple positional arguments. The
Line 51:     cache stores the result for each combination of arguments. Functions with
Line 52:     kwargs are not supported.

Line 88: 
Line 89: class file_modified(object):
Line 90:     """
Line 91:     A memoized validator returning True when a file has changed since the
Line 92:     last validation.
> Maybe add here that this validator assumes that the file system report that

Done
Line 93:     """
Line 94: 
Line 95:     def __init__(self, path):
Line 96:         self.path = path

http://gerrit.ovirt.org/#/c/34709/5/tests/cacheTests.py
File tests/cacheTests.py:

Line 216:     def test_update_when_file_modified(self):
Line 217:         self._update_file_content_and_verify(file_modified)
Line 218: 
Line 219:     def test_update_when_file_content_changed(self):
Line 220:         self._update_file_content_and_verify(file_content_changed)
> I like having tests at the top, and private helpers at the bottom.

In the other tests the private helpers are at the beginning of the file, so I did the same thing in the new patch set.
Line 221: 
Line 222: 
Line 223: @tools.nottest
Line 224: class FilePresenceTests(VdsmTestCase):

Vitor de Lima has posted comments on this change.

Change subject: cache: Add caching decorator with invalidation
......................................................................

Patch Set 6: Verified+1

Verified using the unit tests.

[Jenkins CI RO]

oVirt Jenkins CI Server has posted comments on this change.

Change subject: cache: Add caching decorator with invalidation
......................................................................

Patch Set 6:

Build Failed

http://jenkins.ovirt.org/job/vdsm_master_unit_tests_gerrit_el/12924/ : FAILURE

http://jenkins.ovirt.org/job/vdsm_master_unit-tests_created/13876/ : SUCCESS

http://jenkins.ovirt.org/job/vdsm_master_pep8_gerrit/13713/ : SUCCESS

[Nir Soffer]

Nir Soffer has posted comments on this change.

Change subject: cache: Add caching decorator with invalidation
......................................................................

Patch Set 6:

(2 comments)

http://gerrit.ovirt.org/#/c/34709/6/tests/cacheTests.py
File tests/cacheTests.py:

Line 55: 
Line 56: 
Line 57: class MemoizedTests(VdsmTestCase):
Line 58: 
Line 59:     def _update_file_content_and_verify(self, validator):
This helper and the tests using it do not belong to this class. Lets remove them to MemoizedFileTests class.
Line 60:         with namedTemporaryDir() as tempdir:
Line 61:             path = os.path.join(tempdir, 'data')
Line 62:             read_count = [0]
Line 63:

Line 219:     def test_update_when_file_content_changed(self):
Line 220:         self._update_file_content_and_verify(file_content_changed)
Line 221: 
Line 222: 
Line 223: @tools.nottest
This usage of nottest is confusing. I think this can be more clear by implementing some helper methods (e.g. check_xxx) in the super class, and invoking them in the subclass.
Line 224: class FilePresenceTests(VdsmTestCase):
Line 225: 
Line 226:     validator_class = None
Line 227:

[Jenkins CI RO]

Jenkins CI RO has abandoned this change.

Change subject: cache: Add caching decorator with invalidation
......................................................................

Abandoned

Abandoned due to no activity - please restore if still relevant

[Jenkins CI RO]

Jenkins CI RO has posted comments on this change.

Change subject: cache: Add caching decorator with invalidation
......................................................................

Patch Set 6:

Abandoned due to no activity - please restore if still relevant

[e-mail] has posted comments on this change.

Change subject: cache: Add caching decorator with invalidation
......................................................................

Patch Set 6:

* Update tracker::IGNORE, no Bug-Url found

[Nir Soffer]

Nir Soffer has restored this change.

Change subject: cache: Add caching decorator with invalidation
......................................................................

Restored

Unabandon

[Jenkins CI RO]

Jenkins CI RO has abandoned this change.

Change subject: cache: Add caching decorator with invalidation
......................................................................

Abandoned

Abandoned due to no activity - please restore if still relevant

gerrit-hooks has posted comments on this change.

Change subject: cache: Add caching decorator with invalidation
......................................................................

Patch Set 6:

* Update tracker: IGNORE, no Bug-Url found

[Nir Soffer]

Nir Soffer has restored this change.

Change subject: cache: Add caching decorator with invalidation
......................................................................

Restored

Yaniv Bronhaim has posted comments on this change.

Change subject: cache: Add caching decorator with invalidation
......................................................................

Patch Set 6:

(1 comment)

https://gerrit.ovirt.org/#/c/34709/6/lib/vdsm/cache.py
File lib/vdsm/cache.py:

Line 1: #
Line 2: # Copyright 2014 Red Hat, Inc.
2016 - and please move this module to infra package
Line 3: #
Line 4: # This program is free software; you can redistribute it and/or modify
Line 5: # it under the terms of the GNU General Public License as published by
Line 6: # the Free Software Foundation; either version 2 of the License, or

Yaniv Bronhaim has posted comments on this change.

Change subject: cache: Add caching decorator with invalidation
......................................................................

Patch Set 6: Code-Review-1