OAK-12010 Simplified index management (without optimizer) by thomasmueller · Pull Request #2689 · apache/jackrabbit-oak

thomasmueller · 2026-01-19T09:31:45Z

This is a subset of the (much) larger draft PR #2652

This is just support for the "diff.index"; without the optimizer.

bhabegger

Nice clean work 👍

I like the test coverage and the comments among the rest !

I don't see anything blocking and just have a few comments/suggestions.

bhabegger · 2026-01-30T13:58:39Z

oak-core/src/main/java/org/apache/jackrabbit/oak/plugins/index/diff/DiffIndex.java

+            if (!diffContent.exists()) {
+                continue;
+            }
+            PropertyState lastMod = diffContent.getProperty("jcr:lastModified");


Is there not a constant definition for this type of "well-known" entries ? (same for :lastProcessed, jcr:data, /oak:index/, etc.)

In my opinion this helps a lot to understand, identify in one go what are the 'system' properties, paths, etc., and know where they are used. But also have them documented in the code base as javadoc.

(Not a blocker, just a curiosity 😉)

There probably is. I agree we are not consistently using these constants, so there is a risk of typos.

I used constants now where available, and did add new ones where I think it is useful (which -- granted -- is subjective).

oak-core/src/main/java/org/apache/jackrabbit/oak/plugins/index/diff/DiffIndex.java

bhabegger · 2026-01-30T14:01:32Z

oak-core/src/main/java/org/apache/jackrabbit/oak/plugins/index/diff/DiffIndex.java

+     * @param store            the node store
+     * @param indexDefinitions the /oak:index node
+     */
+    public static void applyDiffIndexChanges(NodeStore store, NodeBuilder indexDefinitions) {


To ease readability maybe we could split this method in :

parsing entries

diffing entries

and delegate the orchestration to a caller method ?

I now split it into two parts that I think make sense.

bhabegger · 2026-01-30T14:02:44Z

oak-core/src/main/java/org/apache/jackrabbit/oak/plugins/index/diff/DiffIndex.java

+        JsonObject repositoryDefinitions = RootIndexesListService.getRootIndexDefinitions(indexDefinitions);
+        LOG.debug("Index list {}", repositoryDefinitions.toString());
+        try {
+            DiffIndexMerger.instance().merge(newImageLuceneDefinitions, repositoryDefinitions, store);


merge also seams to do something with diff.index and diff.index.optimizer couldn't we call the mergeDiff directly instead (without a dependency to the NodeStore)?

We want to re-use the "merge" feature in another project (when processing diffs that are sent via script, so come from external sources). So it's better to keep DiffIndexMerger#merge

bhabegger · 2026-01-30T16:11:32Z

oak-core/src/main/java/org/apache/jackrabbit/oak/plugins/index/diff/DiffIndexMerger.java

+
+        // merge
+        JsonObject merged = null;
+        if (indexDiff == null) {


As an example, maybe this premerge null checking logic could be moved down into the processMerge itself ? (The "nothing to do" case could be handled by a null or Optional merged result)

There is some code executed in this case (latestCustomized == null...) so that would require changing processMerge, which I would avoid.

if (indexDiff == null) { // no diff definition: use to the OOTB index if (latestCustomized == null) { log("Only a product index found, nothing to do"); return false; } merged = latestProductIndex; } else { merged = processMerge(latestProductIndex, indexDiff); }

Could be rewritten:

if(indexDiff == null && latestCustomized == null) { log("Only a product index found, nothing to do"); return false; } merged = processMerge(latestProductIndex, indexDiff);

with processMerge returning latestProductIndex if indexDiff is null.

we could even let it return null if both are null and exit fast if merged is null with the log.

Well it would move part of the logic to processMerge, which I would like to avoid. Also, the comment is lost.

Ok, that's fine then.

bhabegger · 2026-01-30T16:14:43Z

oak-core/src/main/java/org/apache/jackrabbit/oak/plugins/index/diff/JsonNodeBuilder.java

+ *
+ * "null" entries are not supported.
+ */
+public class JsonNodeBuilder {


The Builder in the name could be misleading as, unless I missed something. this class doesn't seem to follow the "Builder" pattern, and seams more like a Helper class (but then there are different style tastes out their).

Right... It's a converter from "Json" to "NodeBuilder". It's confusing.

What about JsonNodeWriter, or JsonToNodeConverter?

I would prefer Converter among the two as Writer is quite connotated (PrintWriter, StringWriter, etc.). However, both Writer and Converter still suggests some sort of instantiation and don't highlight that this class is a utility class.

JsonNodeConversionUtil ?
JsonNodeConversionHelper ?

Hm, it's not really a converter... the nodes are persisted in Oak, and "converter" (for me) would imply that the nodes are just converted and the result is returned.

It's mostly about creating, and possibly updating and deleting, nodes... So what about:

JsonNodeSmith (recommended by a tool; sounds right to me)

JsonNodeCreator (because it's mostly about creating new nodes... but not purely)

JsonNodeWriter, for the case it's updating / deleting (currently, I think it's not, but possibly in the future)

Ok, then. Writer could pass, because it does I/O even though it's not in the usual sense. JsonNodeSaver ? JsonNodeStorer ? JsonNodeManager ? JsonNodeUpdater ?

I'm not really convinced by any... But maybe it triggers need ideas ? Otherwise just go with your preference.

OK so I use JsonNodeUpdater.

bhabegger · 2026-01-30T16:20:47Z

oak-core/src/main/java/org/apache/jackrabbit/oak/plugins/index/diff/RootIndexesListService.java

+import java.util.ArrayList;
+
+import org.apache.felix.inventory.Format;
+import org.apache.jackrabbit.oak.commons.json.JsonObject;


I'm guessing that OAK has it's own JSON processing library for historical reasons ?

bhabegger · 2026-01-30T16:22:59Z

oak-core/src/test/java/org/apache/jackrabbit/oak/plugins/index/diff/DiffIndexTest.java

+                + "    } }");
+
+        repositoryDefinitions = RootIndexesListService.getRootIndexDefinitions(store, "lucene");
+        assertSameJson("{\n"


It's a shame Java 11 is a bit too early for text blocks :)

I agree (btw. text blocks are not "raw" and still need escaping).

But I think it's readable as is.

bhabegger · 2026-01-30T16:26:22Z

oak-core/src/test/java/org/apache/jackrabbit/oak/plugins/index/diff/MergeTest.java

+    public void createDummy() {
+        // when enabling "deleteCreatesDummyIndex", then a dummy index is created
+        // (that indexes /dummy, which doesn't exist)
+        String merged = new DiffIndexMerger(new String[0], true, true, false).processMerge(JsonObject.fromJson("{}"


Which true is it ?

Suggested change

String merged = new DiffIndexMerger(new String[0], true, true, false).processMerge(JsonObject.fromJson("{}"

boolean deleteCreatesDummyIndex = true;

String merged = new DiffIndexMerger(new String[0], deleteCreatesDummyIndex, true, false).processMerge(JsonObject.fromJson("{}"

Would make it explicit

Hm, "list of boolean flags" is not user friendly... thinking about an alternative.

I added setters (there no real harm to not have the fields final in this case). Sure we could also add a Builder. Or a Factory, or AdapterFactoryBuilder... :-)

…/diff/DiffIndex.java Co-authored-by: Benjamin Habegger <benjamin@habegger.tech>

amit-jain

Overall looks good to me!!
A few minor suggestions added.

oak-core/src/main/java/org/apache/jackrabbit/oak/plugins/index/diff/DiffIndex.java

oak-core/src/main/java/org/apache/jackrabbit/oak/plugins/index/IndexName.java

oak-core/src/main/java/org/apache/jackrabbit/oak/plugins/index/diff/DiffIndexMerger.java

oak-core/src/main/java/org/apache/jackrabbit/oak/plugins/index/diff/DiffIndex.java

amit-jain · 2026-02-03T05:23:54Z

oak-core/src/main/java/org/apache/jackrabbit/oak/plugins/index/diff/DiffIndexMerger.java

+     *                                  (input)
+     * @return whether a new version of an index was added
+     */
+    public boolean processMerge(String indexName, JsonObject indexDiff, JsonObject newImageLuceneDefinitions, JsonObject combined) {


yes recommend breaking this method, example (from comments)

findLatestVersions

performMerge

isChanged

addMetadata

oak-core/src/main/java/org/apache/jackrabbit/oak/plugins/index/diff/DiffIndex.java

oak-core/src/main/java/org/apache/jackrabbit/oak/plugins/index/diff/JsonNodeBuilder.java

…/diff/DiffIndex.java Co-authored-by: Amit Jain <amitj@apache.org>

…/diff/DiffIndexMerger.java Co-authored-by: Amit Jain <amitj@apache.org>

…/diff/DiffIndex.java Co-authored-by: Amit Jain <amitj@apache.org>

…/diff/JsonNodeBuilder.java Co-authored-by: Benjamin Habegger <benjamin@habegger.tech>

…/diff/DiffIndex.java Co-authored-by: Amit Jain <amitj@apache.org>

sonarqubecloud · 2026-02-06T17:16:17Z

Quality Gate passed

Issues
57 New issues
0 Accepted issues

Measures
0 Security Hotspots
81.3% Coverage on New Code
1.9% Duplication on New Code

See analysis details on SonarQube Cloud

thomasmueller added 4 commits January 19, 2026 10:29

OAK-12010 Simplified index management (without optimizer)

a91de68

OAK-12010 Simplified index management

42d1e36

OAK-12010 Simplified index management

041af37

OAK-12010 Simplified index management

e2d6c72

bhabegger reviewed Jan 30, 2026

View reviewed changes

thomasmueller and others added 2 commits February 2, 2026 16:18

Update oak-core/src/main/java/org/apache/jackrabbit/oak/plugins/index…

a1c2fa9

…/diff/DiffIndex.java Co-authored-by: Benjamin Habegger <benjamin@habegger.tech>

Update oak-core/src/main/java/org/apache/jackrabbit/oak/plugins/index…

8ec6124

…/diff/DiffIndex.java Co-authored-by: Benjamin Habegger <benjamin@habegger.tech>

amit-jain approved these changes Feb 3, 2026

View reviewed changes

bhabegger reviewed Feb 4, 2026

View reviewed changes

oak-core/src/main/java/org/apache/jackrabbit/oak/plugins/index/diff/JsonNodeBuilder.java Outdated Show resolved Hide resolved

thomasmueller and others added 8 commits February 6, 2026 15:25

Update oak-core/src/main/java/org/apache/jackrabbit/oak/plugins/index…

fbeab78

…/diff/DiffIndex.java Co-authored-by: Amit Jain <amitj@apache.org>

Update oak-core/src/main/java/org/apache/jackrabbit/oak/plugins/index…

2dfbe74

…/diff/DiffIndexMerger.java Co-authored-by: Amit Jain <amitj@apache.org>

Update oak-core/src/main/java/org/apache/jackrabbit/oak/plugins/index…

f9e6fec

…/diff/DiffIndex.java Co-authored-by: Amit Jain <amitj@apache.org>

Update oak-core/src/main/java/org/apache/jackrabbit/oak/plugins/index…

fcca185

…/diff/JsonNodeBuilder.java Co-authored-by: Benjamin Habegger <benjamin@habegger.tech>

Update oak-core/src/main/java/org/apache/jackrabbit/oak/plugins/index…

0f71ddd

…/diff/DiffIndex.java Co-authored-by: Amit Jain <amitj@apache.org>

OAK-12010 Simplified index management

aee4d17

OAK-12010 Simplified index management

7097f76

OAK-12010 Simplified index management

d954a1f

OAK-12010 Simplified index management

2f31a0b

	String merged = new DiffIndexMerger(new String[0], true, true, false).processMerge(JsonObject.fromJson("{}"
	boolean deleteCreatesDummyIndex = true;
	String merged = new DiffIndexMerger(new String[0], deleteCreatesDummyIndex, true, false).processMerge(JsonObject.fromJson("{}"

Conversation

thomasmueller commented Jan 19, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bhabegger left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

thomasmueller Feb 6, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bhabegger Feb 4, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

amit-jain left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

thomasmueller commented Jan 19, 2026 •

edited

Loading

thomasmueller Feb 6, 2026 •

edited

Loading

bhabegger Feb 4, 2026 •

edited

Loading