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.

sonarqubecloud · 2026-01-21T09:45:36Z

Quality Gate passed

Issues
58 New issues
0 Accepted issues

Measures
0 Security Hotspots
81.4% Coverage on New Code
2.0% Duplication on New Code

See analysis details on SonarQube Cloud

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.

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 ?

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)?

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)

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 ?

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.

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

amit-jain

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

amit-jain · 2026-02-03T05:05:30Z

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

+        InputStream in = jcrData.getValue(Type.BINARY).getNewStream();
+        try {


Suggested change

InputStream in = jcrData.getValue(Type.BINARY).getNewStream();

try {

try (InputStream in = jcrData.getValue(Type.BINARY).getNewStream()) {

amit-jain · 2026-02-03T05:09:55Z

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

        return result;
    }

+    public static Collection<String> filterNewestIndexes(Collection<String> indexPaths, NodeState rootState) {


rootState ununsed parameter, consider removing

amit-jain · 2026-02-03T05:13:21Z

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

+            return StringUtils.convertBytesToHex(md.digest(bytes));
+        } catch (NoSuchAlgorithmException e) {
+            // SHA-256 is guaranteed to be available in standard Java platforms
+            throw new RuntimeException("SHA-256 algorithm not available", e);


Maybe IllegalStateException is better?

amit-jain · 2026-02-03T05:14:14Z

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

+        merged.getProperties().remove("reindex");
+        if (!deleteCopiesOutOfTheBoxIndex && indexDiff.toString().equals("{}")) {
+            merged.getProperties().put("type", "\"disabled\"");
+            merged.getProperties().put("mergeComment", "\"This index is superseeded and can be removed\"");


Suggested change

merged.getProperties().put("mergeComment", "\"This index is superseeded and can be removed\"");

merged.getProperties().put("mergeComment", "\"This index is superseded and can be removed\"");

amit-jain · 2026-02-03T05:16:08Z

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

+            }
+        }
+        for (String r : toRemove) {
+            LOG.info("Removing old index " + r);


Suggested change

LOG.info("Removing old index " + r);

LOG.info("Removing old index {}", r);

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

amit-jain · 2026-02-03T05:30:17Z

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

+        }
+        LOG.info("Processing a new diff.index with node store {}", store);
+        JsonObject repositoryDefinitions = RootIndexesListService.getRootIndexDefinitions(indexDefinitions);
+        LOG.debug("Index list {}", repositoryDefinitions.toString());


Suggested change

LOG.debug("Index list {}", repositoryDefinitions.toString());

LOG.debug("Index list {}", repositoryDefinitions);

bhabegger · 2026-02-04T12:53:51Z

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

+     */
+    public static void addOrReplace(NodeBuilder builder, NodeStore nodeStore, String targetPath, String nodeType, String jsonString) throws CommitFailedException, IOException {
+        LOG.info("Storing {}: {}", targetPath, jsonString);
+        if (nodeType.indexOf("/") >= 0) {


Suggested change

if (nodeType.indexOf("/") >= 0) {

if (nodeType.contains("/")) {

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

	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("{}"

		InputStream in = jcrData.getValue(Type.BINARY).getNewStream();
		try {

	InputStream in = jcrData.getValue(Type.BINARY).getNewStream();
	try {
	try (InputStream in = jcrData.getValue(Type.BINARY).getNewStream()) {

	merged.getProperties().put("mergeComment", "\"This index is superseeded and can be removed\"");
	merged.getProperties().put("mergeComment", "\"This index is superseded and can be removed\"");

	LOG.info("Removing old index " + r);
	LOG.info("Removing old index {}", r);

	LOG.debug("Index list {}", repositoryDefinitions.toString());
	LOG.debug("Index list {}", repositoryDefinitions);

	if (nodeType.indexOf("/") >= 0) {
	if (nodeType.contains("/")) {

Conversation

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

Uh oh!

Uh oh!

sonarqubecloud bot commented Jan 21, 2026

Quality Gate passed

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!

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!

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!

amit-jain 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!

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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

thomasmueller commented Jan 19, 2026 •

edited

Loading

bhabegger Feb 4, 2026 •

edited

Loading