This is an automated email from the ASF dual-hosted git repository. daim pushed a commit to branch DetailedGC/OAK-10199 in repository https://gitbox.apache.org/repos/asf/jackrabbit-oak.git
commit 5387b42c7e195bbd80b08b5f6dd82dbbd3bede07 Author: Rishabh Kumar <[email protected]> AuthorDate: Mon Sep 11 14:24:54 2023 +0530 OAK-8646 : added metrics for unmergedbranch commits & improved junit coverage --- .gitignore | 1 + .../document/DetailedRevisionGCStatsCollector.java | 31 +- .../DetailedRevisionGCStatsCollectorImpl.java | 10 +- .../plugins/document/VersionGarbageCollector.java | 192 +++++++------ .../oak/plugins/document/BranchCommitGCTest.java | 315 +++++++++++---------- .../oak/plugins/document/DetailGCHelper.java | 61 ++++ .../DetailedRevisionGCStatsCollectorImplTest.java | 12 +- .../document/VersionGarbageCollectorIT.java | 145 ++++++++-- 8 files changed, 490 insertions(+), 277 deletions(-) diff --git a/.gitignore b/.gitignore index d13284858c..60e0127c72 100644 --- a/.gitignore +++ b/.gitignore @@ -12,3 +12,4 @@ atlassian-ide-plugin.xml derby.log .java-version oak-shaded-guava/dependency-reduced-pom.xml +.DS_Store diff --git a/oak-store-document/src/main/java/org/apache/jackrabbit/oak/plugins/document/DetailedRevisionGCStatsCollector.java b/oak-store-document/src/main/java/org/apache/jackrabbit/oak/plugins/document/DetailedRevisionGCStatsCollector.java index 151195f3bd..0a965f44be 100644 --- a/oak-store-document/src/main/java/org/apache/jackrabbit/oak/plugins/document/DetailedRevisionGCStatsCollector.java +++ b/oak-store-document/src/main/java/org/apache/jackrabbit/oak/plugins/document/DetailedRevisionGCStatsCollector.java @@ -26,15 +26,44 @@ import org.apache.jackrabbit.oak.plugins.document.VersionGarbageCollector.Versio */ public interface DetailedRevisionGCStatsCollector { + /** + * Total No. of documents read during DetailedGC phase + */ void documentRead(); + /** + * No. of properties deleted during DetailedGC + * @param numProps no. of properties deleted in current cycle + */ void propertiesDeleted(long numProps); + /** + * No. of unmerged (unique) branch commits deleted during DetailedGC + * @param numCommits no. of unmerged branch commits deleted in current cycle + */ + void unmergedBranchCommitsDeleted(long numCommits); + + /** + * No. of documents updated (i.e. have garbage removed) during DetailedGC + * @param numDocs no. of documents updated in current cycle + */ void documentsUpdated(long numDocs); - void documentsSkippedUpdation(long numDocs); + /** + * No. of documents which had skipped update (i.e. have been updated between garbage collection & removal) + * during DetailedGC + * @param numDocs No. of documents which had skipped update in current cycle + */ + void documentsUpdateSkipped(long numDocs); + /** + * No. of times the DetailedGC has started + */ void started(); + /** + * Timer for different phases in DetailedGC + * @param stats {@link VersionGCStats} containing DetailedGC phases timer + */ void finished(VersionGCStats stats); } diff --git a/oak-store-document/src/main/java/org/apache/jackrabbit/oak/plugins/document/DetailedRevisionGCStatsCollectorImpl.java b/oak-store-document/src/main/java/org/apache/jackrabbit/oak/plugins/document/DetailedRevisionGCStatsCollectorImpl.java index b400f7401b..01f196831f 100644 --- a/oak-store-document/src/main/java/org/apache/jackrabbit/oak/plugins/document/DetailedRevisionGCStatsCollectorImpl.java +++ b/oak-store-document/src/main/java/org/apache/jackrabbit/oak/plugins/document/DetailedRevisionGCStatsCollectorImpl.java @@ -36,6 +36,7 @@ class DetailedRevisionGCStatsCollectorImpl implements DetailedRevisionGCStatsCol static final String DETAILED_GC = "DetailedGC"; static final String READ_DOC = "READ_DOC"; static final String DELETED_PROPERTY = "DELETED_PROPERTY"; + static final String DELETED_UNMERGED_BC = "DELETED_UNMERGED_BC"; static final String UPDATED_DOC = "UPDATED_DOC"; static final String SKIPPED_DOC = "SKIPPED_DOC"; static final String DETAILED_GC_ACTIVE_TIMER = "DETAILED_GC_ACTIVE_TIMER"; @@ -51,6 +52,7 @@ class DetailedRevisionGCStatsCollectorImpl implements DetailedRevisionGCStatsCol private final MeterStats readDoc; private final MeterStats deletedProperty; + private final MeterStats deletedUnmergedBC; private final MeterStats updatedDoc; private final MeterStats skippedDoc; private final TimerStats detailedGCActiveTimer; @@ -68,6 +70,7 @@ class DetailedRevisionGCStatsCollectorImpl implements DetailedRevisionGCStatsCol readDoc = meter(provider, READ_DOC); deletedProperty = meter(provider, DELETED_PROPERTY); + deletedUnmergedBC = meter(provider, DELETED_UNMERGED_BC); updatedDoc = meter(provider, UPDATED_DOC); skippedDoc = meter(provider, SKIPPED_DOC); @@ -95,13 +98,18 @@ class DetailedRevisionGCStatsCollectorImpl implements DetailedRevisionGCStatsCol deletedProperty.mark(numProps); } + @Override + public void unmergedBranchCommitsDeleted(long numCommits) { + deletedUnmergedBC.mark(numCommits); + } + @Override public void documentsUpdated(long numDocs) { updatedDoc.mark(numDocs); } @Override - public void documentsSkippedUpdation(long numDocs) { + public void documentsUpdateSkipped(long numDocs) { skippedDoc.mark(numDocs); } diff --git a/oak-store-document/src/main/java/org/apache/jackrabbit/oak/plugins/document/VersionGarbageCollector.java b/oak-store-document/src/main/java/org/apache/jackrabbit/oak/plugins/document/VersionGarbageCollector.java index 7cd1604ec8..9e69241a68 100644 --- a/oak-store-document/src/main/java/org/apache/jackrabbit/oak/plugins/document/VersionGarbageCollector.java +++ b/oak-store-document/src/main/java/org/apache/jackrabbit/oak/plugins/document/VersionGarbageCollector.java @@ -26,12 +26,12 @@ import java.util.ArrayList; import java.util.Collections; import java.util.EnumSet; import java.util.HashMap; +import java.util.HashSet; import java.util.Iterator; import java.util.List; import java.util.Map; import java.util.Objects; import java.util.Set; -import java.util.Map.Entry; import java.util.concurrent.TimeUnit; import java.util.concurrent.atomic.AtomicBoolean; import java.util.concurrent.atomic.AtomicReference; @@ -301,7 +301,8 @@ public class VersionGarbageCollector { long oldestModifiedDocTimeStamp; String oldestModifiedDocId; int updatedDetailedGCDocsCount; - int deletedPropsGCCount; + int deletedPropsCount; + int deletedUnmergedBCCount; final TimeDurationFormatter df = TimeDurationFormatter.forLogging(); final Stopwatch active = Stopwatch.createUnstarted(); final Stopwatch detailedGCActive = Stopwatch.createUnstarted(); @@ -378,7 +379,8 @@ public class VersionGarbageCollector { ", oldestModifiedDocId=" + oldestModifiedDocId + ", oldestModifiedDocTimeStamp=" + oldestModifiedDocTimeStamp + ", updatedDetailedGCDocsCount=" + updatedDetailedGCDocsCount + - ", deletedPropsGCCount=" + deletedPropsGCCount + + ", deletedPropsCount=" + deletedPropsCount + + ", deletedUnmergedBCCount=" + deletedUnmergedBCCount + ", iterationCount=" + iterationCount + ", timeDetailedGCActive=" + df.format(detailedGCActiveElapsed, MICROSECONDS) + ", timeActive=" + df.format(activeElapsed, MICROSECONDS) + @@ -401,7 +403,8 @@ public class VersionGarbageCollector { this.oldestModifiedDocTimeStamp = run.oldestModifiedDocTimeStamp; this.oldestModifiedDocId = run.oldestModifiedDocId; this.updatedDetailedGCDocsCount += run.updatedDetailedGCDocsCount; - this.deletedPropsGCCount += run.deletedPropsGCCount; + this.deletedPropsCount += run.deletedPropsCount; + this.deletedUnmergedBCCount += run.deletedUnmergedBCCount; if (run.iterationCount > 0) { // run is cumulative with times in elapsed fields this.activeElapsed += run.activeElapsed; @@ -860,6 +863,12 @@ public class VersionGarbageCollector { * In order to calculate the correct no. of updated documents & deleted properties, we save them in a map */ private final Map<String, Integer> deletedPropsCountMap; + + /** + * {@link Set} of unmergedBranchCommit Revisions to calculate the no. of unmergedBranchCommits that would be + * removed in this iteration of DetailedGC. + */ + private final Set<Revision> deletedUnmergedBCSet; private int garbageDocsCount; private int totalGarbageDocsCount; private final Revision revisionForModified; @@ -871,6 +880,7 @@ public class VersionGarbageCollector { this.cancel = cancel; this.updateOpList = new ArrayList<>(); this.deletedPropsCountMap = new HashMap<>(); + this.deletedUnmergedBCSet = new HashSet<>(); this.timer = createUnstarted(); // clusterId is not used this.revisionForModified = Revision.newRevision(0); @@ -885,7 +895,7 @@ public class VersionGarbageCollector { op.equals(MODIFIED_IN_SECS, doc.getModified()); collectDeletedProperties(doc, phases, op); - collectUnmergedBranchCommitDocument(doc, toModifiedMs, phases, op); + collectUnmergedBranchCommits(doc, phases, op, toModifiedMs); collectOldRevisions(doc, phases, op); // only add if there are changes for this doc if (op.hasChanges()) { @@ -894,6 +904,9 @@ public class VersionGarbageCollector { monitor.info("Collected [{}] garbage for doc [{}]", op.getChanges().size(), doc.getId()); updateOpList.add(op); } + if (log.isDebugEnabled()) { + log.debug("UpdateOp for {} is {}", doc.getId(), op); + } } private boolean hasGarbage() { @@ -916,118 +929,116 @@ public class VersionGarbageCollector { .map(p -> p.stream().map(Utils::escapePropertyName).collect(toSet())) .orElse(emptySet()); - final int deletedPropsGCCount = properties.stream() + final int deletedPropsCount = properties.stream() .filter(p -> !retainPropSet.contains(p)) .mapToInt(x -> { updateOp.remove(x); return 1;}) .sum(); - deletedPropsCountMap.put(doc.getId(), deletedPropsGCCount); + deletedPropsCountMap.put(doc.getId(), deletedPropsCount); if (log.isDebugEnabled()) { - log.debug("Collected {} deleted properties for document {}", deletedPropsGCCount, doc.getId()); + log.debug("Collected {} deleted properties for document {}", deletedPropsCount, doc.getId()); } phases.stop(GCPhase.DETAILED_GC_COLLECT_PROPS); } } - private void collectUnmergedBranchCommitDocument(final NodeDocument doc, - long toModifiedMillis, final GCPhases phases, final UpdateOp updateOp) { + private void collectUnmergedBranchCommits(final NodeDocument doc, final GCPhases phases, final UpdateOp updateOp, + final long toModifiedMs) { if (!phases.start(GCPhase.DETAILED_GC_COLLECT_UNMERGED_BC)) { // GC was cancelled, stop return; } - // from - // https://jackrabbit.apache.org/oak/docs/nodestore/documentmk.html#previous-documents - // "branch commits are not moved to previous documents until the branch is - // merged." + // from https://jackrabbit.apache.org/oak/docs/nodestore/documentmk.html#previous-documents + // "branch commits are not moved to previous documents until the branch is merged." // i.e. if we're looking for unmerged branch commits, they cannot be in // previous documents, they have to be in the main one - hence we have to use // getLocalBranchCommits here - final Set<Revision> localBranchCommits = doc.getLocalBranchCommits(); - if (localBranchCommits.isEmpty()) { + final Set<Revision> olderUnmergedBranchCommits = doc.getLocalBranchCommits().stream() + .filter(bcRevision -> isRevisionOlderThan(bcRevision, toModifiedMs)) + .filter(bcRevision -> !isCommitted(nodeStore.getCommitValue(bcRevision, doc))) + .collect(toSet()); + + if (olderUnmergedBranchCommits.isEmpty()) { // nothing to do then phases.stop(GCPhase.DETAILED_GC_COLLECT_UNMERGED_BC); return; } - // !Note, the _bc sub-document was introduced with Oak 1.8 and is not present - // in older versions. The branch commit revision is added to _bc whenever a - // change is done on a document with a branch commit. This helps the - // DocumentNodeStore to more easily identify branch commit changes." + // !Note, the _bc sub-document was introduced with Oak 1.8 and is not present in older versions. + // The branch commit revision is added to _bc whenever a change is done on a document with a + // branch commit. This helps the DocumentNodeStore to more easily identify branch commit changes." // The current implementation of "collectUnmergedBranchCommitDocument" only // supports branch commits that are created after Oak 1.8 - for (Revision bcRevision : localBranchCommits) { - if (!isRevisionOlderThan(bcRevision, toModifiedMillis)) { - // only even consider revisions that are older than the provided - // timestamp - otherwise skip this - continue; - } - final String commitValue = nodeStore.getCommitValue(bcRevision, doc); - if (isCommitted(commitValue)) { - // obviously don't do anything with merged (committed) branch commits - continue; - } - removeUnmergedBCRevision(bcRevision, doc, updateOp); + + olderUnmergedBranchCommits.forEach(bcRevision -> removeUnmergedBCRevision(bcRevision, doc, updateOp)); + deletedUnmergedBCSet.addAll(olderUnmergedBranchCommits); + + if (log.isDebugEnabled()) { + log.debug("Collected {} unmerged branch commits for document {}", olderUnmergedBranchCommits.size(), doc.getId()); } + // now for any of the handled system properties (the normal properties would // already be cleaned up by cleanupDeletedProperties), the resulting - // subdocument could in theory become empty after removing all unmerged branch + // sub document could in theory become empty after removing all unmerged branch // commit revisions is done later. // so we need to go through all of them and check if we'd have removed // the entirety - and then, instead of individually remove revisions, just // delete the entire property. if (updateOp.hasChanges()) { - for (Entry<String, Integer> e : getSystemRemoveMapEntryCounts(updateOp) - .entrySet()) { - final String prop = e.getKey(); - final Object d = doc.data.get(prop); - if (!(d instanceof Map)) { - // unexpected and would likely indicate a bug, hence log.error - log.error( - "collectUnmergedBranchCommitDocument: property without subdocument as expected. id={}, prop={}", - doc.getId(), prop); - continue; - } - @SuppressWarnings("rawtypes") - final Map m = (Map) d; - if (m.size() != e.getValue()) { - // then we're not removing all revisions - so cannot cleanup - continue; - } - // then we're removing all revisions - so replace those REMOVE_MAP_ENTRY - // with one whole remove(prop) - final Iterator<Entry<Key, Operation>> it = updateOp.getChanges().entrySet() - .iterator(); - while (it.hasNext()) { - if (it.next().getKey().getName().equals(prop)) { - it.remove(); - } - } - updateOp.remove(prop); - } + final int deletedSystemPropsCount = getSystemRemoveMapEntryCounts(updateOp) + .entrySet().stream() + .filter(e -> filterEmptyProps(doc, e.getKey(), e.getValue())) + .mapToInt(e -> { + final String prop = e.getKey(); + updateOp.getChanges().entrySet().removeIf(opEntry -> Objects.equals(prop, opEntry.getKey().getName())); + updateOp.remove(prop); + return 1;}) + .sum(); + + // update the deleted properties count Map to calculate the total no. of deleted properties + deletedPropsCountMap.merge(doc.getId(), deletedSystemPropsCount, Integer::sum); } phases.stop(GCPhase.DETAILED_GC_COLLECT_UNMERGED_BC); } + /** + * Filter all would be empty system properties (after cleanup operation). + * <p> + * It verifies this by comparing the size of sub-document with given <code>value</code> + * + * @param doc {@link NodeDocument} on whose properties needs to be checked + * @param prop Name of sub-document which needs to checked whether it would be empty after cleanup or not + * @param value expected no. of entries + * @return true if sub-document would eventually be empty or not + */ + private boolean filterEmptyProps(final NodeDocument doc, final String prop, final int value) { + final Object d = doc.data.get(prop); + if (d instanceof Map) { + @SuppressWarnings("rawtypes") final Map m = (Map) d; + // then we're not removing all revisions - so cannot clean up + return m.size() == value; + } else { + // unexpected and would likely indicate a bug, hence log.error + log.error("collectUnmergedBranchCommitDocument: property without sub-document as expected. " + + "id={}, prop={}", doc.getId(), prop); + return false; + } + } + /** small helper to count number of REMOVE_MAP_ENTRY per system property */ private Map<String, Integer> getSystemRemoveMapEntryCounts(final UpdateOp updateOp) { final Map<String, Integer> propMap = new HashMap<>(); - for (Entry<Key, Operation> e : updateOp.getChanges().entrySet()) { - if (e.getValue().type != Type.REMOVE_MAP_ENTRY) { - // only count REMOVE_MAP_ENTRY types, skip the rest - continue; - } - final String propName = e.getKey().getName(); - if (!propName.startsWith("_")) { - // only count system properties, skip the rest - continue; - } - Integer count = propMap.getOrDefault(propName, 0); - propMap.put(propName, count + 1); - } + + updateOp.getChanges().entrySet().stream() + .filter(e -> e.getValue().type == Type.REMOVE_MAP_ENTRY) + .map(e -> e.getKey().getName()) + .filter(propName -> propName.startsWith("_")) + .forEach(propName -> propMap.merge(propName, 1, Integer::sum)); + return propMap; } @@ -1070,10 +1081,9 @@ public class VersionGarbageCollector { * @param doc the document from which the uncommittedBCRevision * should be removed * @param updateOp the resulting operations yet to be applied - * @param propMap */ - private void removeUnmergedBCRevision(Revision unmergedBCRevision, - NodeDocument doc, UpdateOp updateOp) { + private void removeUnmergedBCRevision(final Revision unmergedBCRevision, final NodeDocument doc, + final UpdateOp updateOp) { // caller ensures the provided revision is an unmerged branch commit NodeDocument.removeBranchCommit(updateOp, unmergedBCRevision); @@ -1091,7 +1101,7 @@ public class VersionGarbageCollector { // commit, but that was never merged. when we now remove that, it could be // that it is then deleted. - // to know whether or not the node is actually deleted, would potentially + // to know whether the node is actually deleted, would potentially // require several commit value lookups. // in order to keep the execution time of detailGC in this regard small, // the code here stops with any further checks and just sets @@ -1112,8 +1122,7 @@ public class VersionGarbageCollector { if (doc.getLocalRevisions().containsKey(unmergedBCRevision)) { NodeDocument.removeRevision(updateOp, unmergedBCRevision); } - if (doc.getLocalMap(NodeDocument.COLLISIONS) - .containsKey(unmergedBCRevision)) { + if (doc.getLocalMap(NodeDocument.COLLISIONS).containsKey(unmergedBCRevision)) { NodeDocument.removeCollision(updateOp, unmergedBCRevision); } // phase 4 : go through normal properties @@ -1132,7 +1141,7 @@ public class VersionGarbageCollector { } } - private void collectOldRevisions(NodeDocument doc, GCPhases phases, UpdateOp updateOp) { + private void collectOldRevisions(final NodeDocument doc, final GCPhases phases, final UpdateOp updateOp) { if (phases.start(GCPhase.DETAILED_GC_COLLECT_OLD_REVS)){ // TODO add old rev collection logic @@ -1179,18 +1188,25 @@ public class VersionGarbageCollector { int deletedProps = oldDocs.stream().filter(Objects::nonNull).mapToInt(d -> deletedPropsCountMap.getOrDefault(d.getId(), 0)).sum(); updatedDocs = (int) oldDocs.stream().filter(Objects::nonNull).count(); stats.updatedDetailedGCDocsCount += updatedDocs; - stats.deletedPropsGCCount += deletedProps; - log.debug("Updated [{}] documents, deleted [{}] properties", updatedDocs, deletedProps); - // now reset delete metadata - updateOpList.clear(); - deletedPropsCountMap.clear(); - garbageDocsCount = 0; + stats.deletedPropsCount += deletedProps; + stats.deletedUnmergedBCCount += deletedUnmergedBCSet.size(); + + if (log.isDebugEnabled()) { + log.debug("Updated [{}] documents, deleted [{}] properties, deleted [{}] unmergedBranchCommits", + updatedDocs, deletedProps, deletedUnmergedBCSet.size()); + } // save stats detailedGCStats.propertiesDeleted(deletedProps); + detailedGCStats.unmergedBranchCommitsDeleted(deletedUnmergedBCSet.size()); detailedGCStats.documentsUpdated(updatedDocs); - detailedGCStats.documentsSkippedUpdation(oldDocs.size() - updatedDocs); + detailedGCStats.documentsUpdateSkipped(oldDocs.size() - updatedDocs); } finally { + // now reset delete metadata + updateOpList.clear(); + deletedPropsCountMap.clear(); + deletedUnmergedBCSet.clear(); + garbageDocsCount = 0; delayOnModifications(timer.stop().elapsed(MILLISECONDS), cancel); } } diff --git a/oak-store-document/src/test/java/org/apache/jackrabbit/oak/plugins/document/BranchCommitGCTest.java b/oak-store-document/src/test/java/org/apache/jackrabbit/oak/plugins/document/BranchCommitGCTest.java index 79d7f9bbae..7597b71d32 100644 --- a/oak-store-document/src/test/java/org/apache/jackrabbit/oak/plugins/document/BranchCommitGCTest.java +++ b/oak-store-document/src/test/java/org/apache/jackrabbit/oak/plugins/document/BranchCommitGCTest.java @@ -16,33 +16,37 @@ */ package org.apache.jackrabbit.oak.plugins.document; -import static java.util.concurrent.TimeUnit.HOURS; -import static org.junit.Assert.assertEquals; -import static org.junit.Assert.assertFalse; -import static org.junit.Assert.assertNotEquals; -import static org.junit.Assert.assertNotNull; -import static org.junit.Assert.assertNull; -import static org.junit.Assert.assertTrue; - -import java.util.LinkedList; -import java.util.List; -import java.util.Map; -import java.util.Map.Entry; -import java.util.function.Consumer; - import org.apache.jackrabbit.oak.plugins.document.DocumentMK.Builder; import org.apache.jackrabbit.oak.plugins.document.util.Utils; import org.apache.jackrabbit.oak.spi.commit.CommitInfo; import org.apache.jackrabbit.oak.spi.commit.EmptyHook; import org.apache.jackrabbit.oak.spi.state.NodeBuilder; -import org.apache.jackrabbit.oak.spi.state.NodeState; -import org.apache.jackrabbit.oak.spi.state.NodeStore; import org.apache.jackrabbit.oak.stats.Clock; import org.junit.After; import org.junit.Before; import org.junit.Rule; import org.junit.Test; +import java.util.LinkedList; +import java.util.List; +import java.util.Map; +import java.util.Map.Entry; +import java.util.Objects; +import java.util.function.Consumer; + +import static java.util.concurrent.TimeUnit.HOURS; +import static org.apache.commons.lang3.reflect.FieldUtils.writeField; +import static org.apache.jackrabbit.oak.plugins.document.DetailGCHelper.assertBranchRevisionRemovedFromAllDocuments; +import static org.apache.jackrabbit.oak.plugins.document.DetailGCHelper.build; +import static org.apache.jackrabbit.oak.plugins.document.DetailGCHelper.mergedBranchCommit; +import static org.apache.jackrabbit.oak.plugins.document.DetailGCHelper.unmergedBranchCommit; +import static org.junit.Assert.assertEquals; +import static org.junit.Assert.assertFalse; +import static org.junit.Assert.assertNotEquals; +import static org.junit.Assert.assertNotNull; +import static org.junit.Assert.assertNull; +import static org.junit.Assert.assertTrue; + public class BranchCommitGCTest { @Rule @@ -72,28 +76,6 @@ public class BranchCommitGCTest { Revision.resetClockToDefault(); } - private void assertNoEmptyProperties() { - for (NodeDocument nd : Utils.getAllDocuments(store.getDocumentStore())) { - for (Entry<String, Object> e : nd.data.entrySet()) { - Object v = e.getValue(); - if (v instanceof Map) { - @SuppressWarnings("rawtypes") - Map m = (Map) v; - if (m.isEmpty() - && (e.getKey().equals("_commitRoot") - || e.getKey().equals("_collisions")) - && nd.getId().equals("0:/")) { - // skip : root document apparently has an empty _commitRoot:{} - continue; - } - assertFalse("document has empty property : id=" + nd.getId() - + ", property=" + e.getKey() + ", document=" + nd.asString(), - m.isEmpty()); - } - } - } - } - @Test public void unmergedAddChildren() throws Exception { RevisionVector br = unmergedBranchCommit(b -> { @@ -102,10 +84,8 @@ public class BranchCommitGCTest { }); assertExists("1:/a"); assertExists("1:/b"); - assertFalse( - store.getDocumentStore().find(Collection.NODES, "1:/a").wasDeletedOnce()); - assertFalse( - store.getDocumentStore().find(Collection.NODES, "1:/b").wasDeletedOnce()); + assertFalse(store.getDocumentStore().find(Collection.NODES, "1:/a").wasDeletedOnce()); + assertFalse(store.getDocumentStore().find(Collection.NODES, "1:/b").wasDeletedOnce()); store.runBackgroundOperations(); @@ -115,20 +95,20 @@ public class BranchCommitGCTest { VersionGarbageCollector.VersionGCStats stats = gc.gc(1, HOURS); assertEquals(3, stats.updatedDetailedGCDocsCount); + assertEquals(1, stats.deletedUnmergedBCCount); - assertTrue( - store.getDocumentStore().find(Collection.NODES, "1:/a").wasDeletedOnce()); - assertTrue( - store.getDocumentStore().find(Collection.NODES, "1:/b").wasDeletedOnce()); + assertTrue(store.getDocumentStore().find(Collection.NODES, "1:/a").wasDeletedOnce()); + assertTrue(store.getDocumentStore().find(Collection.NODES, "1:/b").wasDeletedOnce()); // now do another gc to get those documents actually deleted clock.waitUntil(clock.getTime() + HOURS.toMillis(2)); stats = gc.gc(1, HOURS); assertEquals(0, stats.updatedDetailedGCDocsCount); assertEquals(2, stats.deletedDocGCCount); + assertEquals(0, stats.deletedUnmergedBCCount); assertNotExists("1:/a"); assertNotExists("1:/b"); - assertBranchRevisionRemovedFromAllDocuments(br); + assertBranchRevisionRemovedFromAllDocuments(store, br); } @Test @@ -139,10 +119,8 @@ public class BranchCommitGCTest { }); assertExists("1:/a"); assertExists("1:/b"); - assertFalse( - store.getDocumentStore().find(Collection.NODES, "1:/a").wasDeletedOnce()); - assertFalse( - store.getDocumentStore().find(Collection.NODES, "1:/b").wasDeletedOnce()); + assertFalse(store.getDocumentStore().find(Collection.NODES, "1:/a").wasDeletedOnce()); + assertFalse(store.getDocumentStore().find(Collection.NODES, "1:/b").wasDeletedOnce()); store.runBackgroundOperations(); @@ -165,11 +143,10 @@ public class BranchCommitGCTest { // clean everything older than one hour VersionGarbageCollector.VersionGCStats stats = gc.gc(1, HOURS); - assertTrue( - "stats.updatedDetailedGCDocsCount expected 1 or less, was: " - + stats.updatedDetailedGCDocsCount, + assertTrue("stats.updatedDetailedGCDocsCount expected 1 or less, was: " + stats.updatedDetailedGCDocsCount, stats.updatedDetailedGCDocsCount <= 1); assertEquals(2, stats.deletedDocGCCount); + assertEquals(1, stats.deletedUnmergedBCCount); assertNotExists("1:/a"); assertNotExists("1:/b"); @@ -178,7 +155,50 @@ public class BranchCommitGCTest { stats = gc.gc(1, HOURS); assertEquals(0, stats.updatedDetailedGCDocsCount); assertEquals(0, stats.deletedDocGCCount); - assertBranchRevisionRemovedFromAllDocuments(br); + assertEquals(0, stats.deletedUnmergedBCCount); + assertBranchRevisionRemovedFromAllDocuments(store, br); + } + + @Test + public void testDeletedPropsAndUnmergedBC() throws Exception { + // create a node with property. + NodeBuilder nb = store.getRoot().builder(); + nb.child("bar").setProperty("prop", "value"); + nb.child("bar").setProperty("x", "y"); + store.merge(nb, EmptyHook.INSTANCE, CommitInfo.EMPTY); + store.runBackgroundOperations(); + + // remove the property + nb = store.getRoot().builder(); + nb.child("bar").removeProperty("prop"); + store.merge(nb, EmptyHook.INSTANCE, CommitInfo.EMPTY); + store.runBackgroundOperations(); + + // create branch commits + mergedBranchCommit(b -> b.child("foo").setProperty("p", "prop")); + RevisionVector br1 = unmergedBranchCommit(b -> b.child("foo").setProperty("a", "b")); + RevisionVector br2 = unmergedBranchCommit(b -> b.child("foo").setProperty("a", "c")); + RevisionVector br3 = unmergedBranchCommit(b -> b.child("foo").setProperty("a", "d")); + RevisionVector br4 = unmergedBranchCommit(b -> b.child("bar").setProperty("x", "z")); + mergedBranchCommit(b -> b.child("foo").removeProperty("p")); + store.runBackgroundOperations(); + + // enable the detailed gc flag + writeField(gc, "detailedGCEnabled", true, true); + + // wait two hours + clock.waitUntil(clock.getTime() + HOURS.toMillis(2)); + // clean everything older than one hour + VersionGarbageCollector.VersionGCStats stats = gc.gc(1, HOURS); + + assertEquals(3, stats.updatedDetailedGCDocsCount); + // deleted properties are : 1:/foo -> prop, a, _collisions & p && 1:/bar -> _bc + assertEquals(5, stats.deletedPropsCount); + assertEquals(4, stats.deletedUnmergedBCCount); + assertBranchRevisionRemovedFromAllDocuments(store, br1); + assertBranchRevisionRemovedFromAllDocuments(store, br2); + assertBranchRevisionRemovedFromAllDocuments(store, br3); + assertBranchRevisionRemovedFromAllDocuments(store, br4); } @Test @@ -193,27 +213,24 @@ public class BranchCommitGCTest { }); assertExists("1:/a"); assertExists("1:/b"); - assertFalse( - store.getDocumentStore().find(Collection.NODES, "1:/a").wasDeletedOnce()); - assertFalse( - store.getDocumentStore().find(Collection.NODES, "1:/b").wasDeletedOnce()); + assertFalse(store.getDocumentStore().find(Collection.NODES, "1:/a").wasDeletedOnce()); + assertFalse(store.getDocumentStore().find(Collection.NODES, "1:/b").wasDeletedOnce()); store.runBackgroundOperations(); - Long originalModified = store.getDocumentStore().find(Collection.NODES, "1:/a") - .getModified(); + Long originalModified = store.getDocumentStore().find(Collection.NODES, "1:/a").getModified(); // wait two hours clock.waitUntil(clock.getTime() + HOURS.toMillis(2)); // clean everything older than one hour VersionGarbageCollector.VersionGCStats stats = gc.gc(1, HOURS); - Long laterModified = store.getDocumentStore().find(Collection.NODES, "1:/a") - .getModified(); + Long laterModified = store.getDocumentStore().find(Collection.NODES, "1:/a").getModified(); assertNotEquals(originalModified, laterModified); assertEquals(3, stats.updatedDetailedGCDocsCount); assertEquals(0, stats.deletedDocGCCount); + assertEquals(2, stats.deletedUnmergedBCCount); assertExists("1:/a"); assertExists("1:/b"); @@ -223,8 +240,9 @@ public class BranchCommitGCTest { stats = gc.gc(1, HOURS); assertEquals(0, stats.updatedDetailedGCDocsCount); assertEquals(2, stats.deletedDocGCCount); - assertBranchRevisionRemovedFromAllDocuments(br1); - assertBranchRevisionRemovedFromAllDocuments(br2); + assertEquals(0, stats.deletedUnmergedBCCount); + assertBranchRevisionRemovedFromAllDocuments(store, br1); + assertBranchRevisionRemovedFromAllDocuments(store, br2); } @Test @@ -239,10 +257,8 @@ public class BranchCommitGCTest { }); assertExists("1:/a"); assertExists("1:/b"); - assertFalse( - store.getDocumentStore().find(Collection.NODES, "1:/a").wasDeletedOnce()); - assertFalse( - store.getDocumentStore().find(Collection.NODES, "1:/b").wasDeletedOnce()); + assertFalse(store.getDocumentStore().find(Collection.NODES, "1:/a").wasDeletedOnce()); + assertFalse(store.getDocumentStore().find(Collection.NODES, "1:/b").wasDeletedOnce()); store.runBackgroundOperations(); @@ -255,12 +271,13 @@ public class BranchCommitGCTest { // wait two hours clock.waitUntil(clock.getTime() + HOURS.toMillis(2)); - // clean everything older than one hours + // clean everything older than one hour VersionGarbageCollector.VersionGCStats stats = gc.gc(1, HOURS); assertTrue("should have been 2 or more, was: " + stats.updatedDetailedGCDocsCount, stats.updatedDetailedGCDocsCount >= 2); assertEquals(0, stats.deletedDocGCCount); + assertEquals(2, stats.deletedUnmergedBCCount); assertExists("1:/a"); assertExists("1:/b"); @@ -270,8 +287,9 @@ public class BranchCommitGCTest { stats = gc.gc(1, HOURS); assertEquals(0, stats.updatedDetailedGCDocsCount); assertEquals(0, stats.deletedDocGCCount); - assertBranchRevisionRemovedFromAllDocuments(br1); - assertBranchRevisionRemovedFromAllDocuments(br2); + assertEquals(0, stats.deletedUnmergedBCCount); + assertBranchRevisionRemovedFromAllDocuments(store, br1); + assertBranchRevisionRemovedFromAllDocuments(store, br2); } @Test @@ -286,10 +304,8 @@ public class BranchCommitGCTest { }); assertExists("1:/a"); assertExists("1:/b"); - assertFalse( - store.getDocumentStore().find(Collection.NODES, "1:/a").wasDeletedOnce()); - assertFalse( - store.getDocumentStore().find(Collection.NODES, "1:/b").wasDeletedOnce()); + assertFalse(store.getDocumentStore().find(Collection.NODES, "1:/a").wasDeletedOnce()); + assertFalse(store.getDocumentStore().find(Collection.NODES, "1:/b").wasDeletedOnce()); store.runBackgroundOperations(); @@ -318,6 +334,7 @@ public class BranchCommitGCTest { assertEquals(3, stats.updatedDetailedGCDocsCount); assertEquals(0, stats.deletedDocGCCount); + assertEquals(4, stats.deletedUnmergedBCCount); assertExists("1:/a"); assertExists("1:/b"); @@ -327,20 +344,11 @@ public class BranchCommitGCTest { stats = gc.gc(1, HOURS); assertEquals(0, stats.updatedDetailedGCDocsCount); assertEquals(0, stats.deletedDocGCCount); - assertBranchRevisionRemovedFromAllDocuments(br1); - assertBranchRevisionRemovedFromAllDocuments(br2); - assertBranchRevisionRemovedFromAllDocuments(br3); - assertBranchRevisionRemovedFromAllDocuments(br4); - } - - private void assertNotExists(String id) { - NodeDocument doc = store.getDocumentStore().find(Collection.NODES, id); - assertNull("doc exists but was expected not to : id=" + id, doc); - } - - private void assertExists(String id) { - NodeDocument doc = store.getDocumentStore().find(Collection.NODES, id); - assertNotNull("doc does not exist : id=" + id, doc); + assertEquals(0, stats.deletedUnmergedBCCount); + assertBranchRevisionRemovedFromAllDocuments(store, br1); + assertBranchRevisionRemovedFromAllDocuments(store, br2); + assertBranchRevisionRemovedFromAllDocuments(store, br3); + assertBranchRevisionRemovedFromAllDocuments(store, br4); } @Test @@ -351,18 +359,19 @@ public class BranchCommitGCTest { }); RevisionVector br = unmergedBranchCommit(b -> { b.child("test").remove(); - b.getChildNode("foo").child("childfoo"); + b.getChildNode("foo").child("childFoo"); }); store.runBackgroundOperations(); // wait two hours clock.waitUntil(clock.getTime() + HOURS.toMillis(2)); - // clean everything older than one hours + // clean everything older than one hour VersionGarbageCollector.VersionGCStats stats = gc.gc(1, HOURS); // first gc round will only mark document for deleting by second round assertEquals(0, stats.deletedDocGCCount); assertEquals(4, stats.updatedDetailedGCDocsCount); + assertEquals(1, stats.deletedUnmergedBCCount); // wait two hours clock.waitUntil(clock.getTime() + HOURS.toMillis(2)); @@ -370,25 +379,8 @@ public class BranchCommitGCTest { stats = gc.gc(1, HOURS); assertEquals(0, stats.updatedDetailedGCDocsCount); assertEquals(1, stats.deletedDocGCCount); - assertBranchRevisionRemovedFromAllDocuments(br); - } - - private void assertBranchRevisionRemovedFromAllDocuments(RevisionVector br) { - assertTrue(br.isBranch()); - Revision br1 = br.getRevision(1); - Revision r1 = br1.asTrunkRevision(); - for (NodeDocument nd : Utils.getAllDocuments(store.getDocumentStore())) { - if (nd.getId().equals("0:/")) { - NodeDocument target = new NodeDocument(store.getDocumentStore()); - nd.deepCopy(target); - } - System.out.println("nd=" + nd.asString()); - if (nd.asString().contains(r1.toString())) { - System.out.println("break"); - } - assertFalse("document not fully cleaned up: " + nd.asString(), - nd.asString().contains(r1.toString())); - } + assertEquals(0, stats.deletedUnmergedBCCount); + assertBranchRevisionRemovedFromAllDocuments(store, br); } @Test @@ -406,27 +398,30 @@ public class BranchCommitGCTest { // wait two hours clock.waitUntil(clock.getTime() + HOURS.toMillis(2)); - // clean everything older than one hours + // clean everything older than one hour stats = gc.gc(1, HOURS); assertEquals(2, stats.updatedDetailedGCDocsCount); - assertBranchRevisionRemovedFromAllDocuments(br); + assertEquals(0, stats.deletedPropsCount); + assertEquals(1, stats.deletedUnmergedBCCount); + assertBranchRevisionRemovedFromAllDocuments(store, br); } @Test public void unmergedAddProperty() throws Exception { mergedBranchCommit(b -> b.child("foo")); - RevisionVector br = unmergedBranchCommit( - b -> b.child("foo").setProperty("a", "b")); + RevisionVector br = unmergedBranchCommit(b -> b.child("foo").setProperty("a", "b")); store.runBackgroundOperations(); // wait two hours clock.waitUntil(clock.getTime() + HOURS.toMillis(2)); - // clean everything older than one hours + // clean everything older than one hour stats = gc.gc(1, HOURS); assertEquals(2, stats.updatedDetailedGCDocsCount); - assertBranchRevisionRemovedFromAllDocuments(br); + assertEquals(1, stats.deletedPropsCount); + assertEquals(1, stats.deletedUnmergedBCCount); + assertBranchRevisionRemovedFromAllDocuments(store, br); } @Test @@ -439,6 +434,7 @@ public class BranchCommitGCTest { store.runBackgroundOperations(); stats = gc.gc(1, HOURS); assertEquals(0, stats.updatedDetailedGCDocsCount); + assertEquals(0, stats.deletedUnmergedBCCount); final List<RevisionVector> brs = new LinkedList<>(); for (int j = 0; j < 10; j++) { @@ -451,10 +447,11 @@ public class BranchCommitGCTest { // wait two hours clock.waitUntil(clock.getTime() + HOURS.toMillis(2)); - // clean everything older than one hours + // clean everything older than one hour stats = gc.gc(1, HOURS); assertEquals(2, stats.updatedDetailedGCDocsCount); + assertEquals(10, stats.deletedUnmergedBCCount); doc = store.getDocumentStore().find(Collection.NODES, "1:/foo"); Long finalModified = doc.getModified(); @@ -462,7 +459,7 @@ public class BranchCommitGCTest { assertEquals(originalModified, finalModified); for (RevisionVector br : brs) { - assertBranchRevisionRemovedFromAllDocuments(br); + assertBranchRevisionRemovedFromAllDocuments(store, br); } } @@ -476,6 +473,7 @@ public class BranchCommitGCTest { store.runBackgroundOperations(); stats = gc.gc(1, HOURS); assertEquals(0, stats.updatedDetailedGCDocsCount); + assertEquals(0, stats.deletedUnmergedBCCount); for (int i = 0; i < 50; i++) { mergedBranchCommit(b -> b.child("foo").remove()); @@ -489,12 +487,13 @@ public class BranchCommitGCTest { // wait two hours clock.waitUntil(clock.getTime() + HOURS.toMillis(2)); - // clean everything older than one hours + // clean everything older than one hour stats = gc.gc(1, HOURS); assertEquals(2, stats.updatedDetailedGCDocsCount); + assertEquals(10, stats.deletedUnmergedBCCount); for (RevisionVector br : brs) { - assertBranchRevisionRemovedFromAllDocuments(br); + assertBranchRevisionRemovedFromAllDocuments(store, br); } } @@ -507,6 +506,8 @@ public class BranchCommitGCTest { store.runBackgroundOperations(); stats = gc.gc(1, HOURS); assertEquals(0, stats.updatedDetailedGCDocsCount); + assertEquals(0, stats.deletedUnmergedBCCount); + assertEquals(0, stats.deletedPropsCount); RevisionVector br = unmergedBranchCommit(b -> { b.setProperty("rootProp", "v"); @@ -514,20 +515,26 @@ public class BranchCommitGCTest { }); store.runBackgroundOperations(); DocumentNodeStore store2 = newStore(2); - mergedBranchCommit(store2, b -> b.child("foo").removeProperty("a")); + DetailGCHelper.mergedBranchCommit(store2, b -> b.child("foo").removeProperty("a")); store2.runBackgroundOperations(); store2.dispose(); store.runBackgroundOperations(); // wait two hours clock.waitUntil(clock.getTime() + HOURS.toMillis(2)); - // clean everything older than one hours + // clean everything older than one hour stats = gc.gc(1, HOURS); assertEquals(2, stats.updatedDetailedGCDocsCount); - assertBranchRevisionRemovedFromAllDocuments(br); + assertEquals(1, stats.deletedUnmergedBCCount); + // deleted properties are 0:/ -> rootProp, _collisions & 1:/foo -> a + assertEquals(3, stats.deletedPropsCount); + assertBranchRevisionRemovedFromAllDocuments(store, br); } + + // helper methods + private DocumentNodeStore newStore(int clusterId) { Builder builder = builderProvider.newBuilder().clock(clock) .setLeaseCheckMode(LeaseCheckMode.DISABLED).setDetailedGCEnabled(true) @@ -535,50 +542,46 @@ public class BranchCommitGCTest { if (clusterId > 0) { builder.setClusterId(clusterId); } - DocumentNodeStore store2 = builder.getNodeStore(); - return store2; + return builder.getNodeStore(); } - private RevisionVector mergedBranchCommit(Consumer<NodeBuilder> buildFunction) - throws Exception { + private RevisionVector mergedBranchCommit(Consumer<NodeBuilder> buildFunction) throws Exception { return build(store, true, true, buildFunction); } - private RevisionVector mergedBranchCommit(NodeStore store, - Consumer<NodeBuilder> buildFunction) throws Exception { - return build(store, true, true, buildFunction); - } - - private RevisionVector unmergedBranchCommit(Consumer<NodeBuilder> buildFunction) - throws Exception { + private RevisionVector unmergedBranchCommit(Consumer<NodeBuilder> buildFunction) throws Exception { RevisionVector result = build(store, true, false, buildFunction); assertTrue(result.isBranch()); return result; } - private RevisionVector build(NodeStore store, boolean persistToBranch, boolean merge, - Consumer<NodeBuilder> buildFunction) throws Exception { - if (!persistToBranch && !merge) { - throw new IllegalArgumentException("must either persistToBranch or merge"); - } - NodeBuilder b = store.getRoot().builder(); - buildFunction.accept(b); - RevisionVector result = null; - if (persistToBranch) { - DocumentRootBuilder drb = (DocumentRootBuilder) b; - drb.persist(); - DocumentNodeState ns = (DocumentNodeState) drb.getNodeState(); - result = ns.getLastRevision(); - } - if (merge) { - DocumentNodeState dns = (DocumentNodeState) merge(b); - result = dns.getLastRevision(); - } - return result; + private void assertNotExists(String id) { + NodeDocument doc = store.getDocumentStore().find(Collection.NODES, id); + assertNull("doc exists but was expected not to : id=" + id, doc); + } + + private void assertExists(String id) { + NodeDocument doc = store.getDocumentStore().find(Collection.NODES, id); + assertNotNull("doc does not exist : id=" + id, doc); } - private NodeState merge(NodeBuilder builder) throws Exception { - return store.merge(builder, EmptyHook.INSTANCE, CommitInfo.EMPTY); + private void assertNoEmptyProperties() { + for (NodeDocument nd : Utils.getAllDocuments(store.getDocumentStore())) { + for (Entry<String, Object> e : nd.data.entrySet()) { + Object v = e.getValue(); + if (v instanceof Map) { + @SuppressWarnings("rawtypes") + Map m = (Map) v; + if (m.isEmpty() && (e.getKey().equals("_commitRoot") || e.getKey().equals("_collisions")) + && Objects.equals(nd.getId(), "0:/")) { + // skip : root document apparently has an empty _commitRoot:{} + continue; + } + assertFalse("document has empty property : id=" + nd.getId() + + ", property=" + e.getKey() + ", document=" + nd.asString(), m.isEmpty()); + } + } + } } } diff --git a/oak-store-document/src/test/java/org/apache/jackrabbit/oak/plugins/document/DetailGCHelper.java b/oak-store-document/src/test/java/org/apache/jackrabbit/oak/plugins/document/DetailGCHelper.java index d52c5e33ae..61b8fa0ab5 100644 --- a/oak-store-document/src/test/java/org/apache/jackrabbit/oak/plugins/document/DetailGCHelper.java +++ b/oak-store-document/src/test/java/org/apache/jackrabbit/oak/plugins/document/DetailGCHelper.java @@ -18,7 +18,19 @@ */ package org.apache.jackrabbit.oak.plugins.document; +import org.apache.jackrabbit.oak.plugins.document.util.Utils; +import org.apache.jackrabbit.oak.spi.commit.CommitInfo; +import org.apache.jackrabbit.oak.spi.commit.EmptyHook; +import org.apache.jackrabbit.oak.spi.state.NodeBuilder; +import org.apache.jackrabbit.oak.spi.state.NodeState; +import org.apache.jackrabbit.oak.spi.state.NodeStore; + +import java.util.Objects; +import java.util.function.Consumer; + import static org.apache.commons.lang3.reflect.FieldUtils.writeField; +import static org.junit.Assert.assertFalse; +import static org.junit.Assert.assertTrue; public class DetailGCHelper { @@ -33,4 +45,53 @@ public class DetailGCHelper { writeField(vgc, "detailedGCEnabled", false, true); } } + + public static RevisionVector mergedBranchCommit(final NodeStore store, final Consumer<NodeBuilder> buildFunction) throws Exception { + return build(store, true, true, buildFunction); + } + + public static RevisionVector unmergedBranchCommit(final NodeStore store, final Consumer<NodeBuilder> buildFunction) throws Exception { + RevisionVector result = build(store, true, false, buildFunction); + assertTrue(result.isBranch()); + return result; + } + + public static void assertBranchRevisionRemovedFromAllDocuments(final DocumentNodeStore store, final RevisionVector br) { + assertTrue(br.isBranch()); + Revision br1 = br.getRevision(1); + assert br1 != null; + Revision r1 = br1.asTrunkRevision(); + for (NodeDocument nd : Utils.getAllDocuments(store.getDocumentStore())) { + if (Objects.equals(nd.getId(), "0:/")) { + NodeDocument target = new NodeDocument(store.getDocumentStore()); + nd.deepCopy(target); + } + assertFalse("document not fully cleaned up: " + nd.asString(), nd.asString().contains(r1.toString())); + } + } + + static RevisionVector build(final NodeStore store, final boolean persistToBranch, final boolean merge, + final Consumer<NodeBuilder> buildFunction) throws Exception { + if (!persistToBranch && !merge) { + throw new IllegalArgumentException("must either persistToBranch or merge"); + } + NodeBuilder b = store.getRoot().builder(); + buildFunction.accept(b); + RevisionVector result = null; + if (persistToBranch) { + DocumentRootBuilder drb = (DocumentRootBuilder) b; + drb.persist(); + DocumentNodeState ns = (DocumentNodeState) drb.getNodeState(); + result = ns.getLastRevision(); + } + if (merge) { + DocumentNodeState dns = (DocumentNodeState) merge(store, b); + result = dns.getLastRevision(); + } + return result; + } + + private static NodeState merge(final NodeStore store, final NodeBuilder builder) throws Exception { + return store.merge(builder, EmptyHook.INSTANCE, CommitInfo.EMPTY); + } } diff --git a/oak-store-document/src/test/java/org/apache/jackrabbit/oak/plugins/document/DetailedRevisionGCStatsCollectorImplTest.java b/oak-store-document/src/test/java/org/apache/jackrabbit/oak/plugins/document/DetailedRevisionGCStatsCollectorImplTest.java index b84084d439..3af5165f5c 100644 --- a/oak-store-document/src/test/java/org/apache/jackrabbit/oak/plugins/document/DetailedRevisionGCStatsCollectorImplTest.java +++ b/oak-store-document/src/test/java/org/apache/jackrabbit/oak/plugins/document/DetailedRevisionGCStatsCollectorImplTest.java @@ -40,6 +40,7 @@ import static org.apache.jackrabbit.oak.plugins.document.DetailedRevisionGCStats import static org.apache.jackrabbit.oak.plugins.document.DetailedRevisionGCStatsCollectorImpl.COLLECT_UNMERGED_BC_TIMER; import static org.apache.jackrabbit.oak.plugins.document.DetailedRevisionGCStatsCollectorImpl.COUNTER; import static org.apache.jackrabbit.oak.plugins.document.DetailedRevisionGCStatsCollectorImpl.DELETED_PROPERTY; +import static org.apache.jackrabbit.oak.plugins.document.DetailedRevisionGCStatsCollectorImpl.DELETED_UNMERGED_BC; import static org.apache.jackrabbit.oak.plugins.document.DetailedRevisionGCStatsCollectorImpl.DELETE_DETAILED_GC_DOCS_TIMER; import static org.apache.jackrabbit.oak.plugins.document.DetailedRevisionGCStatsCollectorImpl.DETAILED_GC; import static org.apache.jackrabbit.oak.plugins.document.DetailedRevisionGCStatsCollectorImpl.DETAILED_GC_ACTIVE_TIMER; @@ -78,7 +79,7 @@ public class DetailedRevisionGCStatsCollectorImplTest { public void getDocumentsSkippedUpdationCount() throws IllegalAccessException { Meter m = getMeter(SKIPPED_DOC); long count = m.getCount(); - stats.documentsSkippedUpdation(17); + stats.documentsUpdateSkipped(17); assertEquals(count + 17, m.getCount()); assertEquals(count + 17, ((MeterStats) readField(stats, "skippedDoc", true)).getCount()); } @@ -92,6 +93,15 @@ public class DetailedRevisionGCStatsCollectorImplTest { assertEquals(count + 10, ((MeterStats) readField(stats, "deletedProperty", true)).getCount()); } + @Test + public void getUnmergedBCDeletedCount() throws IllegalAccessException { + Meter m = getMeter(DELETED_UNMERGED_BC); + long count = m.getCount(); + stats.unmergedBranchCommitsDeleted(10); + assertEquals(count + 10, m.getCount()); + assertEquals(count + 10, ((MeterStats) readField(stats, "deletedUnmergedBC", true)).getCount()); + } + @Test public void getDocumentsUpdatedCount() throws IllegalAccessException { Meter m = getMeter(UPDATED_DOC); diff --git a/oak-store-document/src/test/java/org/apache/jackrabbit/oak/plugins/document/VersionGarbageCollectorIT.java b/oak-store-document/src/test/java/org/apache/jackrabbit/oak/plugins/document/VersionGarbageCollectorIT.java index 5f7335d73a..87203073d4 100644 --- a/oak-store-document/src/test/java/org/apache/jackrabbit/oak/plugins/document/VersionGarbageCollectorIT.java +++ b/oak-store-document/src/test/java/org/apache/jackrabbit/oak/plugins/document/VersionGarbageCollectorIT.java @@ -50,6 +50,9 @@ import static org.apache.jackrabbit.oak.api.Type.STRING; import static org.apache.jackrabbit.oak.api.Type.STRINGS; import static org.apache.jackrabbit.oak.plugins.document.Collection.NODES; import static org.apache.jackrabbit.oak.plugins.document.Collection.SETTINGS; +import static org.apache.jackrabbit.oak.plugins.document.DetailGCHelper.assertBranchRevisionRemovedFromAllDocuments; +import static org.apache.jackrabbit.oak.plugins.document.DetailGCHelper.mergedBranchCommit; +import static org.apache.jackrabbit.oak.plugins.document.DetailGCHelper.unmergedBranchCommit; import static org.apache.jackrabbit.oak.plugins.document.NodeDocument.MIN_ID_VALUE; import static org.apache.jackrabbit.oak.plugins.document.NodeDocument.NUM_REVS_THRESHOLD; import static org.apache.jackrabbit.oak.plugins.document.NodeDocument.PREV_SPLIT_FACTOR; @@ -320,7 +323,7 @@ public class VersionGarbageCollectorIT { // 6. Now run gc after checkpoint and see removed properties gets collected clock.waitUntil(clock.getTime() + delta*2); VersionGCStats stats = gc.gc(delta, MILLISECONDS); - assertEquals(1, stats.deletedPropsGCCount); + assertEquals(1, stats.deletedPropsCount); assertEquals(1, stats.updatedDetailedGCDocsCount); assertTrue(stats.ignoredGCDueToCheckPoint); assertFalse(stats.ignoredDetailedGCDueToCheckPoint); @@ -354,7 +357,7 @@ public class VersionGarbageCollectorIT { //1. Go past GC age and check no GC done as nothing deleted clock.waitUntil(getCurrentTimestamp() + maxAge); VersionGCStats stats = gc.gc(maxAge, HOURS); - assertEquals(0, stats.deletedPropsGCCount); + assertEquals(0, stats.deletedPropsCount); assertEquals(0, stats.updatedDetailedGCDocsCount); //Remove property @@ -368,14 +371,14 @@ public class VersionGarbageCollectorIT { //Clock cannot move back (it moved forward in #1) so double the maxAge clock.waitUntil(clock.getTime() + delta); stats = gc.gc(maxAge*2, HOURS); - assertEquals(0, stats.deletedPropsGCCount); + assertEquals(0, stats.deletedPropsCount); assertEquals(0, stats.updatedDetailedGCDocsCount); //3. Check that deleted property does get collected post maxAge clock.waitUntil(clock.getTime() + HOURS.toMillis(maxAge*2) + delta); stats = gc.gc(maxAge*2, HOURS); - assertEquals(1, stats.deletedPropsGCCount); + assertEquals(1, stats.deletedPropsCount); assertEquals(1, stats.updatedDetailedGCDocsCount); assertEquals(MIN_ID_VALUE, stats.oldestModifiedDocId); @@ -390,7 +393,7 @@ public class VersionGarbageCollectorIT { clock.waitUntil(clock.getTime() + HOURS.toMillis(maxAge*2) + delta); stats = gc.gc(maxAge*2, HOURS); - assertEquals(0, stats.deletedPropsGCCount); + assertEquals(0, stats.deletedPropsCount); assertEquals(0, stats.updatedDetailedGCDocsCount); assertEquals(MIN_ID_VALUE, stats.oldestModifiedDocId); } @@ -428,7 +431,7 @@ public class VersionGarbageCollectorIT { clock.waitUntil(clock.getTime() + HOURS.toMillis(maxAge*2) + delta); VersionGCStats stats = gc.gc(maxAge*2, HOURS); - assertEquals(50_000, stats.deletedPropsGCCount); + assertEquals(50_000, stats.deletedPropsCount); assertEquals(5_000, stats.updatedDetailedGCDocsCount); assertEquals(MIN_ID_VALUE, stats.oldestModifiedDocId); } @@ -471,7 +474,7 @@ public class VersionGarbageCollectorIT { clock.waitUntil(clock.getTime() + HOURS.toMillis(maxAge*2) + delta); VersionGCStats stats = gc.gc(maxAge, HOURS); - assertEquals(50_000, stats.deletedPropsGCCount); + assertEquals(50_000, stats.deletedPropsCount); assertEquals(5_000, stats.updatedDetailedGCDocsCount); assertEquals(MIN_ID_VALUE, stats.oldestModifiedDocId); } @@ -534,7 +537,7 @@ public class VersionGarbageCollectorIT { //1. Go past GC age and check no GC done as nothing deleted clock.waitUntil(getCurrentTimestamp() + maxAge); VersionGCStats stats = gc.gc(maxAge, HOURS); - assertEquals(0, stats.deletedPropsGCCount); + assertEquals(0, stats.deletedPropsCount); //Remove property NodeBuilder b2 = store.getRoot().builder(); @@ -548,7 +551,7 @@ public class VersionGarbageCollectorIT { clock.waitUntil(clock.getTime() + HOURS.toMillis(maxAge*2) + delta); stats = gc.gc(maxAge*2, HOURS); - assertEquals(10, stats.deletedPropsGCCount); + assertEquals(10, stats.deletedPropsCount); assertEquals(10, stats.updatedDetailedGCDocsCount); assertEquals(MIN_ID_VALUE, stats.oldestModifiedDocId); @@ -573,7 +576,7 @@ public class VersionGarbageCollectorIT { // increment the clock again by more than 2 hours + delta clock.waitUntil(clock.getTime() + HOURS.toMillis(maxAge*2) + delta); stats = gc.gc(maxAge*2, HOURS); - assertEquals(10, stats.deletedPropsGCCount); + assertEquals(10, stats.deletedPropsCount); assertEquals(10, stats.updatedDetailedGCDocsCount); assertEquals(MIN_ID_VALUE, stats.oldestModifiedDocId); } @@ -641,7 +644,7 @@ public class VersionGarbageCollectorIT { // increment the clock again by more than 2 hours + delta clock.waitUntil(clock.getTime() + HOURS.toMillis(maxAge*2) + delta); VersionGCStats stats = gc.gc(maxAge*2, HOURS); - assertEquals(10, stats.deletedPropsGCCount); + assertEquals(10, stats.deletedPropsCount); assertEquals(10, stats.updatedDetailedGCDocsCount); assertEquals(MIN_ID_VALUE, stats.oldestModifiedDocId); } @@ -664,7 +667,7 @@ public class VersionGarbageCollectorIT { //1. Go past GC age and check no GC done as nothing deleted clock.waitUntil(getCurrentTimestamp() + maxAge); VersionGCStats stats = gc.gc(maxAge, HOURS); - assertEquals(0, stats.deletedPropsGCCount); + assertEquals(0, stats.deletedPropsCount); assertEquals(0, stats.updatedDetailedGCDocsCount); //Remove property @@ -680,14 +683,14 @@ public class VersionGarbageCollectorIT { //Clock cannot move back (it moved forward in #1) so double the maxAge clock.waitUntil(clock.getTime() + delta); stats = gc.gc(maxAge*2, HOURS); - assertEquals(0, stats.deletedPropsGCCount); + assertEquals(0, stats.deletedPropsCount); assertEquals(0, stats.updatedDetailedGCDocsCount); //3. Check that deleted property does get collected post maxAge clock.waitUntil(clock.getTime() + HOURS.toMillis(maxAge*2) + delta); stats = gc.gc(maxAge*2, HOURS); - assertEquals(10, stats.deletedPropsGCCount); + assertEquals(10, stats.deletedPropsCount); //4. Check that a revived property (deleted and created again) does not get gc NodeBuilder b4 = store.getRoot().builder(); @@ -698,7 +701,7 @@ public class VersionGarbageCollectorIT { clock.waitUntil(clock.getTime() + HOURS.toMillis(maxAge*2) + delta); stats = gc.gc(maxAge*2, HOURS); - assertEquals(0, stats.deletedPropsGCCount); + assertEquals(0, stats.deletedPropsCount); assertEquals(0, stats.updatedDetailedGCDocsCount); } @@ -722,7 +725,7 @@ public class VersionGarbageCollectorIT { //1. Go past GC age and check no GC done as nothing deleted clock.waitUntil(getCurrentTimestamp() + maxAge); VersionGCStats stats = gc.gc(maxAge, HOURS); - assertEquals(0, stats.deletedPropsGCCount); + assertEquals(0, stats.deletedPropsCount); assertEquals(0, stats.updatedDetailedGCDocsCount); //Remove property @@ -738,14 +741,14 @@ public class VersionGarbageCollectorIT { //Clock cannot move back (it moved forward in #1) so double the maxAge clock.waitUntil(clock.getTime() + delta); stats = gc.gc(maxAge*2, HOURS); - assertEquals(0, stats.deletedPropsGCCount); + assertEquals(0, stats.deletedPropsCount); assertEquals(0, stats.updatedDetailedGCDocsCount); //3. Check that deleted property does get collected post maxAge clock.waitUntil(clock.getTime() + HOURS.toMillis(maxAge*2) + delta); stats = gc.gc(maxAge*2, HOURS); - assertEquals(10, stats.deletedPropsGCCount); + assertEquals(10, stats.deletedPropsCount); //4. Check that a revived property (deleted and created again) does not get gc NodeBuilder b4 = store.getRoot().builder(); @@ -756,7 +759,7 @@ public class VersionGarbageCollectorIT { clock.waitUntil(clock.getTime() + HOURS.toMillis(maxAge*2) + delta); stats = gc.gc(maxAge*2, HOURS); - assertEquals(0, stats.deletedPropsGCCount); + assertEquals(0, stats.deletedPropsCount); assertEquals(0, stats.updatedDetailedGCDocsCount); } @@ -789,7 +792,7 @@ public class VersionGarbageCollectorIT { //1. Go past GC age and check no GC done as nothing deleted clock.waitUntil(getCurrentTimestamp() + maxAge); VersionGCStats stats = gc.gc(maxAge, HOURS); - assertEquals(0, stats.deletedPropsGCCount); + assertEquals(0, stats.deletedPropsCount); assertEquals(0, stats.updatedDetailedGCDocsCount); //Remove property @@ -805,14 +808,14 @@ public class VersionGarbageCollectorIT { //Clock cannot move back (it moved forward in #1) so double the maxAge clock.waitUntil(clock.getTime() + delta); stats = gc.gc(maxAge*2, HOURS); - assertEquals(0, stats.deletedPropsGCCount); + assertEquals(0, stats.deletedPropsCount); assertEquals(0, stats.updatedDetailedGCDocsCount); //3. Check that deleted property does get collected post maxAge clock.waitUntil(clock.getTime() + HOURS.toMillis(maxAge*2) + delta); stats = gc.gc(maxAge*2, HOURS); - assertEquals(10, stats.deletedPropsGCCount); + assertEquals(10, stats.deletedPropsCount); } @Test @@ -844,7 +847,7 @@ public class VersionGarbageCollectorIT { //1. Go past GC age and check no GC done as nothing deleted clock.waitUntil(getCurrentTimestamp() + maxAge); VersionGCStats stats = gc.gc(maxAge, HOURS); - assertEquals(0, stats.deletedPropsGCCount); + assertEquals(0, stats.deletedPropsCount); assertEquals(0, stats.updatedDetailedGCDocsCount); //Remove property @@ -860,14 +863,14 @@ public class VersionGarbageCollectorIT { //Clock cannot move back (it moved forward in #1) so double the maxAge clock.waitUntil(clock.getTime() + delta); stats = gc.gc(maxAge*2, HOURS); - assertEquals(0, stats.deletedPropsGCCount); + assertEquals(0, stats.deletedPropsCount); assertEquals(0, stats.updatedDetailedGCDocsCount); //3. Check that deleted property does get collected post maxAge clock.waitUntil(clock.getTime() + HOURS.toMillis(maxAge*2) + delta); stats = gc.gc(maxAge*2, HOURS); - assertEquals(10, stats.deletedPropsGCCount); + assertEquals(10, stats.deletedPropsCount); } @Test @@ -888,7 +891,7 @@ public class VersionGarbageCollectorIT { //1. Go past GC age and check no GC done as nothing deleted clock.waitUntil(getCurrentTimestamp() + maxAge); VersionGCStats stats = gc.gc(maxAge, HOURS); - assertEquals(0, stats.deletedPropsGCCount); + assertEquals(0, stats.deletedPropsCount); assertEquals(0, stats.updatedDetailedGCDocsCount); //Remove property @@ -903,7 +906,7 @@ public class VersionGarbageCollectorIT { //Clock cannot move back (it moved forward in #1) so double the maxAge clock.waitUntil(clock.getTime() + delta); stats = gc.gc(maxAge*2, HOURS); - assertEquals(0, stats.deletedPropsGCCount); + assertEquals(0, stats.deletedPropsCount); assertEquals(0, stats.updatedDetailedGCDocsCount); assertEquals(MIN_ID_VALUE, stats.oldestModifiedDocId); // as GC hadn't run @@ -932,7 +935,7 @@ public class VersionGarbageCollectorIT { VersionGarbageCollector gc = new VersionGarbageCollector(store, gcSupport, true); stats = gc.gc(maxAge*2, HOURS); assertEquals(0, stats.updatedDetailedGCDocsCount); - assertEquals(0, stats.deletedPropsGCCount); + assertEquals(0, stats.deletedPropsCount); assertEquals(MIN_ID_VALUE, stats.oldestModifiedDocId); } @@ -957,7 +960,7 @@ public class VersionGarbageCollectorIT { //1. Go past GC age and check no GC done as nothing deleted clock.waitUntil(getCurrentTimestamp() + maxAge); VersionGCStats stats = gc.gc(maxAge, HOURS); - assertEquals(0, stats.deletedPropsGCCount); + assertEquals(0, stats.deletedPropsCount); assertEquals(0, stats.updatedDetailedGCDocsCount); //Remove property @@ -1002,11 +1005,93 @@ public class VersionGarbageCollectorIT { stats = gcRef.get().gc(maxAge*2, HOURS); assertTrue(stats.canceled); assertEquals(0, stats.updatedDetailedGCDocsCount); - assertEquals(0, stats.deletedPropsGCCount); + assertEquals(0, stats.deletedPropsCount); assertEquals(MIN_ID_VALUE, stats.oldestModifiedDocId); } // OAK-10199 END + + // OAK-8646 + @Test + public void testDeletedPropsAndUnmergedBCWithoutCollision() throws Exception { + // create a node with property. + NodeBuilder nb = store.getRoot().builder(); + nb.child("bar").setProperty("prop", "value"); + nb.child("bar").setProperty("x", "y"); + store.merge(nb, EmptyHook.INSTANCE, CommitInfo.EMPTY); + store.runBackgroundOperations(); + + // remove the property + nb = store.getRoot().builder(); + nb.child("bar").removeProperty("prop"); + store.merge(nb, EmptyHook.INSTANCE, CommitInfo.EMPTY); + store.runBackgroundOperations(); + + // create branch commits + mergedBranchCommit(store, b -> b.child("foo").setProperty("p", "prop")); + RevisionVector br1 = unmergedBranchCommit(store, b -> b.child("foo").setProperty("a", "b")); + RevisionVector br4 = unmergedBranchCommit(store, b -> b.child("bar").setProperty("x", "z")); + mergedBranchCommit(store, b -> b.child("foo").removeProperty("p")); + store.runBackgroundOperations(); + + // enable the detailed gc flag + writeField(gc, "detailedGCEnabled", true, true); + + // wait two hours + clock.waitUntil(clock.getTime() + HOURS.toMillis(2)); + // clean everything older than one hour + VersionGCStats stats = gc.gc(1, HOURS); + + assertEquals(3, stats.updatedDetailedGCDocsCount); + // deleted properties are : 1:/foo -> prop, a & p && 1:/bar -> _bc + assertEquals(4, stats.deletedPropsCount); + assertEquals(2, stats.deletedUnmergedBCCount); + assertBranchRevisionRemovedFromAllDocuments(store, br1); + assertBranchRevisionRemovedFromAllDocuments(store, br4); + } + + @Test + public void testDeletedPropsAndUnmergedBCWithCollision() throws Exception { + // create a node with property. + NodeBuilder nb = store.getRoot().builder(); + nb.child("bar").setProperty("prop", "value"); + nb.child("bar").setProperty("x", "y"); + store.merge(nb, EmptyHook.INSTANCE, CommitInfo.EMPTY); + store.runBackgroundOperations(); + + // remove the property + nb = store.getRoot().builder(); + nb.child("bar").removeProperty("prop"); + store.merge(nb, EmptyHook.INSTANCE, CommitInfo.EMPTY); + store.runBackgroundOperations(); + + // create branch commits + mergedBranchCommit(store, b -> b.child("foo").setProperty("p", "prop")); + RevisionVector br1 = unmergedBranchCommit(store, b -> b.child("foo").setProperty("a", "b")); + RevisionVector br2 = unmergedBranchCommit(store, b -> b.child("foo").setProperty("a", "c")); + RevisionVector br3 = unmergedBranchCommit(store, b -> b.child("foo").setProperty("a", "d")); + RevisionVector br4 = unmergedBranchCommit(store, b -> b.child("bar").setProperty("x", "z")); + mergedBranchCommit(store, b -> b.child("foo").removeProperty("p")); + store.runBackgroundOperations(); + + // enable the detailed gc flag + writeField(gc, "detailedGCEnabled", true, true); + + // wait two hours + clock.waitUntil(clock.getTime() + HOURS.toMillis(2)); + // clean everything older than one hour + VersionGCStats stats = gc.gc(1, HOURS); + + assertEquals(3, stats.updatedDetailedGCDocsCount); + // deleted properties are : 1:/foo -> prop, a, _collisions & p && 1:/bar -> _bc + assertEquals(5, stats.deletedPropsCount); + assertEquals(4, stats.deletedUnmergedBCCount); + assertBranchRevisionRemovedFromAllDocuments(store, br1); + assertBranchRevisionRemovedFromAllDocuments(store, br2); + assertBranchRevisionRemovedFromAllDocuments(store, br3); + assertBranchRevisionRemovedFromAllDocuments(store, br4); + } + // OAK-8646 END private void gcSplitDocsInternal(String subNodeName) throws Exception { long maxAge = 1; //hrs
