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

nsoffer at redhat.com nsoffer at redhat.com
Wed Nov 26 14:54:49 UTC 2014


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: 


-- 
To view, visit http://gerrit.ovirt.org/34709
To unsubscribe, visit http://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I6dd8fb29d94286e3e3a3e29b8218501cbdc5c018
Gerrit-PatchSet: 5
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Nir Soffer <nsoffer at redhat.com>
Gerrit-Reviewer: Adam Litke <alitke at redhat.com>
Gerrit-Reviewer: Antoni Segura Puimedon <asegurap at redhat.com>
Gerrit-Reviewer: Dan Kenigsberg <danken at redhat.com>
Gerrit-Reviewer: Federico Simoncelli <fsimonce at redhat.com>
Gerrit-Reviewer: Francesco Romani <fromani at redhat.com>
Gerrit-Reviewer: Martin Sivák <msivak at redhat.com>
Gerrit-Reviewer: Nir Soffer <nsoffer at redhat.com>
Gerrit-Reviewer: Saggi Mizrahi <smizrahi at redhat.com>
Gerrit-Reviewer: Vitor de Lima <vdelima at redhat.com>
Gerrit-Reviewer: automation at ovirt.org
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: Yes


More information about the vdsm-patches mailing list