This is an automated email from the ASF dual-hosted git repository. joscorbe pushed a commit to branch old-revisions-cleanup-rebase in repository https://gitbox.apache.org/repos/asf/jackrabbit-oak.git
commit 8732e6d0f4b9b52c9c5a9de684b956c93fe94713 Author: Jose Cordero <[email protected]> AuthorDate: Mon Nov 6 23:23:04 2023 +0100 Some test fixes and improvements. --- .../document/DocumentRevisionCleanupHelper.java | 40 ++++--- .../DocumentRevisionCleanupHelperTest.java | 126 +++++++++++++++------ 2 files changed, 117 insertions(+), 49 deletions(-) diff --git a/oak-store-document/src/main/java/org/apache/jackrabbit/oak/plugins/document/DocumentRevisionCleanupHelper.java b/oak-store-document/src/main/java/org/apache/jackrabbit/oak/plugins/document/DocumentRevisionCleanupHelper.java index 77c96ea465..efb5f7a31a 100644 --- a/oak-store-document/src/main/java/org/apache/jackrabbit/oak/plugins/document/DocumentRevisionCleanupHelper.java +++ b/oak-store-document/src/main/java/org/apache/jackrabbit/oak/plugins/document/DocumentRevisionCleanupHelper.java @@ -39,7 +39,8 @@ public class DocumentRevisionCleanupHelper { private final NodeDocument rootDoc; private final NodeDocument workingDocument; - private SortedMap<String, SortedMap<Integer, TreeSet<Revision>>> revisionsModifyingProperty; + private SortedMap<String, SortedMap<Integer, TreeSet<Revision>>> revisionsModifyingPropertyByCluster; + private SortedMap<String, TreeSet<Revision>> revisionsModifyingProperty; private SortedMap<Revision, TreeSet<String>> propertiesModifiedByRevision; private SortedMap<Integer, TreeSet<Revision>> blockedRevisionsToKeep; private SortedMap<Integer, TreeSet<Revision>> candidateRevisionsToClean; @@ -53,6 +54,7 @@ public class DocumentRevisionCleanupHelper { public DocumentRevisionCleanupHelper(DocumentStore documentStore, DocumentNodeStore documentNodeStore, String path) { this.candidateRevisionsToClean = new TreeMap<>(); this.blockedRevisionsToKeep = new TreeMap<>(); + this.revisionsModifyingPropertyByCluster = new TreeMap<>(); this.revisionsModifyingProperty = new TreeMap<>(); this.propertiesModifiedByRevision = new TreeMap<>(StableRevisionComparator.INSTANCE); @@ -87,6 +89,7 @@ public class DocumentRevisionCleanupHelper { protected void classifyAndMapRevisionsAndProperties() { candidateRevisionsToClean = new TreeMap<>(); blockedRevisionsToKeep = new TreeMap<>(); + revisionsModifyingPropertyByCluster = new TreeMap<>(); revisionsModifyingProperty = new TreeMap<>(); propertiesModifiedByRevision = new TreeMap<>(StableRevisionComparator.INSTANCE); @@ -137,10 +140,10 @@ public class DocumentRevisionCleanupHelper { /** * Step 3: * This method processes a set of revisions that modified certain properties and keeps the last revision that - * modified each property. This means, the current status of the node will be preserved. + * modified each property. This means, the current status of the node will be preserved, for each clusterId. */ protected void markLastRevisionForEachProperty() { - for (SortedMap<Integer, TreeSet<Revision>> revisionsByCluster : revisionsModifyingProperty.values()) { + for (SortedMap<Integer, TreeSet<Revision>> revisionsByCluster : revisionsModifyingPropertyByCluster.values()) { for (TreeSet<Revision> revisions : revisionsByCluster.values()) { Revision lastRevision = revisions.last(); addBlockedRevisionToKeep(lastRevision); @@ -157,19 +160,16 @@ public class DocumentRevisionCleanupHelper { SortedMap<Revision, Checkpoints.Info> checkpoints = documentNodeStore.getCheckpoints().getCheckpoints(); checkpoints.forEach((revision, info) -> { // For each checkpoint, keep the last revision that modified a property prior to checkpoint - revisionsModifyingProperty.forEach((propertyName, revisionsByCluster) -> { + revisionsModifyingProperty.forEach((propertyName, revisionsSet) -> { // Traverse the revisionVector of the checkpoint and find the last revision that modified the property info.getCheckpoint().forEach(revisionToFind -> { - TreeSet<Revision> listOfRevisionsForProperty = revisionsByCluster.get(revisionToFind.getClusterId()); - if (listOfRevisionsForProperty != null) { - // If the exact revision exists, keep it. If not, find the previous one that modified that property - if (listOfRevisionsForProperty.contains(revisionToFind)) { - addBlockedRevisionToKeep(revisionToFind); - } else { - Revision previousRevision = listOfRevisionsForProperty.descendingSet().ceiling(revisionToFind); - if (previousRevision != null) { - addBlockedRevisionToKeep(previousRevision); - } + // If the exact revision exists, keep it. If not, find the previous one that modified that property + if (revisionsSet.contains(revisionToFind)) { + addBlockedRevisionToKeep(revisionToFind); + } else { + Revision previousRevision = revisionsSet.descendingSet().ceiling(revisionToFind); + if (previousRevision != null) { + addBlockedRevisionToKeep(previousRevision); } } }); @@ -204,11 +204,15 @@ public class DocumentRevisionCleanupHelper { new TreeSet<>()).add(propertyEntry.getKey() ); - revisionsModifyingProperty.computeIfAbsent(propertyEntry.getKey(), key -> + revisionsModifyingPropertyByCluster.computeIfAbsent(propertyEntry.getKey(), key -> new TreeMap<>() ).computeIfAbsent(revision.getClusterId(), key -> new TreeSet<>(StableRevisionComparator.INSTANCE) ).add(revision); + + revisionsModifyingProperty.computeIfAbsent(propertyEntry.getKey(), key -> + new TreeSet<>(StableRevisionComparator.INSTANCE) + ).add(revision); } } } @@ -261,7 +265,11 @@ public class DocumentRevisionCleanupHelper { return (NavigableMap<Revision, String>) workingDocument.get("_revisions"); } - public SortedMap<String, SortedMap<Integer, TreeSet<Revision>>> getRevisionsModifyingProperty() { + public SortedMap<String, SortedMap<Integer, TreeSet<Revision>>> getRevisionsModifyingPropertyByCluster() { + return revisionsModifyingPropertyByCluster; + } + + public SortedMap<String, TreeSet<Revision>> getRevisionsModifyingProperty() { return revisionsModifyingProperty; } diff --git a/oak-store-document/src/test/java/org/apache/jackrabbit/oak/plugins/document/DocumentRevisionCleanupHelperTest.java b/oak-store-document/src/test/java/org/apache/jackrabbit/oak/plugins/document/DocumentRevisionCleanupHelperTest.java index add00b08f5..b56df5ae2c 100644 --- a/oak-store-document/src/test/java/org/apache/jackrabbit/oak/plugins/document/DocumentRevisionCleanupHelperTest.java +++ b/oak-store-document/src/test/java/org/apache/jackrabbit/oak/plugins/document/DocumentRevisionCleanupHelperTest.java @@ -152,38 +152,38 @@ public class DocumentRevisionCleanupHelperTest { // Some initial revisions (0-2) revs.add(Revision.fromString("r100000000-0-1")); - revs.add(Revision.fromString("r100000000-0-2")); - revs.add(Revision.fromString("r100000000-0-3")); + revs.add(Revision.fromString("r100000001-0-2")); + revs.add(Revision.fromString("r100000002-0-3")); // Blocked by first checkpoint r109000000 (3-5) - revs.add(Revision.fromString("r106000000-0-1")); - revs.add(Revision.fromString("r105000000-0-2")); - revs.add(Revision.fromString("r108000000-0-3")); + revs.add(Revision.fromString("r106000003-0-1")); + revs.add(Revision.fromString("r107000004-0-2")); + revs.add(Revision.fromString("r108000005-0-3")); // Some unblocked revisions in middle (6-10) - revs.add(Revision.fromString("r110000000-0-3")); - revs.add(Revision.fromString("r110000000-1-3")); - revs.add(Revision.fromString("r114000000-0-2")); - revs.add(Revision.fromString("r115000000-0-1")); - revs.add(Revision.fromString("r118000000-0-1")); + revs.add(Revision.fromString("r110000006-0-3")); + revs.add(Revision.fromString("r110000006-1-3")); + revs.add(Revision.fromString("r114000007-0-2")); + revs.add(Revision.fromString("r115000008-0-1")); + revs.add(Revision.fromString("r118000009-0-1")); // Blocked by second checkpoint r118000000 (11-13) - revs.add(Revision.fromString("r118000000-1-1")); - revs.add(Revision.fromString("r118000000-0-2")); - revs.add(Revision.fromString("r118000000-0-3")); + revs.add(Revision.fromString("r118000009-1-1")); + revs.add(Revision.fromString("r118000010-0-2")); + revs.add(Revision.fromString("r118000011-0-3")); // Some more unblocked revisions (14-19) - revs.add(Revision.fromString("r120000000-0-1")); - revs.add(Revision.fromString("r122000000-0-2")); - revs.add(Revision.fromString("r122000000-1-2")); - revs.add(Revision.fromString("r123000000-0-2")); - revs.add(Revision.fromString("r125000000-0-3")); - revs.add(Revision.fromString("r130000000-0-1")); + revs.add(Revision.fromString("r120000012-0-1")); + revs.add(Revision.fromString("r122000013-0-2")); + revs.add(Revision.fromString("r122000013-1-2")); + revs.add(Revision.fromString("r123000014-0-2")); + revs.add(Revision.fromString("r125000015-0-3")); + revs.add(Revision.fromString("r130000016-0-1")); // Last revision (20-22) - revs.add(Revision.fromString("r130000000-1-1")); - revs.add(Revision.fromString("r130000000-0-2")); - revs.add(Revision.fromString("r130000000-0-3")); + revs.add(Revision.fromString("r130000016-1-1")); + revs.add(Revision.fromString("r130000017-0-2")); + revs.add(Revision.fromString("r130000018-0-3")); // Checkpoint revisions Revision checkpoint1 = Revision.fromString("r109000000-0-1"); @@ -207,20 +207,19 @@ public class DocumentRevisionCleanupHelperTest { jsonBuilder.append(jsonPropBuilder).append(", ").append(jsonRevisionsBuilder).append("}"); String jsonCheckpoints = "{" + - "'" + checkpoint1 + "': {'expires':'200000000','rv':'r109000000-0-1,r109000000-0-2,r109000000-0-3'}," + - "'" + checkpoint2 + "': {'expires':'200000000','rv':'r118000000-1-1,r118000000-0-2,r118000000-0-3'}" + + "'" + checkpoint1 + "': {'expires':'200000000','rv':'r106500003-0-1,r107500004-0-2,r108500005-0-3'}," + + "'" + checkpoint2 + "': {'expires':'200000000','rv':'r118000009-1-1,r118000010-0-2,r118000011-0-3'}" + "}"; - prepareDocumentMock(jsonBuilder.toString()); prepareCheckpointsMock(jsonCheckpoints); documentRevisionCleanupHelper.initializeCleanupProcess(); // The revisions blocked should be: - // - r106000000-0-1, r118000000-0-2, r108000000-0-3 (referenced by checkpoint 1) - // - r118000000-1-1, r118000000-0-2, r118000000-0-3 (referenced by checkpoint 2) - // - r130000000-1-1, r130000000-0-2, r130000000-0-3 (last revision) + // - r106000003-0-3, r118000004-0-2, r108000005-0-3 (referenced by checkpoint 1) + // - r118000009-1-1, r118000010-0-2, r118000011-0-3 (referenced by checkpoint 2) + // - r130000016-1-1, r130000017-0-2, r130000018-0-3 (last revision) assertEquals(Set.of(revs.get(3), revs.get(11), revs.get(20)), documentRevisionCleanupHelper.getBlockedRevisionsToKeep().get(1)); assertEquals(Set.of(revs.get(4), revs.get(12), revs.get(21)), documentRevisionCleanupHelper.getBlockedRevisionsToKeep().get(2)); assertEquals(Set.of(revs.get(5), revs.get(13), revs.get(22)), documentRevisionCleanupHelper.getBlockedRevisionsToKeep().get(3)); @@ -235,6 +234,67 @@ public class DocumentRevisionCleanupHelperTest { assertTrue(Collections.disjoint(documentRevisionCleanupHelper.getBlockedRevisionsToKeep().get(3), documentRevisionCleanupHelper.getCandidateRevisionsToClean().get(3))); } + @Test + public void testCheckpointedRevisionFallback() throws IOException { + List<Revision> revs = new ArrayList<>(); + + revs.add(Revision.fromString("r100000000-0-1")); + revs.add(Revision.fromString("r100000001-0-2")); + revs.add(Revision.fromString("r100000002-0-3")); + + revs.add(Revision.fromString("r101100003-0-1")); + revs.add(Revision.fromString("r101200004-0-2")); + revs.add(Revision.fromString("r101300005-0-3")); + + revs.add(Revision.fromString("r102000006-0-1")); + revs.add(Revision.fromString("r102000007-0-3")); + + revs.add(Revision.fromString("r103000008-0-1")); + revs.add(Revision.fromString("r103000009-0-2")); + revs.add(Revision.fromString("r103000010-0-3")); + + // Checkpoint + Revision checkpoint1 = Revision.fromString("r102000000-0-1"); + + StringBuilder jsonPropBuilder = new StringBuilder("'prop1': {"); + StringBuilder jsonRevisionsBuilder = new StringBuilder("'_revisions': {"); + + for (int i = 0; i < revs.size(); i++) { + jsonPropBuilder.append("'").append(revs.get(i)).append("': 'value").append(i).append("'"); + jsonRevisionsBuilder.append("'").append(revs.get(i)).append("': 'c'"); + if (i < revs.size() - 1) { + jsonPropBuilder.append(", "); + jsonRevisionsBuilder.append(", "); + } + } + + jsonPropBuilder.append("}"); + jsonRevisionsBuilder.append("}"); + StringBuilder jsonBuilder = new StringBuilder("{"); + jsonBuilder.append(jsonPropBuilder).append(", ").append(jsonRevisionsBuilder).append("}"); + + String jsonCheckpoints = "{" + + "'" + checkpoint1 + "': {'expires':'200000000','rv':'r102000007-0-1,r102000000-0-2,r102000008-0-3'}" + + "}"; + + prepareDocumentMock(jsonBuilder.toString()); + prepareCheckpointsMock(jsonCheckpoints); + + documentRevisionCleanupHelper.initializeCleanupProcess(); + + // The revisions blocked should be: + // - r103000008-0-1, r103000009-0-2, r103000010-0-3 (last revisions) + // - r102000006-0-1, r101300005-0-3, r102000007-0-3 (referenced by checkpoint for clusters 1, 2 and 3 respectively) + assertEquals(Set.of(revs.get(6), revs.get(8)), documentRevisionCleanupHelper.getBlockedRevisionsToKeep().get(1)); + assertEquals(Set.of(revs.get(9)), documentRevisionCleanupHelper.getBlockedRevisionsToKeep().get(2)); + assertEquals(Set.of(revs.get(5), revs.get(7), revs.get(10)), documentRevisionCleanupHelper.getBlockedRevisionsToKeep().get(3)); + + // Rest of revisions are candidates to clean + assertEquals(Set.of(revs.get(0), revs.get(3)), documentRevisionCleanupHelper.getCandidateRevisionsToClean().get(1)); + assertEquals(Set.of(revs.get(1), revs.get(4)), documentRevisionCleanupHelper.getCandidateRevisionsToClean().get(2)); + assertEquals(Set.of(revs.get(2)), documentRevisionCleanupHelper.getCandidateRevisionsToClean().get(3)); + } + @Test public void testLastRevisionIsBlocked() throws IOException { Revision revisionA = new Revision(111111111L, 0, 1); @@ -295,7 +355,7 @@ public class DocumentRevisionCleanupHelperTest { assertEquals(Set.of("prop1", "prop2"), propertiesModifiedByRevision.get(revisionB)); assertEquals(Set.of("prop1"), propertiesModifiedByRevision.get(revisionC)); - SortedMap<String, SortedMap<Integer, TreeSet<Revision>>> revisionsModifyingProperty = documentRevisionCleanupHelper.getRevisionsModifyingProperty(); + SortedMap<String, SortedMap<Integer, TreeSet<Revision>>> revisionsModifyingProperty = documentRevisionCleanupHelper.getRevisionsModifyingPropertyByCluster(); assertEquals(Set.of(revisionA, revisionB, revisionC), revisionsModifyingProperty.get("prop1").get(1)); assertEquals(Set.of(revisionB), revisionsModifyingProperty.get("prop2").get(1)); assertEquals(Set.of(revisionA), revisionsModifyingProperty.get("_deleted").get(1)); @@ -320,7 +380,7 @@ public class DocumentRevisionCleanupHelperTest { assertEquals(Set.of("_deleted"), propertiesModifiedByRevision.get(revisionB)); assertEquals(Set.of("_deleted"), propertiesModifiedByRevision.get(revisionC)); - SortedMap<String, SortedMap<Integer, TreeSet<Revision>>> revisionsModifyingProperty = documentRevisionCleanupHelper.getRevisionsModifyingProperty(); + SortedMap<String, SortedMap<Integer, TreeSet<Revision>>> revisionsModifyingProperty = documentRevisionCleanupHelper.getRevisionsModifyingPropertyByCluster(); assertEquals(Set.of(revisionA, revisionB, revisionC), revisionsModifyingProperty.get("_deleted").get(1)); } @@ -344,7 +404,7 @@ public class DocumentRevisionCleanupHelperTest { assertEquals(Set.of("prop1", "prop2"), propertiesModifiedByRevision.get(revisionB)); assertEquals(Set.of("prop1"), propertiesModifiedByRevision.get(revisionC)); - SortedMap<String, SortedMap<Integer, TreeSet<Revision>>> revisionsModifyingProperty = documentRevisionCleanupHelper.getRevisionsModifyingProperty(); + SortedMap<String, SortedMap<Integer, TreeSet<Revision>>> revisionsModifyingProperty = documentRevisionCleanupHelper.getRevisionsModifyingPropertyByCluster(); assertEquals(Set.of(revisionA, revisionB, revisionC), revisionsModifyingProperty.get("prop1").get(1)); assertEquals(Set.of(revisionB), revisionsModifyingProperty.get("prop2").get(1)); } @@ -372,7 +432,7 @@ public class DocumentRevisionCleanupHelperTest { assertEquals(Set.of("prop1"), propertiesModifiedByRevision.get(revisionC)); assertNull(propertiesModifiedByRevision.get(revisionD)); - SortedMap<String, SortedMap<Integer, TreeSet<Revision>>> revisionsModifyingProperty = documentRevisionCleanupHelper.getRevisionsModifyingProperty(); + SortedMap<String, SortedMap<Integer, TreeSet<Revision>>> revisionsModifyingProperty = documentRevisionCleanupHelper.getRevisionsModifyingPropertyByCluster(); assertEquals(Set.of(revisionA, revisionB, revisionC), revisionsModifyingProperty.get("prop1").get(1)); assertEquals(Set.of(revisionB), revisionsModifyingProperty.get("prop2").get(1)); } @@ -399,7 +459,7 @@ public class DocumentRevisionCleanupHelperTest { assertEquals(Set.of("prop1"), propertiesModifiedByRevision.get(revisionC)); assertEquals(Set.of("prop1", "prop2"), propertiesModifiedByRevision.get(revisionD)); - SortedMap<String, SortedMap<Integer, TreeSet<Revision>>> revisionsModifyingProperty = documentRevisionCleanupHelper.getRevisionsModifyingProperty(); + SortedMap<String, SortedMap<Integer, TreeSet<Revision>>> revisionsModifyingProperty = documentRevisionCleanupHelper.getRevisionsModifyingPropertyByCluster(); assertEquals(Set.of(revisionA, revisionD), revisionsModifyingProperty.get("prop1").get(1)); assertEquals(Set.of(revisionB), revisionsModifyingProperty.get("prop1").get(2)); assertEquals(Set.of(revisionC), revisionsModifyingProperty.get("prop1").get(3));
