You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@jackrabbit.apache.org by "Joscorbe (via GitHub)" <gi...@apache.org> on 2024/02/26 23:41:50 UTC
[PR] Old revisions cleanup (rebased) [jackrabbit-oak]
Joscorbe opened a new pull request, #1328:
URL: https://github.com/apache/jackrabbit-oak/pull/1328
Introduce a way to safely detect and delete old revisions in a document. This could be useful to cleanup documents that sometimes grows above the supported size in MongoDB (16MB).
This is still WIP and needs to be integrated into the DetailedGC logic.
This PR should supersede #1191
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@jackrabbit.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] OAK-10535: Clean up old revisions in a document [jackrabbit-oak]
Posted by "Joscorbe (via GitHub)" <gi...@apache.org>.
Joscorbe commented on code in PR #1328:
URL: https://github.com/apache/jackrabbit-oak/pull/1328#discussion_r1504557325
##########
oak-store-document/src/main/java/org/apache/jackrabbit/oak/plugins/document/NodeDocumentRevisionCleaner.java:
##########
@@ -0,0 +1,270 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.jackrabbit.oak.plugins.document;
+
+import org.apache.jackrabbit.oak.plugins.document.util.Utils;
+
+import java.time.Instant;
+import java.time.temporal.ChronoUnit;
+import java.util.Map;
+import java.util.SortedMap;
+import java.util.TreeMap;
+import java.util.TreeSet;
+
+public class NodeDocumentRevisionCleaner {
+
+ private final DocumentStore documentStore;
+ private final DocumentNodeStore documentNodeStore;
+ private final NodeDocument workingDocument;
+ private final RevisionPropertiesClassifier revisionClassifier;
+ private final RevisionCleanerUtility revisionCleaner;
+
+ /**
+ * Constructor for NodeDocumentRevisionCleaner.
+ * @param documentStore The DocumentStore instance. Must be writable to perform cleanup.
+ * @param documentNodeStore The DocumentNodeStore instance.
+ * @param workingDocument The document to clean up.
+ */
+ public NodeDocumentRevisionCleaner(DocumentStore documentStore, DocumentNodeStore documentNodeStore,
Review Comment:
In the initial version this would receive a read-only DS and the write one. It is now obsolete since I don't think we are going to run this from oak-run console, but from the revisions command.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@jackrabbit.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] OAK-10535: Clean up old revisions in a document [jackrabbit-oak]
Posted by "stefan-egli (via GitHub)" <gi...@apache.org>.
stefan-egli commented on code in PR #1328:
URL: https://github.com/apache/jackrabbit-oak/pull/1328#discussion_r1504318420
##########
oak-store-document/src/main/java/org/apache/jackrabbit/oak/plugins/document/NodeDocumentRevisionCleaner.java:
##########
@@ -0,0 +1,270 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.jackrabbit.oak.plugins.document;
+
+import org.apache.jackrabbit.oak.plugins.document.util.Utils;
+
+import java.time.Instant;
+import java.time.temporal.ChronoUnit;
+import java.util.Map;
+import java.util.SortedMap;
+import java.util.TreeMap;
+import java.util.TreeSet;
+
+public class NodeDocumentRevisionCleaner {
+
+ private final DocumentStore documentStore;
+ private final DocumentNodeStore documentNodeStore;
+ private final NodeDocument workingDocument;
+ private final RevisionPropertiesClassifier revisionClassifier;
+ private final RevisionCleanerUtility revisionCleaner;
+
+ /**
+ * Constructor for NodeDocumentRevisionCleaner.
+ * @param documentStore The DocumentStore instance. Must be writable to perform cleanup.
+ * @param documentNodeStore The DocumentNodeStore instance.
+ * @param workingDocument The document to clean up.
+ */
+ public NodeDocumentRevisionCleaner(DocumentStore documentStore, DocumentNodeStore documentNodeStore,
+ NodeDocument workingDocument) {
+ this.workingDocument = workingDocument;
+ this.documentStore = documentStore;
+ this.documentNodeStore = documentNodeStore;
+
+ revisionClassifier = new RevisionPropertiesClassifier(workingDocument);
+ revisionCleaner = new RevisionCleanerUtility(revisionClassifier);
+ }
+
+ /**
+ * Collects cleanable old revisions for the given document.
+ */
+ public void collectOldRevisions(UpdateOp op) {
+ revisionClassifier.classifyRevisionsAndProperties();
+ revisionCleaner.preserveRevisionsNewerThanThreshold(24, ChronoUnit.HOURS);
+ revisionCleaner.preserveLastRevisionForEachProperty();
+ revisionCleaner.preserveRevisionsReferencedByCheckpoints();
+ revisionCleaner.removeCandidatesInList();
+
+ /*for (Map.Entry<Integer, TreeSet<Revision>> entry : revisionCleaner.candidateRevisionsToClean.entrySet()) {
+ for (Revision revision : entry.getValue()) {
+ System.out.println("Removing revision " + revision);
+ TreeSet<String> properties = revisionClassifier.propertiesModifiedByRevision.get(revision);
+ System.out.println("Properties modified by this revision: " + properties);
+ if (properties != null) {
+ for (String property : properties) {
+ op.removeMapEntry(property, revision);
+ }
+ }
+
+ op.removeMapEntry("_revisions", revision);
+ }
+ }*/
+ }
+
+ protected class RevisionPropertiesClassifier {
+ private final NodeDocument workingDocument;
+ private SortedMap<String, SortedMap<Integer, TreeSet<Revision>>> revisionsModifyingPropertyByCluster;
+ private SortedMap<String, TreeSet<Revision>> revisionsModifyingProperty;
+ private SortedMap<Revision, TreeSet<String>> propertiesModifiedByRevision;
+
+ private RevisionPropertiesClassifier(NodeDocument workingDocument) {
+ this.workingDocument = workingDocument;
+
+ this.revisionsModifyingPropertyByCluster = new TreeMap<>();
+ this.revisionsModifyingProperty = new TreeMap<>();
+ this.propertiesModifiedByRevision = new TreeMap<>(StableRevisionComparator.INSTANCE);
+ }
+
+ /**
+ * This method processes the revisions of the working document, creating maps to
+ * track the relationships between revisions and modified properties.
+ */
+ protected void classifyRevisionsAndProperties() {
+ SortedMap<Revision, String> deletedRevisions = workingDocument.getLocalDeleted();
+ // Always keep the first "_deleted" entry, as is when the document was created
+ if (!deletedRevisions.isEmpty()) {
+ Revision createdRevision = deletedRevisions.firstKey();
+ revisionCleaner.addBlockedRevisionToKeep(createdRevision);
+ }
+
+ SortedMap<Revision, String> documentRevisions = workingDocument.getLocalRevisions();
+ for (Map.Entry<Revision, String> revisionEntry : documentRevisions.entrySet()) {
+ Revision revision = revisionEntry.getKey();
+ String revisionValue = revisionEntry.getValue();
+
+ // Only check committed revisions
+ if (Utils.isCommitted(revisionValue)) {
+ // Candidate to clean up
+ revisionCleaner.addCandidateRevisionToClean(revision);
+ // Store properties usage
+ classifyPropertiesModifiedByRevision(revision);
+ }
+ }
+ }
+
+ private void classifyPropertiesModifiedByRevision(Revision revision) {
+ for (Map.Entry<String, Object> propertyEntry : workingDocument.entrySet()) {
+ if (Utils.isPropertyName(propertyEntry.getKey()) || NodeDocument.isDeletedEntry(propertyEntry.getKey())) {
+ Map<Revision, String> valueMap = (Map) propertyEntry.getValue();
+ if (valueMap.containsKey(revision)) {
+ propertiesModifiedByRevision.computeIfAbsent(revision, key ->
+ new TreeSet<>()).add(propertyEntry.getKey()
+ );
+
+ 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);
+ }
+ }
+ }
+ }
+ }
+
+ protected class RevisionCleanerUtility {
+
+ private final SortedMap<Integer, TreeSet<Revision>> blockedRevisionsToKeep;
+ private final SortedMap<Integer, TreeSet<Revision>> candidateRevisionsToClean;
+ private final RevisionPropertiesClassifier revisionClassifier;
Review Comment:
While these are internal (presumably `private`) classes, so no expected interference from elsewhere, and given these are not value or holder but more worker type of classes, it might be worth while to avoid direct field access between each other (and instead go via getter methods)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@jackrabbit.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] OAK-10535: Clean up old revisions in a document [jackrabbit-oak]
Posted by "Joscorbe (via GitHub)" <gi...@apache.org>.
Joscorbe commented on code in PR #1328:
URL: https://github.com/apache/jackrabbit-oak/pull/1328#discussion_r1511343772
##########
oak-run/src/main/java/org/apache/jackrabbit/oak/run/RevisionsCommand.java:
##########
@@ -460,6 +494,97 @@ private void sweep(RevisionsOptions options, Closer closer)
SweepHelper.sweep(store, new RevisionContextWrapper(ns, clusterId), seeker);
}
+ private void cleanup(RevisionsOptions options, Closer closer) throws IOException {
+ String path = options.getPath();
+ if (path == null || path.isEmpty()) {
+ System.err.println("path option is required for " + RevisionsOptions.CMD_CLEANUP + " command");
+ return;
+ }
+
+ DocumentNodeStoreBuilder<?> builder = createDocumentMKBuilder(options, closer);
+ if (builder == null) {
+ System.err.println("revisions mode only available for DocumentNodeStore");
+ return;
+ }
+
+ DocumentStore documentStore = builder.getDocumentStore();
+ builder.setReadOnlyMode();
+ useMemoryBlobStore(builder);
+ DocumentNodeStore documentNodeStore = builder.build();
+ String id = org.apache.jackrabbit.oak.plugins.document.util.Utils.getIdFromPath(path);
+ NodeDocument workingDocument = documentStore.find(NODES, id);
+ NodeDocumentRevisionCleaner revisionCleaner = new NodeDocumentRevisionCleaner(documentStore, documentNodeStore, workingDocument);
Review Comment:
This now calls the general detailedGC
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@jackrabbit.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] OAK-10535: Clean up old revisions in a document [jackrabbit-oak]
Posted by "Joscorbe (via GitHub)" <gi...@apache.org>.
Joscorbe commented on code in PR #1328:
URL: https://github.com/apache/jackrabbit-oak/pull/1328#discussion_r1515291565
##########
oak-run/src/main/java/org/apache/jackrabbit/oak/run/RevisionsCommand.java:
##########
@@ -74,15 +80,18 @@ public class RevisionsCommand implements Command {
private static final Logger LOG = LoggerFactory.getLogger(RevisionsCommand.class);
+ private static final int REVISION_CAP = getInteger("oak.revision.cap", 250);
+
private static final String USAGE = Joiner.on(System.lineSeparator()).join(
"revisions {<jdbc-uri> | <mongodb-uri>} <sub-command> [options]",
"where sub-command is one of",
" info give information about the revisions state without performing",
Review Comment:
Fixed
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@jackrabbit.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] OAK-10535: Clean up old revisions in a document [jackrabbit-oak]
Posted by "Joscorbe (via GitHub)" <gi...@apache.org>.
Joscorbe commented on code in PR #1328:
URL: https://github.com/apache/jackrabbit-oak/pull/1328#discussion_r1515277468
##########
oak-store-document/src/main/java/org/apache/jackrabbit/oak/plugins/document/VersionGarbageCollector.java:
##########
@@ -1398,6 +1399,24 @@ private void delayOnModifications(final long durationMs, final AtomicBoolean can
}
}
+ public UpdateOp collectGarbageOnDocument(DocumentNodeStore store, NodeDocument doc) {
Review Comment:
After some work in that area, I'm abandoning this approach. I initially thought it would be a good idea because that method is already quite long, so splitting it could improve its readability. However, the _real core_ part would be [just these lines](https://github.com/apache/jackrabbit-oak/blob/cf870b532b107b0c243e1d0180f1213d3f6e9197/oak-store-document/src/main/java/org/apache/jackrabbit/oak/plugins/document/VersionGarbageCollector.java#L752-L756), as the rest of the code is essentially to keep the progress of the iterators.
So I simply [added this part](https://github.com/apache/jackrabbit-oak/blob/cf870b532b107b0c243e1d0180f1213d3f6e9197/oak-store-document/src/main/java/org/apache/jackrabbit/oak/plugins/document/VersionGarbageCollector.java#L768-L772) to the facade method I already added.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@jackrabbit.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] OAK-10535: Clean up old revisions in a document [jackrabbit-oak]
Posted by "rishabhdaim (via GitHub)" <gi...@apache.org>.
rishabhdaim commented on code in PR #1328:
URL: https://github.com/apache/jackrabbit-oak/pull/1328#discussion_r1511357714
##########
oak-run/src/main/java/org/apache/jackrabbit/oak/run/RevisionsCommand.java:
##########
@@ -32,6 +37,7 @@
import java.util.concurrent.TimeUnit;
import java.util.concurrent.TimeoutException;
import java.util.concurrent.atomic.AtomicBoolean;
+import java.util.stream.Collectors;
Review Comment:
Not used inside code. Please remove this.
##########
oak-run/src/main/java/org/apache/jackrabbit/oak/run/RevisionsCommand.java:
##########
@@ -74,15 +89,18 @@ public class RevisionsCommand implements Command {
private static final Logger LOG = LoggerFactory.getLogger(RevisionsCommand.class);
+ private static final int REVISION_CAP = getInteger("oak.revision.cap", 250);
+
private static final String USAGE = Joiner.on(System.lineSeparator()).join(
"revisions {<jdbc-uri> | <mongodb-uri>} <sub-command> [options]",
"where sub-command is one of",
" info give information about the revisions state without performing",
" any modifications",
- " collect perform garbage collection",
- " reset clear all persisted metadata",
- " sweep clean up uncommitted changes",
- " detailedGC perform detailed garbage collection i.e. remove unmerged branch commits, old revisions, deleted properties etc"
+ " collect perform garbage collection",
+ " reset clear all persisted metadata",
+ " sweep clean up uncommitted changes",
+ " detailedGC perform detailed garbage collection i.e. remove unmerged branch commits, old revisions, deleted properties etc",
+ " pathCleanup clean up old/unused revisions and unmerged branch commits on a specific path"
Review Comment:
we are removing whole detailed garbage here. I would change this description to reflect that it is performing detailedGC on given path.
##########
oak-run/src/main/java/org/apache/jackrabbit/oak/run/RevisionsCommand.java:
##########
@@ -460,6 +479,55 @@ private void sweep(RevisionsOptions options, Closer closer)
SweepHelper.sweep(store, new RevisionContextWrapper(ns, clusterId), seeker);
}
+ private void pathCleanup(RevisionsOptions options, Closer closer) throws IOException {
+ String path = options.getPath();
+ if (path == null || path.isEmpty()) {
+ System.err.println("path option is required for " + RevisionsOptions.CMD_CLEANUP + " command");
+ return;
+ }
+
+ DocumentNodeStoreBuilder<?> builder = createDocumentMKBuilder(options, closer);
+ if (builder == null) {
+ System.err.println("revisions mode only available for DocumentNodeStore");
+ return;
+ }
+
+ DocumentStore documentStore = builder.getDocumentStore();
+ builder.setReadOnlyMode();
+ useMemoryBlobStore(builder);
+ DocumentNodeStore documentNodeStore = builder.build();
+ String id = org.apache.jackrabbit.oak.plugins.document.util.Utils.getIdFromPath(path);
+ NodeDocument workingDocument = documentStore.find(NODES, id);
+ //NodeDocumentRevisionCleaner revisionCleaner = new NodeDocumentRevisionCleaner(documentNodeStore, workingDocument);
+
+ VersionGarbageCollector gc = bootstrapVGC(options, closer, true);
+ // Set a LoggingGC monitor that will output the detailedGC operations
+ gc.setGCMonitor(new LoggingGCMonitor(LOG));
+ // Run detailedGC on the given document
+ UpdateOp op = gc.collectGarbageOnDocument(documentNodeStore, workingDocument);
Review Comment:
Returning `UpdateOp` from detailedGC and handling deletion here is not a good idea, cause we might need to duplicate the code written in `DetailedGC.removeGarbage()` OR we might miss something.
In this case, we are missing orphan nodes cause they are handled differently in `detailedGC.removeGarbage()`.
I would delegate the collection & removal of garbage to DetailedGC itself.
##########
oak-run/src/main/java/org/apache/jackrabbit/oak/run/RevisionsCommand.java:
##########
@@ -23,6 +23,11 @@
import java.io.IOException;
import java.util.List;
import java.util.Locale;
+import java.util.Map;
+import java.util.Scanner;
+import java.util.SortedMap;
+import java.util.TreeMap;
+import java.util.TreeSet;
Review Comment:
please remove these un-used imports.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@jackrabbit.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] OAK-10535: Clean up old revisions in a document [jackrabbit-oak]
Posted by "Joscorbe (via GitHub)" <gi...@apache.org>.
Joscorbe commented on PR #1328:
URL: https://github.com/apache/jackrabbit-oak/pull/1328#issuecomment-1976895834
I have squashed the WIP commits, sorry for the forced-PR.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@jackrabbit.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] OAK-10535: Clean up old revisions in a document [jackrabbit-oak]
Posted by "stefan-egli (via GitHub)" <gi...@apache.org>.
stefan-egli commented on code in PR #1328:
URL: https://github.com/apache/jackrabbit-oak/pull/1328#discussion_r1504304416
##########
oak-run/src/main/java/org/apache/jackrabbit/oak/run/RevisionsCommand.java:
##########
@@ -460,6 +494,97 @@ private void sweep(RevisionsOptions options, Closer closer)
SweepHelper.sweep(store, new RevisionContextWrapper(ns, clusterId), seeker);
}
+ private void cleanup(RevisionsOptions options, Closer closer) throws IOException {
+ String path = options.getPath();
+ if (path == null || path.isEmpty()) {
+ System.err.println("path option is required for " + RevisionsOptions.CMD_CLEANUP + " command");
+ return;
+ }
+
+ DocumentNodeStoreBuilder<?> builder = createDocumentMKBuilder(options, closer);
+ if (builder == null) {
+ System.err.println("revisions mode only available for DocumentNodeStore");
+ return;
+ }
+
+ DocumentStore documentStore = builder.getDocumentStore();
+ builder.setReadOnlyMode();
+ useMemoryBlobStore(builder);
+ DocumentNodeStore documentNodeStore = builder.build();
+ String id = org.apache.jackrabbit.oak.plugins.document.util.Utils.getIdFromPath(path);
+ NodeDocument workingDocument = documentStore.find(NODES, id);
+ NodeDocumentRevisionCleaner revisionCleaner = new NodeDocumentRevisionCleaner(documentStore, documentNodeStore, workingDocument);
Review Comment:
As was discussed offline, we'd need to change this into a way which calls GC in general (or if that is unfeasible at least call DetailedGC in general).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@jackrabbit.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] OAK-10535: Clean up old revisions in a document [jackrabbit-oak]
Posted by "stefan-egli (via GitHub)" <gi...@apache.org>.
stefan-egli commented on code in PR #1328:
URL: https://github.com/apache/jackrabbit-oak/pull/1328#discussion_r1504309300
##########
oak-store-document/src/test/java/org/apache/jackrabbit/oak/plugins/document/NodeDocumentRevisionCleanerTest.java:
##########
@@ -0,0 +1,549 @@
+package org.apache.jackrabbit.oak.plugins.document;
+
+import com.fasterxml.jackson.core.type.TypeReference;
+import com.fasterxml.jackson.databind.ObjectMapper;
+import org.junit.Before;
+import org.junit.Test;
+import org.mockito.Mock;
+import org.mockito.Mockito;
+import org.mockito.MockitoAnnotations;
+
+import java.io.IOException;
+import java.time.Instant;
+import java.time.temporal.ChronoUnit;
+import java.util.ArrayList;
+import java.util.Collections;
+import java.util.List;
+import java.util.Map;
+import java.util.Set;
+import java.util.SortedMap;
+import java.util.TreeMap;
+import java.util.TreeSet;
+import java.util.stream.Collectors;
+import java.util.stream.Stream;
+
+import static org.apache.jackrabbit.oak.plugins.document.Collection.NODES;
+import static org.junit.Assert.assertEquals;
+import static org.junit.Assert.assertFalse;
+import static org.junit.Assert.assertNull;
+import static org.junit.Assert.assertTrue;
+
+public class NodeDocumentRevisionCleanerTest {
+
+ @Mock
+ DocumentStore documentStore;
+
+ @Mock
+ DocumentNodeStore documentNodeStore;
+
+ @Mock
+ Checkpoints checkpoints;
+
+ NodeDocument workingDocument;
+
+ NodeDocumentRevisionCleaner nodeDocumentRevisionCleaner;
+
+ @Before
+ public void setUp() {
+ MockitoAnnotations.initMocks(this);
Review Comment:
This shows up as deprecated. Would using the suggested alternative (`MockitoAnnotations.openMocks` - plus a later `close()`) work?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@jackrabbit.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] OAK-10535: Clean up old revisions in a document [jackrabbit-oak]
Posted by "stefan-egli (via GitHub)" <gi...@apache.org>.
stefan-egli commented on code in PR #1328:
URL: https://github.com/apache/jackrabbit-oak/pull/1328#discussion_r1504283327
##########
oak-run/src/main/java/org/apache/jackrabbit/oak/run/RevisionsCommand.java:
##########
@@ -82,6 +97,7 @@ public class RevisionsCommand implements Command {
" collect perform garbage collection",
" reset clear all persisted metadata",
" sweep clean up uncommitted changes",
+ " cleanup clean up old/unused revisions",
Review Comment:
We might be able to handle this under the existing `collect` command - that one currently runs classic GC, but you can get it to do detailed GC as well, with the corresponding parameter.
(What's then missing is the ability to restrict it to specifically only a path)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@jackrabbit.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] OAK-10535: Clean up old revisions in a document [jackrabbit-oak]
Posted by "Joscorbe (via GitHub)" <gi...@apache.org>.
Joscorbe commented on code in PR #1328:
URL: https://github.com/apache/jackrabbit-oak/pull/1328#discussion_r1511343112
##########
oak-store-document/src/main/java/org/apache/jackrabbit/oak/plugins/document/NodeDocumentRevisionCleaner.java:
##########
@@ -0,0 +1,270 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.jackrabbit.oak.plugins.document;
+
+import org.apache.jackrabbit.oak.plugins.document.util.Utils;
+
+import java.time.Instant;
+import java.time.temporal.ChronoUnit;
+import java.util.Map;
+import java.util.SortedMap;
+import java.util.TreeMap;
+import java.util.TreeSet;
+
+public class NodeDocumentRevisionCleaner {
+
+ private final DocumentStore documentStore;
+ private final DocumentNodeStore documentNodeStore;
+ private final NodeDocument workingDocument;
+ private final RevisionPropertiesClassifier revisionClassifier;
+ private final RevisionCleanerUtility revisionCleaner;
+
+ /**
+ * Constructor for NodeDocumentRevisionCleaner.
+ * @param documentStore The DocumentStore instance. Must be writable to perform cleanup.
+ * @param documentNodeStore The DocumentNodeStore instance.
+ * @param workingDocument The document to clean up.
+ */
+ public NodeDocumentRevisionCleaner(DocumentStore documentStore, DocumentNodeStore documentNodeStore,
+ NodeDocument workingDocument) {
+ this.workingDocument = workingDocument;
+ this.documentStore = documentStore;
+ this.documentNodeStore = documentNodeStore;
+
+ revisionClassifier = new RevisionPropertiesClassifier(workingDocument);
+ revisionCleaner = new RevisionCleanerUtility(revisionClassifier);
+ }
+
+ /**
+ * Collects cleanable old revisions for the given document.
+ */
+ public void collectOldRevisions(UpdateOp op) {
+ revisionClassifier.classifyRevisionsAndProperties();
+ revisionCleaner.preserveRevisionsNewerThanThreshold(24, ChronoUnit.HOURS);
+ revisionCleaner.preserveLastRevisionForEachProperty();
+ revisionCleaner.preserveRevisionsReferencedByCheckpoints();
+ revisionCleaner.removeCandidatesInList();
+
+ /*for (Map.Entry<Integer, TreeSet<Revision>> entry : revisionCleaner.candidateRevisionsToClean.entrySet()) {
+ for (Revision revision : entry.getValue()) {
+ System.out.println("Removing revision " + revision);
+ TreeSet<String> properties = revisionClassifier.propertiesModifiedByRevision.get(revision);
+ System.out.println("Properties modified by this revision: " + properties);
+ if (properties != null) {
+ for (String property : properties) {
+ op.removeMapEntry(property, revision);
+ }
+ }
+
+ op.removeMapEntry("_revisions", revision);
+ }
+ }*/
+ }
+
+ protected class RevisionPropertiesClassifier {
Review Comment:
Left over from initial testing. I have made it private.
##########
oak-store-document/src/main/java/org/apache/jackrabbit/oak/plugins/document/NodeDocumentRevisionCleaner.java:
##########
@@ -0,0 +1,270 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.jackrabbit.oak.plugins.document;
+
+import org.apache.jackrabbit.oak.plugins.document.util.Utils;
+
+import java.time.Instant;
+import java.time.temporal.ChronoUnit;
+import java.util.Map;
+import java.util.SortedMap;
+import java.util.TreeMap;
+import java.util.TreeSet;
+
+public class NodeDocumentRevisionCleaner {
+
+ private final DocumentStore documentStore;
+ private final DocumentNodeStore documentNodeStore;
+ private final NodeDocument workingDocument;
+ private final RevisionPropertiesClassifier revisionClassifier;
+ private final RevisionCleanerUtility revisionCleaner;
+
+ /**
+ * Constructor for NodeDocumentRevisionCleaner.
+ * @param documentStore The DocumentStore instance. Must be writable to perform cleanup.
+ * @param documentNodeStore The DocumentNodeStore instance.
+ * @param workingDocument The document to clean up.
+ */
+ public NodeDocumentRevisionCleaner(DocumentStore documentStore, DocumentNodeStore documentNodeStore,
+ NodeDocument workingDocument) {
+ this.workingDocument = workingDocument;
+ this.documentStore = documentStore;
+ this.documentNodeStore = documentNodeStore;
+
+ revisionClassifier = new RevisionPropertiesClassifier(workingDocument);
+ revisionCleaner = new RevisionCleanerUtility(revisionClassifier);
+ }
+
+ /**
+ * Collects cleanable old revisions for the given document.
+ */
+ public void collectOldRevisions(UpdateOp op) {
+ revisionClassifier.classifyRevisionsAndProperties();
+ revisionCleaner.preserveRevisionsNewerThanThreshold(24, ChronoUnit.HOURS);
+ revisionCleaner.preserveLastRevisionForEachProperty();
+ revisionCleaner.preserveRevisionsReferencedByCheckpoints();
+ revisionCleaner.removeCandidatesInList();
+
+ /*for (Map.Entry<Integer, TreeSet<Revision>> entry : revisionCleaner.candidateRevisionsToClean.entrySet()) {
+ for (Revision revision : entry.getValue()) {
+ System.out.println("Removing revision " + revision);
+ TreeSet<String> properties = revisionClassifier.propertiesModifiedByRevision.get(revision);
+ System.out.println("Properties modified by this revision: " + properties);
+ if (properties != null) {
+ for (String property : properties) {
+ op.removeMapEntry(property, revision);
+ }
+ }
+
+ op.removeMapEntry("_revisions", revision);
+ }
+ }*/
+ }
+
+ protected class RevisionPropertiesClassifier {
+ private final NodeDocument workingDocument;
+ private SortedMap<String, SortedMap<Integer, TreeSet<Revision>>> revisionsModifyingPropertyByCluster;
+ private SortedMap<String, TreeSet<Revision>> revisionsModifyingProperty;
+ private SortedMap<Revision, TreeSet<String>> propertiesModifiedByRevision;
+
+ private RevisionPropertiesClassifier(NodeDocument workingDocument) {
+ this.workingDocument = workingDocument;
+
+ this.revisionsModifyingPropertyByCluster = new TreeMap<>();
+ this.revisionsModifyingProperty = new TreeMap<>();
+ this.propertiesModifiedByRevision = new TreeMap<>(StableRevisionComparator.INSTANCE);
+ }
+
+ /**
+ * This method processes the revisions of the working document, creating maps to
+ * track the relationships between revisions and modified properties.
+ */
+ protected void classifyRevisionsAndProperties() {
+ SortedMap<Revision, String> deletedRevisions = workingDocument.getLocalDeleted();
+ // Always keep the first "_deleted" entry, as is when the document was created
+ if (!deletedRevisions.isEmpty()) {
+ Revision createdRevision = deletedRevisions.firstKey();
+ revisionCleaner.addBlockedRevisionToKeep(createdRevision);
+ }
+
+ SortedMap<Revision, String> documentRevisions = workingDocument.getLocalRevisions();
+ for (Map.Entry<Revision, String> revisionEntry : documentRevisions.entrySet()) {
+ Revision revision = revisionEntry.getKey();
+ String revisionValue = revisionEntry.getValue();
+
+ // Only check committed revisions
+ if (Utils.isCommitted(revisionValue)) {
+ // Candidate to clean up
+ revisionCleaner.addCandidateRevisionToClean(revision);
+ // Store properties usage
+ classifyPropertiesModifiedByRevision(revision);
+ }
+ }
+ }
+
+ private void classifyPropertiesModifiedByRevision(Revision revision) {
+ for (Map.Entry<String, Object> propertyEntry : workingDocument.entrySet()) {
+ if (Utils.isPropertyName(propertyEntry.getKey()) || NodeDocument.isDeletedEntry(propertyEntry.getKey())) {
+ Map<Revision, String> valueMap = (Map) propertyEntry.getValue();
+ if (valueMap.containsKey(revision)) {
+ propertiesModifiedByRevision.computeIfAbsent(revision, key ->
+ new TreeSet<>()).add(propertyEntry.getKey()
+ );
+
+ 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);
+ }
+ }
+ }
+ }
+ }
+
+ protected class RevisionCleanerUtility {
Review Comment:
Left over from initial testing. I have made it private.
##########
oak-store-document/src/test/java/org/apache/jackrabbit/oak/plugins/document/NodeDocumentRevisionCleanerTest.java:
##########
@@ -0,0 +1,549 @@
+package org.apache.jackrabbit.oak.plugins.document;
+
+import com.fasterxml.jackson.core.type.TypeReference;
+import com.fasterxml.jackson.databind.ObjectMapper;
+import org.junit.Before;
+import org.junit.Test;
+import org.mockito.Mock;
+import org.mockito.Mockito;
+import org.mockito.MockitoAnnotations;
+
+import java.io.IOException;
+import java.time.Instant;
+import java.time.temporal.ChronoUnit;
+import java.util.ArrayList;
+import java.util.Collections;
+import java.util.List;
+import java.util.Map;
+import java.util.Set;
+import java.util.SortedMap;
+import java.util.TreeMap;
+import java.util.TreeSet;
+import java.util.stream.Collectors;
+import java.util.stream.Stream;
+
+import static org.apache.jackrabbit.oak.plugins.document.Collection.NODES;
+import static org.junit.Assert.assertEquals;
+import static org.junit.Assert.assertFalse;
+import static org.junit.Assert.assertNull;
+import static org.junit.Assert.assertTrue;
+
+public class NodeDocumentRevisionCleanerTest {
+
+ @Mock
+ DocumentStore documentStore;
+
+ @Mock
+ DocumentNodeStore documentNodeStore;
+
+ @Mock
+ Checkpoints checkpoints;
+
+ NodeDocument workingDocument;
+
+ NodeDocumentRevisionCleaner nodeDocumentRevisionCleaner;
+
+ @Before
+ public void setUp() {
+ MockitoAnnotations.initMocks(this);
Review Comment:
Done
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@jackrabbit.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] OAK-10535: Clean up old revisions in a document [jackrabbit-oak]
Posted by "Joscorbe (via GitHub)" <gi...@apache.org>.
Joscorbe commented on code in PR #1328:
URL: https://github.com/apache/jackrabbit-oak/pull/1328#discussion_r1511258728
##########
oak-run/src/main/java/org/apache/jackrabbit/oak/run/RevisionsCommand.java:
##########
@@ -82,6 +97,7 @@ public class RevisionsCommand implements Command {
" collect perform garbage collection",
" reset clear all persisted metadata",
" sweep clean up uncommitted changes",
+ " cleanup clean up old/unused revisions",
Review Comment:
I agree we should at some point combine some of the commands into the same one.
For now, I have changed this to `pathCleanup`. But I think a better name can be found.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@jackrabbit.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] OAK-10535: Clean up old revisions in a document [jackrabbit-oak]
Posted by "stefan-egli (via GitHub)" <gi...@apache.org>.
stefan-egli commented on code in PR #1328:
URL: https://github.com/apache/jackrabbit-oak/pull/1328#discussion_r1504280658
##########
oak-run/src/main/groovy/org/apache/jackrabbit/oak/console/GroovyConsole.groovy:
##########
@@ -124,6 +124,7 @@ class GroovyConsole {
new CheckpointCommand(shell),
new LsCommand(shell),
new PnCommand(shell),
+ new RevisionCommand(shell),
Review Comment:
You might already be aware of this, but just to be sure : do we need a new RevisionCommand when we have the RevisionsCommand?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@jackrabbit.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] OAK-10535: Clean up old revisions in a document [jackrabbit-oak]
Posted by "stefan-egli (via GitHub)" <gi...@apache.org>.
stefan-egli commented on code in PR #1328:
URL: https://github.com/apache/jackrabbit-oak/pull/1328#discussion_r1504290603
##########
oak-store-document/src/main/java/org/apache/jackrabbit/oak/plugins/document/VersionGarbageCollector.java:
##########
@@ -1053,6 +1053,28 @@ private void collectUnmergedBranchCommits(final NodeDocument doc, final GCPhases
phases.stop(GCPhase.DETAILED_GC_COLLECT_UNMERGED_BC);
}
+ private void collectOldRevisions(final NodeDocument doc, final GCPhases phases, final UpdateOp updateOp,
+ final long toModifiedMs) {
+ if (phases.start(GCPhase.DETAILED_GC_COLLECT_OLD_REVS)){
+ // Identify the revisions that can be removed
+ final Set<Revision> oldRevisions = doc.getLocalRevisions().keySet().stream()
+ .filter(rev -> isRevisionOlderThan(rev, toModifiedMs))
+ .collect(toSet());
+
+ if (oldRevisions.isEmpty()) {
+ phases.stop(GCPhase.DETAILED_GC_COLLECT_OLD_REVS);
+ return;
+ }
+
+
+
+
+
+
Review Comment:
```suggestion
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@jackrabbit.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] OAK-10535: Clean up old revisions in a document [jackrabbit-oak]
Posted by "Joscorbe (via GitHub)" <gi...@apache.org>.
Joscorbe commented on code in PR #1328:
URL: https://github.com/apache/jackrabbit-oak/pull/1328#discussion_r1511389602
##########
oak-store-document/src/main/java/org/apache/jackrabbit/oak/plugins/document/NodeDocumentRevisionCleaner.java:
##########
@@ -0,0 +1,270 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.jackrabbit.oak.plugins.document;
+
+import org.apache.jackrabbit.oak.plugins.document.util.Utils;
+
+import java.time.Instant;
+import java.time.temporal.ChronoUnit;
+import java.util.Map;
+import java.util.SortedMap;
+import java.util.TreeMap;
+import java.util.TreeSet;
+
+public class NodeDocumentRevisionCleaner {
+
+ private final DocumentStore documentStore;
+ private final DocumentNodeStore documentNodeStore;
+ private final NodeDocument workingDocument;
+ private final RevisionPropertiesClassifier revisionClassifier;
+ private final RevisionCleanerUtility revisionCleaner;
+
+ /**
+ * Constructor for NodeDocumentRevisionCleaner.
+ * @param documentStore The DocumentStore instance. Must be writable to perform cleanup.
+ * @param documentNodeStore The DocumentNodeStore instance.
+ * @param workingDocument The document to clean up.
+ */
+ public NodeDocumentRevisionCleaner(DocumentStore documentStore, DocumentNodeStore documentNodeStore,
+ NodeDocument workingDocument) {
+ this.workingDocument = workingDocument;
+ this.documentStore = documentStore;
+ this.documentNodeStore = documentNodeStore;
+
+ revisionClassifier = new RevisionPropertiesClassifier(workingDocument);
+ revisionCleaner = new RevisionCleanerUtility(revisionClassifier);
+ }
+
+ /**
+ * Collects cleanable old revisions for the given document.
+ */
+ public void collectOldRevisions(UpdateOp op) {
+ revisionClassifier.classifyRevisionsAndProperties();
+ revisionCleaner.preserveRevisionsNewerThanThreshold(24, ChronoUnit.HOURS);
+ revisionCleaner.preserveLastRevisionForEachProperty();
+ revisionCleaner.preserveRevisionsReferencedByCheckpoints();
+ revisionCleaner.removeCandidatesInList();
+
+ /*for (Map.Entry<Integer, TreeSet<Revision>> entry : revisionCleaner.candidateRevisionsToClean.entrySet()) {
+ for (Revision revision : entry.getValue()) {
+ System.out.println("Removing revision " + revision);
+ TreeSet<String> properties = revisionClassifier.propertiesModifiedByRevision.get(revision);
+ System.out.println("Properties modified by this revision: " + properties);
+ if (properties != null) {
+ for (String property : properties) {
+ op.removeMapEntry(property, revision);
+ }
+ }
+
+ op.removeMapEntry("_revisions", revision);
+ }
+ }*/
+ }
+
+ protected class RevisionPropertiesClassifier {
+ private final NodeDocument workingDocument;
+ private SortedMap<String, SortedMap<Integer, TreeSet<Revision>>> revisionsModifyingPropertyByCluster;
+ private SortedMap<String, TreeSet<Revision>> revisionsModifyingProperty;
+ private SortedMap<Revision, TreeSet<String>> propertiesModifiedByRevision;
+
+ private RevisionPropertiesClassifier(NodeDocument workingDocument) {
+ this.workingDocument = workingDocument;
+
+ this.revisionsModifyingPropertyByCluster = new TreeMap<>();
+ this.revisionsModifyingProperty = new TreeMap<>();
+ this.propertiesModifiedByRevision = new TreeMap<>(StableRevisionComparator.INSTANCE);
+ }
+
+ /**
+ * This method processes the revisions of the working document, creating maps to
+ * track the relationships between revisions and modified properties.
+ */
+ protected void classifyRevisionsAndProperties() {
+ SortedMap<Revision, String> deletedRevisions = workingDocument.getLocalDeleted();
+ // Always keep the first "_deleted" entry, as is when the document was created
+ if (!deletedRevisions.isEmpty()) {
+ Revision createdRevision = deletedRevisions.firstKey();
+ revisionCleaner.addBlockedRevisionToKeep(createdRevision);
+ }
+
+ SortedMap<Revision, String> documentRevisions = workingDocument.getLocalRevisions();
+ for (Map.Entry<Revision, String> revisionEntry : documentRevisions.entrySet()) {
+ Revision revision = revisionEntry.getKey();
+ String revisionValue = revisionEntry.getValue();
+
+ // Only check committed revisions
+ if (Utils.isCommitted(revisionValue)) {
+ // Candidate to clean up
+ revisionCleaner.addCandidateRevisionToClean(revision);
+ // Store properties usage
+ classifyPropertiesModifiedByRevision(revision);
+ }
+ }
+ }
+
+ private void classifyPropertiesModifiedByRevision(Revision revision) {
+ for (Map.Entry<String, Object> propertyEntry : workingDocument.entrySet()) {
+ if (Utils.isPropertyName(propertyEntry.getKey()) || NodeDocument.isDeletedEntry(propertyEntry.getKey())) {
+ Map<Revision, String> valueMap = (Map) propertyEntry.getValue();
+ if (valueMap.containsKey(revision)) {
+ propertiesModifiedByRevision.computeIfAbsent(revision, key ->
+ new TreeSet<>()).add(propertyEntry.getKey()
+ );
+
+ 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);
+ }
+ }
+ }
+ }
+ }
+
+ protected class RevisionCleanerUtility {
+
+ private final SortedMap<Integer, TreeSet<Revision>> blockedRevisionsToKeep;
+ private final SortedMap<Integer, TreeSet<Revision>> candidateRevisionsToClean;
+ private final RevisionPropertiesClassifier revisionClassifier;
Review Comment:
Done
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@jackrabbit.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] OAK-10535: Clean up old revisions in a document [jackrabbit-oak]
Posted by "Joscorbe (via GitHub)" <gi...@apache.org>.
Joscorbe commented on code in PR #1328:
URL: https://github.com/apache/jackrabbit-oak/pull/1328#discussion_r1511333001
##########
oak-store-document/src/test/java/org/apache/jackrabbit/oak/plugins/document/VersionGarbageCollectorIT.java:
##########
Review Comment:
I haven't done any changes in this file.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@jackrabbit.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] OAK-10535: Clean up old revisions in a document [jackrabbit-oak]
Posted by "Joscorbe (via GitHub)" <gi...@apache.org>.
Joscorbe commented on code in PR #1328:
URL: https://github.com/apache/jackrabbit-oak/pull/1328#discussion_r1515291771
##########
oak-run/src/main/java/org/apache/jackrabbit/oak/run/RevisionsCommand.java:
##########
@@ -74,15 +80,18 @@ public class RevisionsCommand implements Command {
private static final Logger LOG = LoggerFactory.getLogger(RevisionsCommand.class);
+ private static final int REVISION_CAP = getInteger("oak.revision.cap", 250);
Review Comment:
No, some leftover from old code.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@jackrabbit.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] OAK-10535: Clean up old revisions in a document [jackrabbit-oak]
Posted by "Joscorbe (via GitHub)" <gi...@apache.org>.
Joscorbe merged PR #1328:
URL: https://github.com/apache/jackrabbit-oak/pull/1328
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@jackrabbit.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] OAK-10535: Clean up old revisions in a document [jackrabbit-oak]
Posted by "stefan-egli (via GitHub)" <gi...@apache.org>.
stefan-egli commented on code in PR #1328:
URL: https://github.com/apache/jackrabbit-oak/pull/1328#discussion_r1504293588
##########
oak-store-document/src/main/java/org/apache/jackrabbit/oak/plugins/document/NodeDocumentRevisionCleaner.java:
##########
@@ -0,0 +1,270 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.jackrabbit.oak.plugins.document;
+
+import org.apache.jackrabbit.oak.plugins.document.util.Utils;
+
+import java.time.Instant;
+import java.time.temporal.ChronoUnit;
+import java.util.Map;
+import java.util.SortedMap;
+import java.util.TreeMap;
+import java.util.TreeSet;
+
+public class NodeDocumentRevisionCleaner {
Review Comment:
Some javadoc to describe what this class is about would be good to have.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@jackrabbit.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] OAK-10535: Clean up old revisions in a document [jackrabbit-oak]
Posted by "stefan-egli (via GitHub)" <gi...@apache.org>.
stefan-egli commented on code in PR #1328:
URL: https://github.com/apache/jackrabbit-oak/pull/1328#discussion_r1504314717
##########
oak-store-document/src/main/java/org/apache/jackrabbit/oak/plugins/document/NodeDocumentRevisionCleaner.java:
##########
@@ -0,0 +1,270 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.jackrabbit.oak.plugins.document;
+
+import org.apache.jackrabbit.oak.plugins.document.util.Utils;
+
+import java.time.Instant;
+import java.time.temporal.ChronoUnit;
+import java.util.Map;
+import java.util.SortedMap;
+import java.util.TreeMap;
+import java.util.TreeSet;
+
+public class NodeDocumentRevisionCleaner {
+
+ private final DocumentStore documentStore;
+ private final DocumentNodeStore documentNodeStore;
+ private final NodeDocument workingDocument;
+ private final RevisionPropertiesClassifier revisionClassifier;
+ private final RevisionCleanerUtility revisionCleaner;
+
+ /**
+ * Constructor for NodeDocumentRevisionCleaner.
+ * @param documentStore The DocumentStore instance. Must be writable to perform cleanup.
+ * @param documentNodeStore The DocumentNodeStore instance.
+ * @param workingDocument The document to clean up.
+ */
+ public NodeDocumentRevisionCleaner(DocumentStore documentStore, DocumentNodeStore documentNodeStore,
+ NodeDocument workingDocument) {
+ this.workingDocument = workingDocument;
+ this.documentStore = documentStore;
+ this.documentNodeStore = documentNodeStore;
+
+ revisionClassifier = new RevisionPropertiesClassifier(workingDocument);
+ revisionCleaner = new RevisionCleanerUtility(revisionClassifier);
+ }
+
+ /**
+ * Collects cleanable old revisions for the given document.
+ */
+ public void collectOldRevisions(UpdateOp op) {
+ revisionClassifier.classifyRevisionsAndProperties();
+ revisionCleaner.preserveRevisionsNewerThanThreshold(24, ChronoUnit.HOURS);
+ revisionCleaner.preserveLastRevisionForEachProperty();
+ revisionCleaner.preserveRevisionsReferencedByCheckpoints();
+ revisionCleaner.removeCandidatesInList();
+
+ /*for (Map.Entry<Integer, TreeSet<Revision>> entry : revisionCleaner.candidateRevisionsToClean.entrySet()) {
+ for (Revision revision : entry.getValue()) {
+ System.out.println("Removing revision " + revision);
+ TreeSet<String> properties = revisionClassifier.propertiesModifiedByRevision.get(revision);
+ System.out.println("Properties modified by this revision: " + properties);
+ if (properties != null) {
+ for (String property : properties) {
+ op.removeMapEntry(property, revision);
+ }
+ }
+
+ op.removeMapEntry("_revisions", revision);
+ }
+ }*/
+ }
+
+ protected class RevisionPropertiesClassifier {
Review Comment:
Is there a reason this is not `private` ?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@jackrabbit.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] OAK-10535: Clean up old revisions in a document [jackrabbit-oak]
Posted by "Joscorbe (via GitHub)" <gi...@apache.org>.
Joscorbe commented on code in PR #1328:
URL: https://github.com/apache/jackrabbit-oak/pull/1328#discussion_r1511344989
##########
oak-store-document/src/main/java/org/apache/jackrabbit/oak/plugins/document/NodeDocumentRevisionCleaner.java:
##########
@@ -0,0 +1,270 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.jackrabbit.oak.plugins.document;
+
+import org.apache.jackrabbit.oak.plugins.document.util.Utils;
+
+import java.time.Instant;
+import java.time.temporal.ChronoUnit;
+import java.util.Map;
+import java.util.SortedMap;
+import java.util.TreeMap;
+import java.util.TreeSet;
+
+public class NodeDocumentRevisionCleaner {
Review Comment:
Done. The class is a prototype, probably we will never use it in production, as deep cleaning in-between checkpointed revisions is probably too much.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@jackrabbit.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] OAK-10535: Clean up old revisions in a document [jackrabbit-oak]
Posted by "rishabhdaim (via GitHub)" <gi...@apache.org>.
rishabhdaim commented on PR #1328:
URL: https://github.com/apache/jackrabbit-oak/pull/1328#issuecomment-1976901217
Could you please revert `VersionGarbageCollectorIT` from your PR, it looks like you haven't made any changes in this file.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@jackrabbit.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] OAK-10535: Clean up old revisions in a document [jackrabbit-oak]
Posted by "Joscorbe (via GitHub)" <gi...@apache.org>.
Joscorbe commented on code in PR #1328:
URL: https://github.com/apache/jackrabbit-oak/pull/1328#discussion_r1515274804
##########
oak-run/src/main/java/org/apache/jackrabbit/oak/run/RevisionsCommand.java:
##########
@@ -74,15 +89,18 @@ public class RevisionsCommand implements Command {
private static final Logger LOG = LoggerFactory.getLogger(RevisionsCommand.class);
+ private static final int REVISION_CAP = getInteger("oak.revision.cap", 250);
+
private static final String USAGE = Joiner.on(System.lineSeparator()).join(
"revisions {<jdbc-uri> | <mongodb-uri>} <sub-command> [options]",
"where sub-command is one of",
" info give information about the revisions state without performing",
" any modifications",
- " collect perform garbage collection",
- " reset clear all persisted metadata",
- " sweep clean up uncommitted changes",
- " detailedGC perform detailed garbage collection i.e. remove unmerged branch commits, old revisions, deleted properties etc"
+ " collect perform garbage collection",
+ " reset clear all persisted metadata",
+ " sweep clean up uncommitted changes",
+ " detailedGC perform detailed garbage collection i.e. remove unmerged branch commits, old revisions, deleted properties etc",
+ " pathCleanup clean up old/unused revisions and unmerged branch commits on a specific path"
Review Comment:
That's possible, yes, and would be cleaner IMO. Since the `--path` is already a separated argument we can simply apply it to this command.
My only concern with this approach is, if the `path` argument is missing, it could end up running the DetailedGC in all the repository.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@jackrabbit.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] OAK-10535: Clean up old revisions in a document [jackrabbit-oak]
Posted by "stefan-egli (via GitHub)" <gi...@apache.org>.
stefan-egli commented on code in PR #1328:
URL: https://github.com/apache/jackrabbit-oak/pull/1328#discussion_r1512625559
##########
oak-store-document/src/main/java/org/apache/jackrabbit/oak/plugins/document/VersionGarbageCollector.java:
##########
@@ -1238,12 +1240,11 @@ private void removeUnmergedBCRevision(final Revision unmergedBCRevision, final N
}
private void collectOldRevisions(final NodeDocument doc, final GCPhases phases, final UpdateOp updateOp) {
Review Comment:
Would suggest to either rename to clarify that this is about old and between-checkpoint-but-not-younger-than-maxRevisionAgeMillis revisions - or to clarify that in a javadoc.
Now, given the old-revision-gc part competes with OAK-10688 we should execute only one of the two, not both. One idea could be to add an static boolean (not a system property, just a plain boolean) that lets you run DetailedGC in either between-checkpoint mode (OAK-10535) or keep-1-revision mode (OAK-10688). Then tests can control that flag at their discretion, we can compare the two and find bugs in the two, but ultimately decide on which one we want.
##########
oak-run/src/main/java/org/apache/jackrabbit/oak/run/RevisionsCommand.java:
##########
@@ -74,15 +80,18 @@ public class RevisionsCommand implements Command {
private static final Logger LOG = LoggerFactory.getLogger(RevisionsCommand.class);
+ private static final int REVISION_CAP = getInteger("oak.revision.cap", 250);
Review Comment:
(where) Is this still used?
##########
oak-run/src/main/java/org/apache/jackrabbit/oak/run/RevisionsCommand.java:
##########
@@ -460,6 +479,55 @@ private void sweep(RevisionsOptions options, Closer closer)
SweepHelper.sweep(store, new RevisionContextWrapper(ns, clusterId), seeker);
}
+ private void pathCleanup(RevisionsOptions options, Closer closer) throws IOException {
+ String path = options.getPath();
+ if (path == null || path.isEmpty()) {
+ System.err.println("path option is required for " + RevisionsOptions.CMD_CLEANUP + " command");
+ return;
+ }
+
+ DocumentNodeStoreBuilder<?> builder = createDocumentMKBuilder(options, closer);
+ if (builder == null) {
+ System.err.println("revisions mode only available for DocumentNodeStore");
+ return;
+ }
+
+ DocumentStore documentStore = builder.getDocumentStore();
+ builder.setReadOnlyMode();
+ useMemoryBlobStore(builder);
+ DocumentNodeStore documentNodeStore = builder.build();
+ String id = org.apache.jackrabbit.oak.plugins.document.util.Utils.getIdFromPath(path);
+ NodeDocument workingDocument = documentStore.find(NODES, id);
+ //NodeDocumentRevisionCleaner revisionCleaner = new NodeDocumentRevisionCleaner(documentNodeStore, workingDocument);
Review Comment:
Is the idea to keep this as a reminder for a future change (or could it be removed now)?
##########
oak-run/src/main/java/org/apache/jackrabbit/oak/run/RevisionsCommand.java:
##########
@@ -74,15 +89,18 @@ public class RevisionsCommand implements Command {
private static final Logger LOG = LoggerFactory.getLogger(RevisionsCommand.class);
+ private static final int REVISION_CAP = getInteger("oak.revision.cap", 250);
+
private static final String USAGE = Joiner.on(System.lineSeparator()).join(
"revisions {<jdbc-uri> | <mongodb-uri>} <sub-command> [options]",
"where sub-command is one of",
" info give information about the revisions state without performing",
" any modifications",
- " collect perform garbage collection",
- " reset clear all persisted metadata",
- " sweep clean up uncommitted changes",
- " detailedGC perform detailed garbage collection i.e. remove unmerged branch commits, old revisions, deleted properties etc"
+ " collect perform garbage collection",
+ " reset clear all persisted metadata",
+ " sweep clean up uncommitted changes",
+ " detailedGC perform detailed garbage collection i.e. remove unmerged branch commits, old revisions, deleted properties etc",
+ " pathCleanup clean up old/unused revisions and unmerged branch commits on a specific path"
Review Comment:
alternatively I was wondering if the restriction to a single path could not also be a further command line parameter of `detailedGC` mode? (in which case we wouldn't need a new mode just make `detailedGC` more fancy)
##########
oak-run/src/main/java/org/apache/jackrabbit/oak/run/RevisionsCommand.java:
##########
@@ -74,15 +80,18 @@ public class RevisionsCommand implements Command {
private static final Logger LOG = LoggerFactory.getLogger(RevisionsCommand.class);
+ private static final int REVISION_CAP = getInteger("oak.revision.cap", 250);
+
private static final String USAGE = Joiner.on(System.lineSeparator()).join(
"revisions {<jdbc-uri> | <mongodb-uri>} <sub-command> [options]",
"where sub-command is one of",
" info give information about the revisions state without performing",
Review Comment:
```suggestion
" info give information about the revisions state without performing",
```
(minor: indentation)
##########
oak-store-document/src/main/java/org/apache/jackrabbit/oak/plugins/document/VersionGarbageCollector.java:
##########
@@ -1398,6 +1399,24 @@ private void delayOnModifications(final long durationMs, final AtomicBoolean can
}
}
+ public UpdateOp collectGarbageOnDocument(DocumentNodeStore store, NodeDocument doc) {
Review Comment:
As an alternative idea to the 2 steps suggested (`collectGarbageOnDocument` then removing the garbage in `RevisionsCommand`) which @rishabhdaim also commented on : what about focusing on executing the [core part](https://github.com/apache/jackrabbit-oak/blob/cf870b532b107b0c243e1d0180f1213d3f6e9197/oak-store-document/src/main/java/org/apache/jackrabbit/oak/plugins/document/VersionGarbageCollector.java#L736-L782) of `collectDetailedGarbage` in 2 different modes : the current full one, and a new `onPath` one. Eg that would imply refactoring the existing `collectDetailedGarbage`'s inner part out and then more or less directly invoking that on a particular path (perhaps have a small facade method that facilitates that).
The problem I think is indeed, as @rishabhdaim [already mentioned](https://github.com/apache/jackrabbit-oak/pull/1328/files#r1511381906), that some parts might be missed, if we have a second variant of `removeGarbage` (for example we wouldn't do the embedded verification). Of course doing it "the real way" does require some refactoring, but I think it could be done via that inner loop mentioned. Wdyt?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@jackrabbit.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] OAK-10535: Clean up old revisions in a document [jackrabbit-oak]
Posted by "Joscorbe (via GitHub)" <gi...@apache.org>.
Joscorbe commented on code in PR #1328:
URL: https://github.com/apache/jackrabbit-oak/pull/1328#discussion_r1515306093
##########
oak-run/src/main/java/org/apache/jackrabbit/oak/run/RevisionsCommand.java:
##########
@@ -460,6 +479,55 @@ private void sweep(RevisionsOptions options, Closer closer)
SweepHelper.sweep(store, new RevisionContextWrapper(ns, clusterId), seeker);
}
+ private void pathCleanup(RevisionsOptions options, Closer closer) throws IOException {
+ String path = options.getPath();
+ if (path == null || path.isEmpty()) {
+ System.err.println("path option is required for " + RevisionsOptions.CMD_CLEANUP + " command");
+ return;
+ }
+
+ DocumentNodeStoreBuilder<?> builder = createDocumentMKBuilder(options, closer);
+ if (builder == null) {
+ System.err.println("revisions mode only available for DocumentNodeStore");
+ return;
+ }
+
+ DocumentStore documentStore = builder.getDocumentStore();
+ builder.setReadOnlyMode();
+ useMemoryBlobStore(builder);
+ DocumentNodeStore documentNodeStore = builder.build();
+ String id = org.apache.jackrabbit.oak.plugins.document.util.Utils.getIdFromPath(path);
+ NodeDocument workingDocument = documentStore.find(NODES, id);
+ //NodeDocumentRevisionCleaner revisionCleaner = new NodeDocumentRevisionCleaner(documentNodeStore, workingDocument);
Review Comment:
No, it's removed now.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@jackrabbit.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] OAK-10535: Clean up old revisions in a document [jackrabbit-oak]
Posted by "stefan-egli (via GitHub)" <gi...@apache.org>.
stefan-egli commented on code in PR #1328:
URL: https://github.com/apache/jackrabbit-oak/pull/1328#discussion_r1515880419
##########
oak-run/src/main/java/org/apache/jackrabbit/oak/run/RevisionsCommand.java:
##########
@@ -74,15 +89,18 @@ public class RevisionsCommand implements Command {
private static final Logger LOG = LoggerFactory.getLogger(RevisionsCommand.class);
+ private static final int REVISION_CAP = getInteger("oak.revision.cap", 250);
+
private static final String USAGE = Joiner.on(System.lineSeparator()).join(
"revisions {<jdbc-uri> | <mongodb-uri>} <sub-command> [options]",
"where sub-command is one of",
" info give information about the revisions state without performing",
" any modifications",
- " collect perform garbage collection",
- " reset clear all persisted metadata",
- " sweep clean up uncommitted changes",
- " detailedGC perform detailed garbage collection i.e. remove unmerged branch commits, old revisions, deleted properties etc"
+ " collect perform garbage collection",
+ " reset clear all persisted metadata",
+ " sweep clean up uncommitted changes",
+ " detailedGC perform detailed garbage collection i.e. remove unmerged branch commits, old revisions, deleted properties etc",
+ " pathCleanup clean up old/unused revisions and unmerged branch commits on a specific path"
Review Comment:
In theory if someone runs the command they should be aware of what it does - it's an admin command after all.
I guess we're doing it this careful because the new code isn't tested well enough yet.
Could we then - during this testing phase - demand that you either specify `--path` or `--entirerepo` (or something like this)? Then eventually make `--entirerepo` the default and not demand it anymore?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@jackrabbit.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] OAK-10535: Clean up old revisions in a document [jackrabbit-oak]
Posted by "Joscorbe (via GitHub)" <gi...@apache.org>.
Joscorbe commented on code in PR #1328:
URL: https://github.com/apache/jackrabbit-oak/pull/1328#discussion_r1520029980
##########
oak-run/src/main/java/org/apache/jackrabbit/oak/run/RevisionsCommand.java:
##########
@@ -74,15 +89,18 @@ public class RevisionsCommand implements Command {
private static final Logger LOG = LoggerFactory.getLogger(RevisionsCommand.class);
+ private static final int REVISION_CAP = getInteger("oak.revision.cap", 250);
+
private static final String USAGE = Joiner.on(System.lineSeparator()).join(
"revisions {<jdbc-uri> | <mongodb-uri>} <sub-command> [options]",
"where sub-command is one of",
" info give information about the revisions state without performing",
" any modifications",
- " collect perform garbage collection",
- " reset clear all persisted metadata",
- " sweep clean up uncommitted changes",
- " detailedGC perform detailed garbage collection i.e. remove unmerged branch commits, old revisions, deleted properties etc"
+ " collect perform garbage collection",
+ " reset clear all persisted metadata",
+ " sweep clean up uncommitted changes",
+ " detailedGC perform detailed garbage collection i.e. remove unmerged branch commits, old revisions, deleted properties etc",
+ " pathCleanup clean up old/unused revisions and unmerged branch commits on a specific path"
Review Comment:
That makes me a little less nervous. I have added that option for now.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@jackrabbit.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] OAK-10535: Clean up old revisions in a document [jackrabbit-oak]
Posted by "Joscorbe (via GitHub)" <gi...@apache.org>.
Joscorbe commented on code in PR #1328:
URL: https://github.com/apache/jackrabbit-oak/pull/1328#discussion_r1504533722
##########
oak-run/src/main/groovy/org/apache/jackrabbit/oak/console/GroovyConsole.groovy:
##########
@@ -124,6 +124,7 @@ class GroovyConsole {
new CheckpointCommand(shell),
new LsCommand(shell),
new PnCommand(shell),
+ new RevisionCommand(shell),
Review Comment:
The intention of this was to be able to run from the interactive console. We would probably don't this anymore, since that console is read-only anyway.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@jackrabbit.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] OAK-10535: Clean up old revisions in a document [jackrabbit-oak]
Posted by "Joscorbe (via GitHub)" <gi...@apache.org>.
Joscorbe commented on code in PR #1328:
URL: https://github.com/apache/jackrabbit-oak/pull/1328#discussion_r1514423969
##########
oak-store-document/src/main/java/org/apache/jackrabbit/oak/plugins/document/VersionGarbageCollector.java:
##########
@@ -1398,6 +1399,24 @@ private void delayOnModifications(final long durationMs, final AtomicBoolean can
}
}
+ public UpdateOp collectGarbageOnDocument(DocumentNodeStore store, NodeDocument doc) {
Review Comment:
I'm working on this approach, will commit the changes soon.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@jackrabbit.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] OAK-10535: Clean up old revisions in a document [jackrabbit-oak]
Posted by "Joscorbe (via GitHub)" <gi...@apache.org>.
Joscorbe commented on code in PR #1328:
URL: https://github.com/apache/jackrabbit-oak/pull/1328#discussion_r1514429813
##########
oak-run/src/main/java/org/apache/jackrabbit/oak/run/RevisionsCommand.java:
##########
@@ -460,6 +479,55 @@ private void sweep(RevisionsOptions options, Closer closer)
SweepHelper.sweep(store, new RevisionContextWrapper(ns, clusterId), seeker);
}
+ private void pathCleanup(RevisionsOptions options, Closer closer) throws IOException {
+ String path = options.getPath();
+ if (path == null || path.isEmpty()) {
+ System.err.println("path option is required for " + RevisionsOptions.CMD_CLEANUP + " command");
+ return;
+ }
+
+ DocumentNodeStoreBuilder<?> builder = createDocumentMKBuilder(options, closer);
+ if (builder == null) {
+ System.err.println("revisions mode only available for DocumentNodeStore");
+ return;
+ }
+
+ DocumentStore documentStore = builder.getDocumentStore();
+ builder.setReadOnlyMode();
+ useMemoryBlobStore(builder);
+ DocumentNodeStore documentNodeStore = builder.build();
+ String id = org.apache.jackrabbit.oak.plugins.document.util.Utils.getIdFromPath(path);
+ NodeDocument workingDocument = documentStore.find(NODES, id);
+ //NodeDocumentRevisionCleaner revisionCleaner = new NodeDocumentRevisionCleaner(documentNodeStore, workingDocument);
+
+ VersionGarbageCollector gc = bootstrapVGC(options, closer, true);
+ // Set a LoggingGC monitor that will output the detailedGC operations
+ gc.setGCMonitor(new LoggingGCMonitor(LOG));
+ // Run detailedGC on the given document
+ UpdateOp op = gc.collectGarbageOnDocument(documentNodeStore, workingDocument);
Review Comment:
Working on this, but I'm refactoring the `collectDetailedGarbage` to run all the logic on a single document. That way I think we will also improve the readability of that code.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@jackrabbit.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] OAK-10535: Clean up old revisions in a document [jackrabbit-oak]
Posted by "Joscorbe (via GitHub)" <gi...@apache.org>.
Joscorbe commented on code in PR #1328:
URL: https://github.com/apache/jackrabbit-oak/pull/1328#discussion_r1511417455
##########
oak-run/src/main/java/org/apache/jackrabbit/oak/run/RevisionsCommand.java:
##########
@@ -23,6 +23,11 @@
import java.io.IOException;
import java.util.List;
import java.util.Locale;
+import java.util.Map;
+import java.util.Scanner;
+import java.util.SortedMap;
+import java.util.TreeMap;
+import java.util.TreeSet;
Review Comment:
Done
##########
oak-run/src/main/java/org/apache/jackrabbit/oak/run/RevisionsCommand.java:
##########
@@ -32,6 +37,7 @@
import java.util.concurrent.TimeUnit;
import java.util.concurrent.TimeoutException;
import java.util.concurrent.atomic.AtomicBoolean;
+import java.util.stream.Collectors;
Review Comment:
Done
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@jackrabbit.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] OAK-10535: Clean up old revisions in a document [jackrabbit-oak]
Posted by "stefan-egli (via GitHub)" <gi...@apache.org>.
stefan-egli commented on code in PR #1328:
URL: https://github.com/apache/jackrabbit-oak/pull/1328#discussion_r1504315202
##########
oak-store-document/src/main/java/org/apache/jackrabbit/oak/plugins/document/NodeDocumentRevisionCleaner.java:
##########
@@ -0,0 +1,270 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.jackrabbit.oak.plugins.document;
+
+import org.apache.jackrabbit.oak.plugins.document.util.Utils;
+
+import java.time.Instant;
+import java.time.temporal.ChronoUnit;
+import java.util.Map;
+import java.util.SortedMap;
+import java.util.TreeMap;
+import java.util.TreeSet;
+
+public class NodeDocumentRevisionCleaner {
+
+ private final DocumentStore documentStore;
+ private final DocumentNodeStore documentNodeStore;
+ private final NodeDocument workingDocument;
+ private final RevisionPropertiesClassifier revisionClassifier;
+ private final RevisionCleanerUtility revisionCleaner;
+
+ /**
+ * Constructor for NodeDocumentRevisionCleaner.
+ * @param documentStore The DocumentStore instance. Must be writable to perform cleanup.
+ * @param documentNodeStore The DocumentNodeStore instance.
+ * @param workingDocument The document to clean up.
+ */
+ public NodeDocumentRevisionCleaner(DocumentStore documentStore, DocumentNodeStore documentNodeStore,
+ NodeDocument workingDocument) {
+ this.workingDocument = workingDocument;
+ this.documentStore = documentStore;
+ this.documentNodeStore = documentNodeStore;
+
+ revisionClassifier = new RevisionPropertiesClassifier(workingDocument);
+ revisionCleaner = new RevisionCleanerUtility(revisionClassifier);
+ }
+
+ /**
+ * Collects cleanable old revisions for the given document.
+ */
+ public void collectOldRevisions(UpdateOp op) {
+ revisionClassifier.classifyRevisionsAndProperties();
+ revisionCleaner.preserveRevisionsNewerThanThreshold(24, ChronoUnit.HOURS);
+ revisionCleaner.preserveLastRevisionForEachProperty();
+ revisionCleaner.preserveRevisionsReferencedByCheckpoints();
+ revisionCleaner.removeCandidatesInList();
+
+ /*for (Map.Entry<Integer, TreeSet<Revision>> entry : revisionCleaner.candidateRevisionsToClean.entrySet()) {
+ for (Revision revision : entry.getValue()) {
+ System.out.println("Removing revision " + revision);
+ TreeSet<String> properties = revisionClassifier.propertiesModifiedByRevision.get(revision);
+ System.out.println("Properties modified by this revision: " + properties);
+ if (properties != null) {
+ for (String property : properties) {
+ op.removeMapEntry(property, revision);
+ }
+ }
+
+ op.removeMapEntry("_revisions", revision);
+ }
+ }*/
+ }
+
+ protected class RevisionPropertiesClassifier {
+ private final NodeDocument workingDocument;
+ private SortedMap<String, SortedMap<Integer, TreeSet<Revision>>> revisionsModifyingPropertyByCluster;
+ private SortedMap<String, TreeSet<Revision>> revisionsModifyingProperty;
+ private SortedMap<Revision, TreeSet<String>> propertiesModifiedByRevision;
+
+ private RevisionPropertiesClassifier(NodeDocument workingDocument) {
+ this.workingDocument = workingDocument;
+
+ this.revisionsModifyingPropertyByCluster = new TreeMap<>();
+ this.revisionsModifyingProperty = new TreeMap<>();
+ this.propertiesModifiedByRevision = new TreeMap<>(StableRevisionComparator.INSTANCE);
+ }
+
+ /**
+ * This method processes the revisions of the working document, creating maps to
+ * track the relationships between revisions and modified properties.
+ */
+ protected void classifyRevisionsAndProperties() {
+ SortedMap<Revision, String> deletedRevisions = workingDocument.getLocalDeleted();
+ // Always keep the first "_deleted" entry, as is when the document was created
+ if (!deletedRevisions.isEmpty()) {
+ Revision createdRevision = deletedRevisions.firstKey();
+ revisionCleaner.addBlockedRevisionToKeep(createdRevision);
+ }
+
+ SortedMap<Revision, String> documentRevisions = workingDocument.getLocalRevisions();
+ for (Map.Entry<Revision, String> revisionEntry : documentRevisions.entrySet()) {
+ Revision revision = revisionEntry.getKey();
+ String revisionValue = revisionEntry.getValue();
+
+ // Only check committed revisions
+ if (Utils.isCommitted(revisionValue)) {
+ // Candidate to clean up
+ revisionCleaner.addCandidateRevisionToClean(revision);
+ // Store properties usage
+ classifyPropertiesModifiedByRevision(revision);
+ }
+ }
+ }
+
+ private void classifyPropertiesModifiedByRevision(Revision revision) {
+ for (Map.Entry<String, Object> propertyEntry : workingDocument.entrySet()) {
+ if (Utils.isPropertyName(propertyEntry.getKey()) || NodeDocument.isDeletedEntry(propertyEntry.getKey())) {
+ Map<Revision, String> valueMap = (Map) propertyEntry.getValue();
+ if (valueMap.containsKey(revision)) {
+ propertiesModifiedByRevision.computeIfAbsent(revision, key ->
+ new TreeSet<>()).add(propertyEntry.getKey()
+ );
+
+ 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);
+ }
+ }
+ }
+ }
+ }
+
+ protected class RevisionCleanerUtility {
Review Comment:
Is there a reason this is not `private` ?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@jackrabbit.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] OAK-10535: Clean up old revisions in a document [jackrabbit-oak]
Posted by "stefan-egli (via GitHub)" <gi...@apache.org>.
stefan-egli commented on PR #1328:
URL: https://github.com/apache/jackrabbit-oak/pull/1328#issuecomment-1966670655
It's generally speaking a bit difficult to review as the code doesn't compile at this point (therefore eg you can't debug through it, to understand it better). As I understand this is soon going to be fixed - so I'll wait with reviewing actual functionality for now.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@jackrabbit.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] OAK-10535: Clean up old revisions in a document [jackrabbit-oak]
Posted by "stefan-egli (via GitHub)" <gi...@apache.org>.
stefan-egli commented on code in PR #1328:
URL: https://github.com/apache/jackrabbit-oak/pull/1328#discussion_r1504288687
##########
oak-store-document/src/main/java/org/apache/jackrabbit/oak/plugins/document/NodeDocumentRevisionCleaner.java:
##########
@@ -0,0 +1,270 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.jackrabbit.oak.plugins.document;
+
+import org.apache.jackrabbit.oak.plugins.document.util.Utils;
+
+import java.time.Instant;
+import java.time.temporal.ChronoUnit;
+import java.util.Map;
+import java.util.SortedMap;
+import java.util.TreeMap;
+import java.util.TreeSet;
+
+public class NodeDocumentRevisionCleaner {
+
+ private final DocumentStore documentStore;
+ private final DocumentNodeStore documentNodeStore;
+ private final NodeDocument workingDocument;
+ private final RevisionPropertiesClassifier revisionClassifier;
+ private final RevisionCleanerUtility revisionCleaner;
+
+ /**
+ * Constructor for NodeDocumentRevisionCleaner.
+ * @param documentStore The DocumentStore instance. Must be writable to perform cleanup.
+ * @param documentNodeStore The DocumentNodeStore instance.
+ * @param workingDocument The document to clean up.
+ */
+ public NodeDocumentRevisionCleaner(DocumentStore documentStore, DocumentNodeStore documentNodeStore,
Review Comment:
Is it required to have the DocumentStore passed here? The `DocumentNodeStore.getDocumentStore()` would provide it too - but that way it would be avoided that for some reason the two could not link to the same...
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@jackrabbit.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org