Skip to content

[WIP] pack: Clear all non-current entries after pack #162

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

[WIP] pack: Clear all non-current entries after pack #162

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
32c9a7a
pack: Clear all non-current entries after pack
navytux Jul 29, 2020
69c5609
fixup! pack: Clear all non-current entries after pack
navytux Jul 31, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 10 additions & 1 deletion src/ZEO/ClientStorage.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -559,7 +559,16 @@ def pack(self, t=None, referencesf=None, wait=1, days=0):
if t is None:
t = time.time()
t = t - (days * 86400)
return self._call('pack', t, wait)
ret = self._call('pack', t, wait)
# remove all non-current entries from the cache.
# This way we make sure that loadBefore with before < packtime, won't
# return data from the cache, instead of returning "no data" if requested object
# has current revision >= packtime.
# By clearing all noncurrent entries we might remove more data from the
# cache than is strictly necessary, but since access to noncurrent data
# is seldom, that should not cause problems in practice.
self._cache.clearAllNonCurrent()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Personally, I would like to see a specific direct test case for this.

While I know that there was a generic ZODB test failure, it's not obvious that this is the fix for it. What I mean is, the symptom (a failure to raise a particular exception) and the fix (making sure there are no non-current items in the cache) aren't necessarily clearly linked. I could imagine that a slight refactoring of the ZODB test that introduces additional clients or changes the timing or whatever might let it pass without this line of code being present.

I would suggest that a direct test for this would (a) establish non-current items in the cache and verify their existence in cache.noncurrent; (b) pack; (c) verify that cache.noncurrent is empty or equivalent.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree, especially in the light of the bug you point out that the cache is not invalidated on other clients.

return ret

def store(self, oid, serial, data, version, txn):
"""Storage API: store data for an object."""
Expand Down
15 changes: 15 additions & 0 deletions src/ZEO/cache.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -246,6 +246,21 @@ def clear(self):
self.f.truncate()
self._initfile(ZEC_HEADER_SIZE)

# clearAllNonCurrent removes all non-current entries from the cache.
def clearAllNonCurrent(self):
with self._lock:
f = self.f
for oid, tidofs in self.noncurrent.items():
for (tid, ofs) in tidofs.items():
f.seek(ofs)
status = f.read(1)
assert status == b'a', (ofs, f.tell(), oid, tid)
f.seek(ofs)
f.write(b'f')
self._len -= 1

self.noncurrent.clear()

##
# Scan the current contents of the cache file, calling `install`
# for each object found in the cache. This method should only
Expand Down
28 changes: 28 additions & 0 deletions src/ZEO/tests/test_cache.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -253,6 +253,34 @@ def test_clear_zeo_cache(self):
self.assertEqual(cache.load(n3), None)
self.assertEqual(cache.loadBefore(n3, n2), None)

def testClearAllNonCurrent(self):
cache = self.cache
cache.store(p64(1), n1, n2, b'1@1')
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One space following a comma, please. Please don't introduce extra horizontal whitespace for the purposes of vertical alignment. It's very fiddly and leads to unnecessary multi-line changes in the future.

Copy link
Contributor Author

@navytux navytux Jul 31, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

argh, ok. Even though personally I adhere to the oppsite, let's not argue on this and correct it here to be without vertical alignment. For the reference git diff -w and git gui blame help to go through this whitespace-only changes to the origin of where a line was originally modified.

cache.store(p64(1), n2, n3, b'1@2')
cache.store(p64(1), n3, None, b'1')
cache.store(p64(2), n2, n3, b'2@2')
cache.store(p64(2), n3, None, b'2')

eq = self.assertEqual
eq(len(cache), 5)
eq(cache.load(p64(1)), (b'1', n3))
eq(cache.loadBefore(p64(1), n3), (b'1@2', n2, n3))
eq(cache.loadBefore(p64(1), n2), (b'1@1', n1, n2))
eq(cache.loadBefore(p64(1), n1), None)
eq(cache.load(p64(2)), (b'2', n3))
eq(cache.loadBefore(p64(2), n3), (b'2@2', n2, n3))
eq(cache.loadBefore(p64(2), n2), None)

cache.clearAllNonCurrent()
eq(len(cache), 2)
eq(cache.load(p64(1)), (b'1', n3))
eq(cache.loadBefore(p64(1), n3), None)
eq(cache.loadBefore(p64(1), n2), None)
eq(cache.loadBefore(p64(1), n1), None)
eq(cache.load(p64(2)), (b'2', n3))
eq(cache.loadBefore(p64(2), n3), None)
eq(cache.loadBefore(p64(2), n2), None)

def testChangingCacheSize(self):
# start with a small cache
data = b'x'
Expand Down