OAK-11444 [full-gc] Save document id and empty properties names before deletion #2038

daniancu · 2025-01-30T16:18:57Z

Introduce a FullGcBin class as a wrapper around DocumentStore that expose two methods used to clean garbage from NODES collection

public int remove(Map<String, Long> orphanOrDeletedRemovalMap)
public List<NodeDocument> findAndUpdate(List<UpdateOp> updateOpList)

When enabled
Each methods saves the document ID or empty properties names (that will be deleted) to a separate _bin collection as a BinDocument then delegates deletion to DocumentStore

When disabled (default)
Each method delegates directly to DocumentStore

This PR does not contain the necessary configuration parameter to turn on/off the fullGcBin. I'll create a separate PR for that (due the fact how config params are passed in, it will require additional 15 file changes and will make this PR hard to follow)

Update: PR that adds configuration for this feature is available here: daniancu#1

reschke

Hm, this adds a new Collection for the first time in more than a decade. I believe this needs more discussion, optimally not in the PR but in the ticket.

oak-store-document/src/main/java/org/apache/jackrabbit/oak/plugins/document/Collection.java

oak-store-document/src/main/java/org/apache/jackrabbit/oak/plugins/document/FullGcBin.java

oak-store-document/src/test/java/org/apache/jackrabbit/oak/plugins/document/FullGcBinTest.java

oak-store-document/src/main/java/org/apache/jackrabbit/oak/plugins/document/FullGcBin.java

oak-store-document/src/test/java/org/apache/jackrabbit/oak/plugins/document/FullGcBinTest.java

daniancu · 2025-02-14T12:26:57Z

I've addressed all issues mentioned and increased test coverage to 100%

shodaaan · 2025-02-17T10:19:08Z

oak-store-document/src/main/java/org/apache/jackrabbit/oak/plugins/document/FullGcBin.java

+import java.util.Map;
+import java.util.stream.Collectors;
+
+public class FullGcBin {


Please consider adding a short class description summarizing how and where this class is used, for example: "used in MongoDB garbage collection in order to first store the deleted document paths in MongoDB before doing the deletion".

shodaaan · 2025-02-17T10:19:45Z

oak-store-document/src/main/java/org/apache/jackrabbit/oak/plugins/document/FullGcBin.java

+        documentStore = ds;
+    }
+
+    public int remove(Map<String, Long> orphanOrDeletedRemovalMap) {


Please consider adding a short description (one line) for what the methods do.

shodaaan · 2025-02-17T10:24:47Z

oak-store-document/src/main/java/org/apache/jackrabbit/oak/plugins/document/FullGcBin.java

+        try {
+            return documentStore.create(Collection.SETTINGS, docs);
+        } catch (Exception e) {
+            LOG.error("Error while adding delete candidate documents to bin", e);


I suggest we also log the paths of the documents that failed to be added to the bin, they will be helpful for debug purposes.

shodaaan · 2025-02-17T10:25:28Z

oak-store-document/src/main/java/org/apache/jackrabbit/oak/plugins/document/FullGcBin.java

+            documentStore.createOrUpdate(Collection.SETTINGS, binOpList);
+            return true;
+        } catch (Exception e) {
+            LOG.error("Error while adding removed properties to bin", e);


I suggest we also log the paths of the properties that failed to be added to the bin, will be helpful for debugging.

shodaaan

Added suggestions for logging of paths and adding class and method descriptions.

shodaaan

The TTL index for the /bin documents should also be created automatically.
I suggest doing this in MongoDocumentStore.ensureIndexes method.

daniancu · 2025-02-17T12:28:35Z

The TTL index for the /bin documents should also be created automatically. I suggest doing this in MongoDocumentStore.ensureIndexes method.

what should be the TTL for this documents? 30 days? 60 days? ...

shodaaan · 2025-02-17T13:06:36Z

The TTL index for the /bin documents should also be created automatically. I suggest doing this in MongoDocumentStore.ensureIndexes method.

what should be the TTL for this documents? 30 days? 60 days? ...

I suggest we start with 90 days.

rishabhdaim · 2025-02-18T05:07:35Z

oak-run/src/main/java/org/apache/jackrabbit/oak/run/RevisionsCommand.java

@@ -191,10 +191,10 @@ private static class RevisionsOptions extends Utils.NodeStoreOptions {
            fullGcProgressSize = parser.accepts("fullGcProgressSize", "The number of documents to check for " +
                            "garbage in each Full GC cycle")
                    .withRequiredArg().ofType(Integer.class).defaultsTo(10000);
-            fullGcMaxAge = parser.accepts("fullGcMaxAge", "The maximum age of the document in seconds " +
+            fullGcMaxAgeSec = parser.accepts("fullGcMaxAge", "The maximum age of the document in seconds " +


I would keep both variable name & param name same.

Suggested change

fullGcMaxAgeSec = parser.accepts("fullGcMaxAge", "The maximum age of the document in seconds " +

fullGcMaxAge = parser.accepts("fullGcMaxAge", "The maximum age of the document in seconds " +

oak-run/src/main/java/org/apache/jackrabbit/oak/run/RevisionsCommand.java

shodaaan

Looks good to me, approved.

rishabhdaim · 2025-03-03T12:56:43Z

...ument/src/main/java/org/apache/jackrabbit/oak/plugins/document/mongo/MongoFullGcNodeBin.java

+ *  Each method delegates directly to DocumentStore
+ */
+public class MongoFullGcNodeBin implements FullGcNodeBin {
+    private static final Logger LOG = getLogger(MongoFullGcNodeBin.class);


please use Fully qualified class name.

I'm not sure what you mean @rishabhdaim ... why?

I'm not sure what you mean @rishabhdaim ... why?

me too

avoid static import of "getLogger" for consistency with the rest of Oak :-)

ah yes, LoggerFactory.getLogger instead of getLogger. Right!

rishabhdaim · 2025-03-03T13:04:06Z

...ument/src/main/java/org/apache/jackrabbit/oak/plugins/document/mongo/MongoFullGcNodeBin.java

+        return mongoDocumentStore.findAndUpdate(Collection.NODES, updateOpList);
+    }
+
+    protected boolean addToBin(Map<String, Long> orphanOrDeletedRemovalMap) {


please add the comments about what would be added in DB after this operation.

thomasmueller · 2025-03-03T13:04:27Z

...ument/src/main/java/org/apache/jackrabbit/oak/plugins/document/mongo/MongoFullGcNodeBin.java

+
+    public MongoFullGcNodeBin(MongoDocumentStore ds) {
+        mongoDocumentStore = ds;
+        enabled = System.getProperty("oak.document.fullGcBin.enabled", "false").equals("true");


We typically have a (public) constant and then use this here.

Also, Boolean.getBoolean(...)?

There's also:

https://jackrabbit.apache.org/oak/docs/apidocs/org/apache/jackrabbit/oak/commons/properties/SystemPropertySupplier.html

this part is gone in the configuration PR

I know. But the convenience comes with drawbacks. That's why we have a utility method (logging, value checking etc...). How many times have you wondered whether you really have set the correct property? And what the default was without checking the source code?

Here's an example use:

jackrabbit-oak/oak-core/src/main/java/org/apache/jackrabbit/oak/core/MutableRoot.java

Line 142 in 48b26b5

private static final boolean CLASSIC_MOVE =

Also using "System.getProperty" instead of "Boolean.getBoolean" makes things worse because you lose common parsing of boolean values...

understood. updated as suggested

rishabhdaim · 2025-03-03T13:04:49Z

...ument/src/main/java/org/apache/jackrabbit/oak/plugins/document/mongo/MongoFullGcNodeBin.java

+        return mongoDocumentStore.findAndUpdate(Collection.NODES, updateOpList);
+    }
+
+    protected boolean addToBin(Map<String, Long> orphanOrDeletedRemovalMap) {


Suggested change

protected boolean addToBin(Map<String, Long> orphanOrDeletedRemovalMap) {

private boolean addToBin(Map<String, Long> orphanOrDeletedRemovalMap) {

rishabhdaim · 2025-03-03T13:05:37Z

...ument/src/main/java/org/apache/jackrabbit/oak/plugins/document/mongo/MongoFullGcNodeBin.java

+        return false;
+    }
+
+    protected boolean persist(List<BasicDBObject> inserts) {


Suggested change

protected boolean persist(List<BasicDBObject> inserts) {

private boolean persist(List<BasicDBObject> inserts) {

rishabhdaim · 2025-03-03T13:05:48Z

...ument/src/main/java/org/apache/jackrabbit/oak/plugins/document/mongo/MongoFullGcNodeBin.java

+        return true;
+    }
+
+    BasicDBObject toBasicDBObject(UpdateOp op) {


Suggested change

BasicDBObject toBasicDBObject(UpdateOp op) {

private BasicDBObject toBasicDBObject(UpdateOp op) {

rishabhdaim · 2025-03-03T13:06:47Z

...nt/src/main/java/org/apache/jackrabbit/oak/plugins/document/mongo/MongoVersionGCSupport.java

@@ -481,6 +481,11 @@ private void logSplitDocIdsTobeDeleted(Bson query) {
        LOG.debug(sb.toString());
    }

+    @Override
+    public FullGcNodeBin getFullGCBin() {
+        return new MongoFullGcNodeBin(store);


This should be singleton.

rishabhdaim

Also, I think the place to adding getBinCollection() is wrong. We should have this in DocumentStore interface with default implementation of noBin and then override this for MongoDocumentStore only.

For reference we could use default Throttler throttler().

For test we could override this for MemoryDocumentStore too.

cc @Joscorbe @stefan-egli

rishabhdaim · 2025-03-03T13:18:31Z

...ument/src/main/java/org/apache/jackrabbit/oak/plugins/document/mongo/MongoFullGcNodeBin.java

+        return false;
+    }
+
+    private boolean addToBin(List<UpdateOp> updateOpList) {


Both addToBin method does exactly the same thing, just differ in input. We should extract out the common code to a common method.

we also have the conversion of input param to BasicDBObject which is different. The common part is extracted in the persist() method

daniancu · 2025-03-03T14:32:20Z

Also, I think the place to adding getBinCollection() is wrong. We should have this in DocumentStore interface with default implementation of noBin and then override this for MongoDocumentStore only.

For reference we could use default Throttler throttler().

For test we could override this for MemoryDocumentStore too.

cc @Joscorbe @stefan-egli

we discussed about adding a hidden collection in JIra
The idea was to avoid exposing it to other implementations because we only need it for Mongo one

Joscorbe

It seems all the comments have been addressed, but apparently there is an open question. If the implementation detail has to be challenged I propose to discuss in Jira ticket, so we have everything at one place.

daniancu · 2025-03-04T09:25:44Z

I have merged OAK-11525 containing the configuration part into this PR so we have everything in a single place

Joscorbe · 2025-03-04T12:53:55Z

I'm running the pipeline just to ensure it builds properly, and will merge afterwards. Thanks!

…re deletion

…llection * OAK-11525 Update fullGC configuration for audit logging into Mongo collection

reschke requested changes Jan 31, 2025

View reviewed changes

oak-store-document/src/main/java/org/apache/jackrabbit/oak/plugins/document/Collection.java Outdated Show resolved Hide resolved

shodaaan reviewed Jan 31, 2025

View reviewed changes

oak-store-document/src/main/java/org/apache/jackrabbit/oak/plugins/document/FullGcBin.java Outdated Show resolved Hide resolved

shodaaan reviewed Jan 31, 2025

View reviewed changes

oak-store-document/src/main/java/org/apache/jackrabbit/oak/plugins/document/FullGcBin.java Outdated Show resolved Hide resolved

shodaaan reviewed Jan 31, 2025

View reviewed changes

oak-store-document/src/main/java/org/apache/jackrabbit/oak/plugins/document/FullGcBin.java Outdated Show resolved Hide resolved

shodaaan reviewed Jan 31, 2025

View reviewed changes

oak-store-document/src/test/java/org/apache/jackrabbit/oak/plugins/document/FullGcBinTest.java Outdated Show resolved Hide resolved

daniancu force-pushed the OAK-11444 branch from 7ae4329 to 1098770 Compare January 31, 2025 14:31

thomasmueller requested changes Feb 12, 2025

View reviewed changes

daniancu force-pushed the OAK-11444 branch from 1098770 to 56ac43e Compare February 14, 2025 11:15

thomasmueller requested changes Feb 14, 2025

View reviewed changes

oak-store-document/src/test/java/org/apache/jackrabbit/oak/plugins/document/FullGcBinTest.java Outdated Show resolved Hide resolved

daniancu force-pushed the OAK-11444 branch from db41a33 to bc3ef9a Compare February 14, 2025 11:55

daniancu marked this pull request as ready for review February 14, 2025 12:26

thomasmueller self-requested a review February 14, 2025 13:38

daniancu requested review from shodaaan and reschke February 14, 2025 15:04

shodaaan reviewed Feb 17, 2025

View reviewed changes

shodaaan approved these changes Feb 17, 2025

View reviewed changes

shodaaan reviewed Feb 17, 2025

View reviewed changes

rishabhdaim reviewed Feb 18, 2025

View reviewed changes

daniancu requested review from rishabhdaim and shodaaan February 18, 2025 08:17

thomasmueller reviewed Feb 18, 2025

View reviewed changes

oak-run/src/main/java/org/apache/jackrabbit/oak/run/RevisionsCommand.java Outdated Show resolved Hide resolved

shodaaan approved these changes Feb 18, 2025

View reviewed changes

daniancu force-pushed the OAK-11444 branch from 6681a49 to 6ac2ee8 Compare February 24, 2025 13:14

Joscorbe approved these changes Feb 26, 2025

View reviewed changes

rishabhdaim reviewed Mar 3, 2025

View reviewed changes

thomasmueller self-requested a review March 3, 2025 13:01

rishabhdaim reviewed Mar 3, 2025

View reviewed changes

thomasmueller reviewed Mar 3, 2025

View reviewed changes

rishabhdaim reviewed Mar 3, 2025

View reviewed changes

thomasmueller self-requested a review March 3, 2025 13:11

rishabhdaim requested changes Mar 3, 2025

View reviewed changes

rishabhdaim reviewed Mar 3, 2025

View reviewed changes

Joscorbe mentioned this pull request Mar 4, 2025

OAK-11525 Update fullGC configuration for audit logging into Mongo collection daniancu/jackrabbit-oak#1

Merged

Joscorbe approved these changes Mar 4, 2025

View reviewed changes

rishabhdaim approved these changes Mar 4, 2025

View reviewed changes

Daniel Iancu and others added 12 commits March 7, 2025 11:47

OAK-11444: [full-gc] Save document id and empty properties names befo…

1432cfe

…re deletion

OAK-11444: [full-gc] Save document id and empty properties names befo…

a8a11b0

…re deletion

OAK-11444: [full-gc] Save document id and empty properties names befo…

cdc858b

…re deletion

OAK-11444: [full-gc] Save document id and empty properties names befo…

fc67c32

…re deletion

OAK-11444: [full-gc] Save document id and empty properties names befo…

0dfef1f

…re deletion

OAK-11444: [full-gc] Save document id and empty properties names befo…

14a72ff

…re deletion

OAK-11444: [full-gc] Save document id and empty properties names befo…

e9935aa

…re deletion

OAK-11444: [full-gc] Save document id and empty properties names befo…

d70d93e

…re deletion

OAK-11444: [full-gc] Save document id and empty properties names befo…

c349f5d

…re deletion

OAK-11444: [full-gc] Save document id and empty properties names befo…

be08faf

…re deletion

OAK-11444: [full-gc] Save document id and empty properties names befo…

e303df3

…re deletion

OAK-11525 Update fullGC configuration for audit logging into Mongo co…

17af78c

…llection * OAK-11525 Update fullGC configuration for audit logging into Mongo collection

daniancu force-pushed the OAK-11444 branch from 756e189 to 17af78c Compare March 7, 2025 10:03

	fullGcMaxAgeSec = parser.accepts("fullGcMaxAge", "The maximum age of the document in seconds " +
	fullGcMaxAge = parser.accepts("fullGcMaxAge", "The maximum age of the document in seconds " +

	protected boolean addToBin(Map<String, Long> orphanOrDeletedRemovalMap) {
	private boolean addToBin(Map<String, Long> orphanOrDeletedRemovalMap) {

	protected boolean persist(List<BasicDBObject> inserts) {
	private boolean persist(List<BasicDBObject> inserts) {

	BasicDBObject toBasicDBObject(UpdateOp op) {
	private BasicDBObject toBasicDBObject(UpdateOp op) {

OAK-11444 [full-gc] Save document id and empty properties names before deletion #2038

Are you sure you want to change the base?

OAK-11444 [full-gc] Save document id and empty properties names before deletion #2038

Conversation

daniancu commented Jan 30, 2025 • edited Loading

reschke left a comment

Choose a reason for hiding this comment

daniancu commented Feb 14, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shodaaan left a comment

Choose a reason for hiding this comment

shodaaan left a comment

Choose a reason for hiding this comment

daniancu commented Feb 17, 2025

shodaaan commented Feb 17, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shodaaan left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

daniancu Mar 3, 2025 • edited Loading

Choose a reason for hiding this comment

reschke Mar 3, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rishabhdaim left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

daniancu commented Mar 3, 2025

Joscorbe left a comment

Choose a reason for hiding this comment

daniancu commented Mar 4, 2025

Joscorbe commented Mar 4, 2025

daniancu commented Jan 30, 2025 •

edited

Loading

daniancu Mar 3, 2025 •

edited

Loading

reschke Mar 3, 2025 •

edited

Loading

rishabhdaim left a comment •

edited

Loading