Skip to content
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.

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

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

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

Conversation

navytux
Copy link
Contributor

@navytux navytux commented Jul 29, 2020

Else those non-current entries can be used to serve a loadBefore request
with data, while, after pack that loadBefore request must return "data
deleted" if requested object has current revision >= packtime.

Fixes checkPackVSConnectionGet from ZODB from zopefoundation/ZODB#322
which, without this patch fails as e.g.

Failure in test checkPackVSConnectionGet (ZEO.tests.testZEO.MappingStorageTests)
Traceback (most recent call last):
  File "/usr/lib/python2.7/unittest/case.py", line 329, in run
    testMethod()
  File "/home/kirr/src/wendelin/z/ZODB/src/ZODB/tests/PackableStorage.py", line 636, in checkPackVSConnectionGet
    raises(ReadConflictError, conn1.get, oid)
  File "/usr/lib/python2.7/unittest/case.py", line 473, in assertRaises
    callableObj(*args, **kwargs)
  File "/usr/lib/python2.7/unittest/case.py", line 116, in __exit__
    "{0} not raised".format(exc_name))
AssertionError: ReadConflictError not raised

Else those non-current entries can be used to serve a loadBefore request
with data, while, after pack that loadBefore request must return "data
deleted" if requested object has current revision >= packtime.

Fixes checkPackVSConnectionGet from ZODB from zopefoundation/ZODB#322
which, without this patch fails as e.g.

    Failure in test checkPackVSConnectionGet (ZEO.tests.testZEO.MappingStorageTests)
    Traceback (most recent call last):
      File "/usr/lib/python2.7/unittest/case.py", line 329, in run
        testMethod()
      File "/home/kirr/src/wendelin/z/ZODB/src/ZODB/tests/PackableStorage.py", line 636, in checkPackVSConnectionGet
        raises(ReadConflictError, conn1.get, oid)
      File "/usr/lib/python2.7/unittest/case.py", line 473, in assertRaises
        callableObj(*args, **kwargs)
      File "/usr/lib/python2.7/unittest/case.py", line 116, in __exit__
        "{0} not raised".format(exc_name))
    AssertionError: ReadConflictError not raised
Copy link
Member

@jamadden jamadden left a comment

Choose a reason for hiding this comment

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

The patch in isolation seems ok to me. (Remember, I am not a heavy ZEO user.)

I'm forced to wonder, though: this patch fixes the problem in a single ClientStorage, but what if the pack occurred in a different ClientStorage (such as in another process)? Wouldn't any other ClientStorage still have a cache with outdated noncurrent values?

If that's the case, then this could actually lead to more confusing errors in a cluster of clients, couldn't it? Only one particular process (the one that actually ran the pack) generates correct results, while others serve stale data.

src/ZEO/ClientStorage.py Outdated Show resolved Hide resolved
src/ZEO/cache.py Outdated Show resolved Hide resolved
@@ -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

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.

# 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.

Take review feedback by @jamadden into account

- ret -> result
- use () for tuples uniformly
- no vertical alignment
@navytux
Copy link
Contributor Author

navytux commented Jul 31, 2020

@jamadden, thanks for feedback and for catching the bug of not invalidating cache on other clients. I agree it indeed has to be fixed with dedicated test for this problem living in ZEO testsuite. Just could you please have a look at zopefoundation/ZODB#322 first, before I delve into this issue, with probably needing to extend ZEO protocol with something like "packed" message? It would be a pity to spend effort and time here if zopefoundation/ZODB#322 is not going to be generally accepted.

I've fixed up this PR with minor nits that you suggest.

For the referece: NEO will probably need to be similarly fixed (whole this change) to invalidate cache after database is packed /cc @jmuchemb, @vpelletier, @arnaud-fontaine.

@navytux
Copy link
Contributor Author

navytux commented Jul 31, 2020

( CI failure on pypy3 is likely unrelated - it is likely related to #155 )

@navytux
Copy link
Contributor Author

navytux commented Mar 31, 2021

Turning this into WIP - zopefoundation/ZODB#322 for which this patch is/was needed was also put on hold/wip to let loadAt (zopefoundation/ZODB#323) to go in first.

@navytux navytux changed the title pack: Clear all non-current entries after pack [WIP] pack: Clear all non-current entries after pack Mar 31, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants