HBTreeFolder2: remove manage_cleanup/_cleanup methods

It was an unefficient adaptation of BTreeFolder2 code.

These methods have probably never been used.

product/HBTreeFolder2/HBTreeFolder2.py

@@ -31,7 +31,7 @@ from OFS.Folder import Folder
 from AccessControl import getSecurityManager, ClassSecurityInfo
 from AccessControl.Permissions import access_contents_information, \
      view_management_screens
-from zLOG import LOG, INFO, ERROR, WARNING
+from zLOG import LOG, ERROR
 from AccessControl.SimpleObjectPolicies import ContainerAssertions
 
 
@@ -206,64 +206,6 @@ class HBTreeFolder2Base (Persistent):
             self._count.set(new)
         return old, new
 
-
-    security.declareProtected(view_management_screens, 'manage_cleanup')
-    def manage_cleanup(self):
-        """Calls self._cleanup() and reports the result as text.
-        """
-        v = self._cleanup()
-        path = '/'.join(self.getPhysicalPath())
-        if v:
-            return "No damage detected in HBTreeFolder2 at %s." % path
-        else:
-            return ("Fixed HBTreeFolder2 at %s.  "
-                    "See the log for more details." % path)
-
-
-    def _cleanup(self):
-        """Cleans up errors in the BTrees.
-
-        Certain ZODB bugs have caused BTrees to become slightly insane.
-        Fortunately, there is a way to clean up damaged BTrees that
-        always seems to work: make a new BTree containing the items()
-        of the old one.
-
-        Returns 1 if no damage was detected, or 0 if damage was
-        detected and fixed.
-        """
-        def hCheck(htree):
-          """
-              Recursively check the btree
-          """
-          check(htree)
-          for key in htree.keys():
-              if not htree.has_key(key):
-                  raise AssertionError(
-                      "Missing value for key: %s" % repr(key))
-              else:
-                ob = htree[key]
-                if isinstance(ob, OOBTree):
-                  hCheck(ob)
-          return 1
-
-        from BTrees.check import check
-        path = '/'.join(self.getPhysicalPath())
-        try:
-            return hCheck(self._htree)
-        except AssertionError:
-            LOG('HBTreeFolder2', WARNING,
-                'Detected damage to %s. Fixing now.' % path,
-                error=sys.exc_info())
-            try:
-                self._htree = OOBTree(self._htree) # XXX hFix needed
-            except Exception:
-                LOG('HBTreeFolder2', ERROR, 'Failed to fix %s.' % path,
-                    error=sys.exc_info())
-                raise
-            else:
-                LOG('HBTreeFolder2', INFO, 'Fixed %s.' % path)
-            return 0
-
     def hashId(self, id):
         return id.split(H_SEPARATOR)
 

product/HBTreeFolder2/tests/testHBTreeFolder2.py

@@ -196,23 +196,6 @@ class HBTreeFolder2Tests(ERP5TypeTestCase):
         expect = '<option value="%s">%s</option>' % (name, name)
         self.assert_(info['formatted_list'].find(expect) > 0)
 
-    def testCleanup(self):
-        self.assert_(self.f._cleanup())
-        key = TrojanKey('a')
-        self.f._htree[key] = 'b'
-        self.assert_(self.f._cleanup())
-        key.value = 'z'
-
-        # With a key in the wrong place, there should now be damage.
-        self.assert_(not self.f._cleanup())
-        # Now it's fixed.
-        self.assert_(self.f._cleanup())
-        # Verify the management interface also works,
-        # but don't test return values.
-        self.f.manage_cleanup()
-        key.value = 'a'
-        self.f.manage_cleanup()
-
     def testIterItems(self):
         h = HBTreeFolder2()
         id_list = ['a-b', 'a-cd',
@@ -351,21 +334,6 @@ class HBTreeFolder2Tests(ERP5TypeTestCase):
         self.assertTrue(t3_2 > t3_3)
 
 
-class TrojanKey:
-    """Pretends to be a consistent, immutable, humble citizen...
-
-    then sweeps the rug out from under the HBTree.
-    """
-    def __init__(self, value):
-        self.value = value
-
-    def __cmp__(self, other):
-        return cmp(self.value, other)
-
-    def __hash__(self):
-        return hash(self.value)
-
-
 def test_suite():
     return unittest.TestSuite((
         unittest.makeSuite(HBTreeFolder2Tests),

[Rafael Monnerat]

@jm After migrate a module to HBTreeFolder2, I started to get an Error:

Traceback (innermost last):

Module ZPublisher.WSGIPublisher, line 176, in transaction_pubevents
yield
Module ZPublisher.WSGIPublisher, line 386, in publish_module
response = _publish(request, new_mod_info)
Module Products.ERP5Type.patches.WSGIPublisher, line 150, in publish
return _original_publish(request, module_info)
Module ZPublisher.WSGIPublisher, line 288, in publish
bind=1)
Module ZPublisher.mapply, line 85, in mapply
return debug(object, args, context)
Module ZPublisher.WSGIPublisher, line 63, in call_object
return obj(*args)
Module Products.ERP5Type.Base, line 2714, in fixConsistency
return self.checkConsistency(fixit=True, filter=filter, **kw)
Module Products.ERP5Type.Core.Folder, line 1456, in checkConsistency
btree_ok = self._cleanup()
Module Products.ERP5Type.Core.Folder, line 959, in _cleanup
return folder._cleanup(self)
Module Products.ERP5Type.Core.Folder, line 1710, in __call__
raise NotImplementedError(self.__name__)
NotImplementedError: _cleanup

So I think, either we reintroduce (fixing whatever is unefficient) or we by pass (probably not a good idea).

/cc @jerome @romain @vpelletier

[Vincent Pelletier]

Here is my opinion, which will not help you nor comfort you.

HBTreeFolders are a fundamental mistake. They should not be used, and no new container should be migrated to them. The productive way forward is to understand why we measure a performance improvement when they are used, because there is fundamentally no difference stacking BTrees compared to having a single BTree: this is fundamentally what O(log(N)) means, which is how regular BTrees work. I suspect the test scenario has some properties which make this measured improvement appear, and these properties may not be realistic.

In addition to being meaningless on a design level, HBTreeFolders have flaws in the way they are implemented. These flaws cannot be fixed without breaking every existing HBTreeFolder. Folders which cannot be migrated back to standard BTreeFolder without very expensive data migrations (because they most likely contain a significant amount of data, which is why they were converted to begin with).

[Jérome Perrin]

The productive way forward is to understand why we measure a performance improvement when they are used

https://lab.nexedi.com/nexedi/slapos/-/blob/master/software/erp5/test/test/benchmarks.py and maybe scalability test can help. I think I tried a bit (actually I was interested in measuring the patch removing the mt_index but I did not finish all this) and one thing that is visible at the beginning is that unless using HBTreeFolder + _generatePerNodeId (like this is currently done for this test by ERP5Site_setIdGenerator), there are lots of ZODB conflicts because all zope nodes write in the same BTree bucket and when using this pattern each node write in its own btree because of the _generatePerNodeId, so there is no conflicts and HBTreeFolder2 looks better. The theory is that this is only at the beginning and once the modules have enough content this pattern is no longer observable, this is probably one reason why we thought HBTreeFolder2 was better.