Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

OAK-11594 - Converting existing indexes from a type to another leaves the async lane the same #2173

Open
wants to merge 1 commit into
base: trunk
Choose a base branch
from
Open
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
OAK-11594 - Converting existing indexes from a type to another leaves…
… the async lane the same
chibulcu committed Mar 13, 2025
commit caad7b636049adce42e97df089ba1617721d58be
Original file line number Diff line number Diff line change
@@ -36,6 +36,7 @@
import org.apache.jackrabbit.oak.plugins.index.MetricsUtils;
import org.apache.jackrabbit.oak.plugins.index.importer.AsyncIndexerLock.LockToken;
import org.apache.jackrabbit.oak.plugins.index.upgrade.IndexDisabler;
import org.apache.jackrabbit.oak.plugins.memory.PropertyStates;
import org.apache.jackrabbit.oak.spi.commit.EditorDiff;
import org.apache.jackrabbit.oak.spi.commit.VisibleEditor;
import org.apache.jackrabbit.oak.spi.state.NodeBuilder;
@@ -58,6 +59,9 @@
import static org.apache.jackrabbit.oak.commons.conditions.Validate.checkArgument;
import static java.util.Objects.requireNonNull;
import static org.apache.jackrabbit.oak.plugins.index.IndexConstants.REINDEX_COUNT;
import static org.apache.jackrabbit.oak.plugins.index.IndexConstants.INDEXING_MODE_NRT;
import static org.apache.jackrabbit.oak.plugins.index.IndexConstants.ASYNC_PROPERTY_NAME;
import static org.apache.jackrabbit.oak.plugins.index.IndexConstants.TYPE_PROPERTY_NAME;
import static org.apache.jackrabbit.oak.plugins.index.IndexUtils.INDEXING_PHASE_LOGGER;
import static org.apache.jackrabbit.oak.plugins.index.importer.IndexDefinitionUpdater.INDEX_DEFINITIONS_JSON;
import static org.apache.jackrabbit.oak.plugins.index.importer.NodeStoreUtils.mergeWithConcurrentCheck;
@@ -67,6 +71,10 @@ public class IndexImporter {
* Symbolic name use to indicate sync indexes
*/
static final String ASYNC_LANE_SYNC = "sync";
/**
* Symbolic name use to indicate elasticsearch index type
*/
static final String TYPE_ELASTICSEARCH = "elasticsearch";
/*
* System property name for flag for preserve checkpoint. If this is set to true, then checkpoint cleanup will be skipped.
* Default is set to false.
@@ -200,6 +208,21 @@ void switchLanes() throws CommitFailedException {
if (!indexInfo.newIndex) {
NodeBuilder idxBuilder = NodeStoreUtils.childBuilder(builder, indexInfo.indexPath);
indexPathsToUpdate.add(indexInfo.indexPath);
String idxBuilderType = idxBuilder.getString(TYPE_PROPERTY_NAME);

// check if provided index definitions is of different type than existing one
if (idxBuilderType != null &&
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can be replaced with StringUtils.equals()

!idxBuilderType.equals(indexInfo.type)) {

LOG.info("Provided index [{}] has a different type compared to the existing index." +
" Using lane from the index definition provided", indexInfo.indexPath);
Comment on lines +217 to +218
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would improve this message:

Suggested change
LOG.info("Provided index [{}] has a different type compared to the existing index." +
" Using lane from the index definition provided", indexInfo.indexPath);
LOG.info("Provided index [{}] has a different type [{}] compared to the existing index [{}]." +
" Using lane from the index definition provided", indexInfo.indexPath, indexInfo.type, idxBuilderType);


List<String> asyncLaneList = indexInfo.type.equals(TYPE_ELASTICSEARCH) ?
List.of(indexInfo.asyncLaneName) : List.of(indexInfo.asyncLaneName, INDEXING_MODE_NRT);
Comment on lines +220 to +221
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think this is always correct. When the provided index is not of type elasticsearch, we are always adding nrt. This is wrong when we would convert an elasticsearch to lucene that does not need to be nrt.

As an example, try to change the unit test at line 173 with

 builder.child("idx-a").setProperty("async", asList("async"), Type.STRINGS);

I would expect the async-previous at line 188 to be async and not async, nrt.

I think that when the index types differ, we should always use List.of(indexInfo.asyncLaneName). If we want to make sure that this logic is executed only when there is a conversion from lucene to elastic or viceversa, we can check that at least one of the types (provided or existing) is elasticsearch and completely skip this logic.


PropertyState asyncProperty = PropertyStates.createProperty(ASYNC_PROPERTY_NAME, asyncLaneList, Type.STRINGS);
idxBuilder.setProperty(asyncProperty);
}
AsyncLaneSwitcher.switchLane(idxBuilder, AsyncLaneSwitcher.getTempLaneName(indexInfo.asyncLaneName));
}
}
Original file line number Diff line number Diff line change
@@ -25,6 +25,7 @@
import java.nio.file.Files;
import java.nio.file.Path;
import java.text.MessageFormat;
import java.util.List;
import java.util.Properties;
import java.util.Set;

@@ -77,6 +78,7 @@
import static org.apache.jackrabbit.oak.plugins.index.IndexConstants.TYPE_PROPERTY_NAME;
import static org.apache.jackrabbit.oak.plugins.index.IndexUtils.createIndexDefinition;
import static org.apache.jackrabbit.oak.plugins.index.importer.AsyncIndexerLock.NOOP_LOCK;
import static org.apache.jackrabbit.oak.plugins.index.importer.AsyncLaneSwitcher.ASYNC_PREVIOUS;
import static org.apache.jackrabbit.oak.plugins.index.importer.IndexDefinitionUpdater.INDEX_DEFINITIONS_JSON;
import static org.apache.jackrabbit.oak.plugins.memory.EmptyNodeState.EMPTY_NODE;
import static org.junit.Assert.assertEquals;
@@ -142,6 +144,50 @@ public void switchLanes() throws Exception{
assertEquals(AsyncLaneSwitcher.getTempLaneName("async"), idxb.getString(ASYNC_PROPERTY_NAME));
}

@Test
public void switchLanesLuceneToElastic() throws Exception{
NodeBuilder builder = store.getRoot().builder();
builder.child("idx-a").setProperty("type", "elasticsearch");
builder.child("idx-a").setProperty("async", "elastic-async");

store.merge(builder, EmptyHook.INSTANCE, CommitInfo.EMPTY);
createIndexDirs("/idx-a");

builder.child("idx-a").setProperty("type", "lucene");
builder.child("idx-a").setProperty("async", asList("async", "nrt"), Type.STRINGS);

store.merge(builder, EmptyHook.INSTANCE, CommitInfo.EMPTY);

IndexImporter importer = new IndexImporter(store, temporaryFolder.getRoot(), provider, NOOP_LOCK);
importer.switchLanes();

NodeState idxa = NodeStateUtils.getNode(store.getRoot(), "/idx-a");
assertEquals(AsyncLaneSwitcher.getTempLaneName("elastic-async"), idxa.getString(ASYNC_PROPERTY_NAME));
assertEquals(idxa.getStrings(ASYNC_PREVIOUS), List.of("elastic-async"));
}

@Test
public void switchLanesElasticToLucene() throws Exception{
NodeBuilder builder = store.getRoot().builder();
builder.child("idx-a").setProperty("type", "lucene");
builder.child("idx-a").setProperty("async", asList("async", "nrt"), Type.STRINGS);

store.merge(builder, EmptyHook.INSTANCE, CommitInfo.EMPTY);
createIndexDirs("/idx-a");

builder.child("idx-a").setProperty("type", "elasticsearch");
builder.child("idx-a").setProperty("async", List.of("elastic-async"), Type.STRINGS);

store.merge(builder, EmptyHook.INSTANCE, CommitInfo.EMPTY);

IndexImporter importer = new IndexImporter(store, temporaryFolder.getRoot(), provider, NOOP_LOCK);
importer.switchLanes();

NodeState idxa = NodeStateUtils.getNode(store.getRoot(), "/idx-a");
assertEquals(AsyncLaneSwitcher.getTempLaneName("async"), idxa.getString(ASYNC_PROPERTY_NAME));
assertEquals(idxa.getStrings(ASYNC_PREVIOUS), asList("async", "nrt"));
}

@Test(expected = NullPointerException.class)
public void importData_NoProvider() throws Exception{
NodeBuilder builder = store.getRoot().builder();