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

Conversation

chibulcuteanu
Copy link
Contributor

@chibulcuteanu chibulcuteanu commented Mar 11, 2025

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

@chibulcuteanu chibulcuteanu force-pushed the OAK-11594 branch 2 times, most recently from 238123c to 5438a19 Compare March 11, 2025 12:23
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()

Comment on lines +217 to +218
LOG.info("Provided index [{}] has a different type compared to the existing index." +
" Using lane from the index definition provided", indexInfo.indexPath);
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);

Comment on lines +220 to +221
List<String> asyncLaneList = indexInfo.type.equals(TYPE_ELASTICSEARCH) ?
List.of(indexInfo.asyncLaneName) : List.of(indexInfo.asyncLaneName, INDEXING_MODE_NRT);
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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants