This is an automated email from the ASF dual-hosted git repository.
fortino pushed a commit to branch trunk
in repository https://gitbox.apache.org/repos/asf/jackrabbit-oak.git
The following commit(s) were added to refs/heads/trunk by this push:
new 57d124c6fa OAK-10811: (oak-search-elastic) reduce contention in
IndexTracker (#1466)
57d124c6fa is described below
commit 57d124c6fa9f4b3683a45ff96b65259ab21d09ec
Author: Fabrizio Fortino <fabrizio.fort...@gmail.com>
AuthorDate: Thu May 23 09:23:26 2024 +0200
OAK-10811: (oak-search-elastic) reduce contention in IndexTracker (#1466)
* OAK-10811: (oak-search-elastic) reduce contention in IndexTracker
* OAK-10811: ElasticIndexInfoProvider uses node api to get :status info
* OAK-10811: rolled back unneeded change
---
.../index/elastic/ElasticIndexInfoProvider.java | 17 +++++--
.../plugins/index/elastic/ElasticIndexNode.java | 5 ++
.../index/elastic/ElasticIndexProviderService.java | 2 +-
.../plugins/index/elastic/ElasticIndexTracker.java | 37 +++++++++++++-
.../index/elastic/index/ElasticIndexWriter.java | 6 +--
.../index/elastic/ElasticAbstractQueryTest.java | 3 +-
.../index/elastic/index/ElasticIndexTest.java | 58 ++++++++++++++++++++++
7 files changed, 116 insertions(+), 12 deletions(-)
diff --git
a/oak-search-elastic/src/main/java/org/apache/jackrabbit/oak/plugins/index/elastic/ElasticIndexInfoProvider.java
b/oak-search-elastic/src/main/java/org/apache/jackrabbit/oak/plugins/index/elastic/ElasticIndexInfoProvider.java
index 4df7a0bf07..f552962d7c 100644
---
a/oak-search-elastic/src/main/java/org/apache/jackrabbit/oak/plugins/index/elastic/ElasticIndexInfoProvider.java
+++
b/oak-search-elastic/src/main/java/org/apache/jackrabbit/oak/plugins/index/elastic/ElasticIndexInfoProvider.java
@@ -27,6 +27,8 @@ import
org.apache.jackrabbit.oak.plugins.index.IndexInfoProvider;
import org.apache.jackrabbit.oak.plugins.index.IndexUtils;
import org.apache.jackrabbit.oak.plugins.index.search.IndexDefinition;
import org.apache.jackrabbit.oak.spi.state.NodeState;
+import org.apache.jackrabbit.oak.spi.state.NodeStateUtils;
+import org.apache.jackrabbit.oak.spi.state.NodeStore;
import org.apache.jackrabbit.util.ISO8601;
import org.jetbrains.annotations.NotNull;
import org.jetbrains.annotations.Nullable;
@@ -38,12 +40,16 @@ import
co.elastic.clients.elasticsearch.cluster.HealthResponse;
class ElasticIndexInfoProvider implements IndexInfoProvider {
+ private final NodeStore nodeStore;
+
private final ElasticIndexTracker indexTracker;
private final AsyncIndexInfoService asyncIndexInfoService;
- ElasticIndexInfoProvider(@NotNull ElasticIndexTracker indexTracker,
- @NotNull AsyncIndexInfoService
asyncIndexInfoService) {
+ ElasticIndexInfoProvider(@NotNull NodeStore nodeStore,
+ @NotNull ElasticIndexTracker indexTracker,
+ @NotNull AsyncIndexInfoService
asyncIndexInfoService) {
+ this.nodeStore = nodeStore;
this.indexTracker = indexTracker;
this.asyncIndexInfoService = asyncIndexInfoService;
}
@@ -57,18 +63,19 @@ class ElasticIndexInfoProvider implements IndexInfoProvider
{
public @Nullable IndexInfo getInfo(String indexPath) {
ElasticIndexNode node = indexTracker.acquireIndexNode(indexPath);
try {
- String asyncName =
IndexUtils.getAsyncLaneName(node.getDefinition().getDefinitionNodeState(),
indexPath);
+ NodeState idxState = NodeStateUtils.getNode(nodeStore.getRoot(),
indexPath);
+ String asyncName = IndexUtils.getAsyncLaneName(idxState,
indexPath);
AsyncIndexInfo asyncInfo =
asyncIndexInfoService.getInfo(asyncName);
return new ElasticIndexInfo(
indexPath,
asyncName,
asyncInfo != null ? asyncInfo.getLastIndexedTo() : -1L,
-
getStatusTimestamp(node.getDefinition().getDefinitionNodeState(),
IndexDefinition.STATUS_LAST_UPDATED),
+ getStatusTimestamp(idxState,
IndexDefinition.STATUS_LAST_UPDATED),
node.getIndexStatistics().numDocs(),
node.getIndexStatistics().primaryStoreSize(),
node.getIndexStatistics().creationDate(),
-
getStatusTimestamp(node.getDefinition().getDefinitionNodeState(),
IndexDefinition.REINDEX_COMPLETION_TIMESTAMP)
+ getStatusTimestamp(idxState,
IndexDefinition.REINDEX_COMPLETION_TIMESTAMP)
);
} finally {
node.release();
diff --git
a/oak-search-elastic/src/main/java/org/apache/jackrabbit/oak/plugins/index/elastic/ElasticIndexNode.java
b/oak-search-elastic/src/main/java/org/apache/jackrabbit/oak/plugins/index/elastic/ElasticIndexNode.java
index c5179348f2..4bb3722876 100644
---
a/oak-search-elastic/src/main/java/org/apache/jackrabbit/oak/plugins/index/elastic/ElasticIndexNode.java
+++
b/oak-search-elastic/src/main/java/org/apache/jackrabbit/oak/plugins/index/elastic/ElasticIndexNode.java
@@ -40,6 +40,11 @@ public class ElasticIndexNode implements IndexNode {
// do nothing
}
+ /**
+ * Returns the index definition for this index node. This method ensures
that the index definition is always
+ * up-to-date with the latest changes in the repository, except for the
:status node.
+ * For latest changes in the :status node, the node api should be used.
+ */
@Override
public ElasticIndexDefinition getDefinition() {
return indexDefinition;
diff --git
a/oak-search-elastic/src/main/java/org/apache/jackrabbit/oak/plugins/index/elastic/ElasticIndexProviderService.java
b/oak-search-elastic/src/main/java/org/apache/jackrabbit/oak/plugins/index/elastic/ElasticIndexProviderService.java
index 58c62c0fae..f2aebf1670 100644
---
a/oak-search-elastic/src/main/java/org/apache/jackrabbit/oak/plugins/index/elastic/ElasticIndexProviderService.java
+++
b/oak-search-elastic/src/main/java/org/apache/jackrabbit/oak/plugins/index/elastic/ElasticIndexProviderService.java
@@ -181,7 +181,7 @@ public class ElasticIndexProviderService {
// register info provider for oak index stats
regs.add(bundleContext.registerService(IndexInfoProvider.class.getName(),
- new ElasticIndexInfoProvider(indexTracker,
asyncIndexInfoService), null));
+ new ElasticIndexInfoProvider(nodeStore, indexTracker,
asyncIndexInfoService), null));
// register mbean for detailed elastic stats and utility actions
ElasticIndexMBean mBean = new ElasticIndexMBean(indexTracker);
diff --git
a/oak-search-elastic/src/main/java/org/apache/jackrabbit/oak/plugins/index/elastic/ElasticIndexTracker.java
b/oak-search-elastic/src/main/java/org/apache/jackrabbit/oak/plugins/index/elastic/ElasticIndexTracker.java
index fe924b2675..079666b676 100644
---
a/oak-search-elastic/src/main/java/org/apache/jackrabbit/oak/plugins/index/elastic/ElasticIndexTracker.java
+++
b/oak-search-elastic/src/main/java/org/apache/jackrabbit/oak/plugins/index/elastic/ElasticIndexTracker.java
@@ -16,6 +16,7 @@
*/
package org.apache.jackrabbit.oak.plugins.index.elastic;
+import org.apache.jackrabbit.oak.plugins.index.search.IndexDefinition;
import
org.apache.jackrabbit.oak.plugins.index.search.spi.query.FulltextIndexTracker;
import org.apache.jackrabbit.oak.spi.commit.CommitInfo;
import org.apache.jackrabbit.oak.spi.commit.Observer;
@@ -35,12 +36,12 @@ public class ElasticIndexTracker extends
FulltextIndexTracker<ElasticIndexNodeMa
@Override
public boolean isUpdateNeeded(NodeState before, NodeState after) {
- // for Elastic we are not interested in checking for updates on
:status, index definition is enough.
+ // for Elastic, we are not interested in checking for updates on
:status, index definition is enough.
// The :status gets updated every time the indexed content is changed
(with properties like last_update_ts),
// removing the check on :status reduces drastically the contention
between queries (that need to acquire the
// read lock) and updates (need to acquire the write lock).
// Moreover, we don't check diffs in stored index definitions since
are not created for elastic.
- return !EqualsDiff.equals(before, after);
+ return !IgnoreStatusDiff.equals(before, after);
}
@Override
@@ -60,4 +61,36 @@ public class ElasticIndexTracker extends
FulltextIndexTracker<ElasticIndexNodeMa
public ElasticMetricHandler getElasticMetricHandler() {
return elasticMetricHandler;
}
+
+ static class IgnoreStatusDiff extends EqualsDiff {
+
+ public static boolean equals(NodeState before, NodeState after) {
+ return before.exists() == after.exists()
+ && after.compareAgainstBaseState(before, new
IgnoreStatusDiff());
+ }
+
+ /**
+ * The behaviour of this method is not immediately obvious from the
signature.
+ * For this reason, the javadoc from NodeStateDiff is copied below.
+ * When false is returned, the comparison is aborted because something
has changed.
+ * If the changed node is the status node, the comparison is continued
otherwise we call the super method.
+ * The super method will return false if the node is not the status
node, and it has changed.
+ * <p>
+ * Called for all child nodes that may contain changes between the
before
+ * and after states. The comparison implementation is expected to make
an
+ * effort to avoid calling this method on child nodes under which
nothing
+ * has changed.
+ *
+ * @param name name of the changed child node
+ * @param before child node state before the change
+ * @param after child node state after the change
+ * @return {@code true} to continue the comparison, {@code false} to
abort.
+ * Abort will stop comparing completely, that means sibling
nodes
+ * and sibling nodes of all parents are not further compared.
+ */
+ @Override
+ public boolean childNodeChanged(String name, NodeState before,
NodeState after) {
+ return IndexDefinition.STATUS_NODE.equals(name) ||
super.childNodeChanged(name, before, after);
+ }
+ }
}
diff --git
a/oak-search-elastic/src/main/java/org/apache/jackrabbit/oak/plugins/index/elastic/index/ElasticIndexWriter.java
b/oak-search-elastic/src/main/java/org/apache/jackrabbit/oak/plugins/index/elastic/index/ElasticIndexWriter.java
index 3bbd7d3daf..85540b1f47 100644
---
a/oak-search-elastic/src/main/java/org/apache/jackrabbit/oak/plugins/index/elastic/index/ElasticIndexWriter.java
+++
b/oak-search-elastic/src/main/java/org/apache/jackrabbit/oak/plugins/index/elastic/index/ElasticIndexWriter.java
@@ -150,12 +150,12 @@ class ElasticIndexWriter implements
FulltextIndexWriter<ElasticDocument> {
private void saveMetrics() {
ElasticIndexNode indexNode =
indexTracker.acquireIndexNode(indexDefinition.getIndexPath());
if (indexNode != null) {
- ElasticIndexStatistics stats = indexNode.getIndexStatistics();
try {
-
indexTracker.getElasticMetricHandler().markDocuments(indexName,
indexNode.getIndexStatistics().numDocs());
+ ElasticIndexStatistics stats = indexNode.getIndexStatistics();
+
indexTracker.getElasticMetricHandler().markDocuments(indexName,
stats.numDocs());
indexTracker.getElasticMetricHandler().markSize(indexName,
stats.primaryStoreSize(), stats.storeSize());
} catch (Exception e) {
- LOG.warn("Unable to store metrics for {}",
indexNode.getDefinition().getIndexPath(), e);
+ LOG.warn("Unable to store metrics for {}",
indexDefinition.getIndexPath(), e);
} finally {
indexNode.release();
}
diff --git
a/oak-search-elastic/src/test/java/org/apache/jackrabbit/oak/plugins/index/elastic/ElasticAbstractQueryTest.java
b/oak-search-elastic/src/test/java/org/apache/jackrabbit/oak/plugins/index/elastic/ElasticAbstractQueryTest.java
index 976ae264dd..9ffb7aa292 100644
---
a/oak-search-elastic/src/test/java/org/apache/jackrabbit/oak/plugins/index/elastic/ElasticAbstractQueryTest.java
+++
b/oak-search-elastic/src/test/java/org/apache/jackrabbit/oak/plugins/index/elastic/ElasticAbstractQueryTest.java
@@ -70,6 +70,7 @@ public abstract class ElasticAbstractQueryTest extends
AbstractQueryTest {
private static final String elasticConnectionString =
System.getProperty("elasticConnectionString");
protected ElasticConnection esConnection;
+ protected ElasticIndexTracker indexTracker;
// This is instantiated during repo creation but not hooked up to the
async indexing lane
// This can be used by the extending classes to trigger the async index
update as per need (not having to wait for async indexing cycle)
protected AsyncIndexUpdate asyncIndexUpdate;
@@ -139,7 +140,7 @@ public abstract class ElasticAbstractQueryTest extends
AbstractQueryTest {
@Override
protected ContentRepository createRepository() {
esConnection = getElasticConnection();
- ElasticIndexTracker indexTracker = new
ElasticIndexTracker(esConnection, getMetricHandler());
+ indexTracker = new ElasticIndexTracker(esConnection,
getMetricHandler());
ElasticIndexEditorProvider editorProvider = new
ElasticIndexEditorProvider(indexTracker, esConnection,
new ExtractedTextCache(10 * FileUtils.ONE_MB, 100));
ElasticIndexProvider indexProvider = new
ElasticIndexProvider(indexTracker);
diff --git
a/oak-search-elastic/src/test/java/org/apache/jackrabbit/oak/plugins/index/elastic/index/ElasticIndexTest.java
b/oak-search-elastic/src/test/java/org/apache/jackrabbit/oak/plugins/index/elastic/index/ElasticIndexTest.java
index 92309e7dd6..db137bdbfc 100644
---
a/oak-search-elastic/src/test/java/org/apache/jackrabbit/oak/plugins/index/elastic/index/ElasticIndexTest.java
+++
b/oak-search-elastic/src/test/java/org/apache/jackrabbit/oak/plugins/index/elastic/index/ElasticIndexTest.java
@@ -18,12 +18,17 @@ package
org.apache.jackrabbit.oak.plugins.index.elastic.index;
import org.apache.jackrabbit.oak.api.Tree;
import
org.apache.jackrabbit.oak.plugins.index.elastic.ElasticAbstractQueryTest;
+import org.apache.jackrabbit.oak.plugins.index.elastic.ElasticIndexDefinition;
+import org.apache.jackrabbit.oak.plugins.index.elastic.ElasticIndexNode;
+import org.apache.jackrabbit.oak.plugins.index.search.FulltextIndexConstants;
import
org.apache.jackrabbit.oak.plugins.index.search.util.IndexDefinitionBuilder;
import org.junit.Test;
import java.util.UUID;
import static org.junit.Assert.assertEquals;
+import static org.junit.Assert.assertNotEquals;
+import static org.junit.Assert.assertNotNull;
public class ElasticIndexTest extends ElasticAbstractQueryTest {
@@ -37,4 +42,57 @@ public class ElasticIndexTest extends
ElasticAbstractQueryTest {
assertEventually(() -> assertEquals(ElasticIndexHelper.MAPPING_VERSION,
getElasticIndexDefinition(index).getMappingVersion()));
}
+
+ @Test
+ public void indexTrackerHandlesIndexDefinitionChanges() throws Exception {
+ IndexDefinitionBuilder builder = createIndex("a");
+ builder.indexRule("nt:base").property("a").propertyIndex();
+ Tree index = setIndex(UUID.randomUUID().toString(), builder);
+ root.commit();
+
+ ElasticIndexDefinition elasticIndexDefinitionT0;
+ ElasticIndexNode indexNodeT0 =
indexTracker.acquireIndexNode(index.getPath());
+ try {
+ elasticIndexDefinitionT0 = indexNodeT0.getDefinition();
+ assertNotNull(elasticIndexDefinitionT0);
+ } finally {
+ indexNodeT0.release();
+ }
+
+ Tree test = root.getTree("/").addChild("test");
+ test.addChild("t").setProperty("a", "foo");
+ root.commit();
+
+ ElasticIndexDefinition elasticIndexDefinitionT1;
+ ElasticIndexNode indexNodeT1 =
indexTracker.acquireIndexNode(index.getPath());
+ try {
+ elasticIndexDefinitionT1 = indexNodeT1.getDefinition();
+ assertNotNull(elasticIndexDefinitionT1);
+ } finally {
+ indexNodeT1.release();
+ }
+
+ // no changes in the index node and definition when the index content
gets updated
+ assertEquals(indexNodeT0, indexNodeT1);
+ assertEquals(elasticIndexDefinitionT0, elasticIndexDefinitionT1);
+
+ Tree b =
index.getChild("indexRules").getChild("nt:base").getChild("properties").addChild("b");
+ b.setProperty(FulltextIndexConstants.PROP_PROPERTY_INDEX, true);
+ b.setProperty(FulltextIndexConstants.PROP_ANALYZED, true);
+ root.commit();
+
+ ElasticIndexDefinition elasticIndexDefinitionT2;
+ ElasticIndexNode indexNodeT2 =
indexTracker.acquireIndexNode(index.getPath());
+ try {
+ elasticIndexDefinitionT2 = indexNodeT2.getDefinition();
+ assertNotNull(elasticIndexDefinitionT2);
+ } finally {
+ indexNodeT2.release();
+ }
+
+ // index node and definition are different after the index definition
change
+ assertNotEquals(indexNodeT1, indexNodeT2);
+ assertNotEquals(elasticIndexDefinitionT1, elasticIndexDefinitionT2);
+ assertNotNull(elasticIndexDefinitionT2.getPropertiesByName().get("b"));
+ }
}
