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-9143: Use seed instead of reindexCount for elastic index suffix #234

Closed
wants to merge 3 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
Expand Up @@ -20,13 +20,18 @@

import org.apache.jackrabbit.oak.plugins.index.elastic.ElasticConnection;
import org.apache.jackrabbit.oak.plugins.index.elastic.ElasticIndexNameHelper;
import org.elasticsearch.action.admin.indices.alias.get.GetAliasesRequest;
import org.elasticsearch.action.admin.indices.delete.DeleteIndexRequest;
import org.elasticsearch.action.support.master.AcknowledgedResponse;
import org.elasticsearch.client.GetAliasesResponse;
import org.elasticsearch.client.RequestOptions;
import org.elasticsearch.cluster.metadata.AliasMetadata;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;

import java.io.IOException;
import java.util.Map;
import java.util.Set;

public class TestHelper {

Expand All @@ -45,15 +50,16 @@ public static String getUniqueIndexName(String name) {
*/
public static void cleanupRemoteElastic(ElasticConnection connection, String indexName) throws IOException {
String alias = ElasticIndexNameHelper.getIndexAlias(connection.getIndexPrefix(), "/oak:index/" + indexName);
/*
Adding index suffix as -1 because reindex count will always be 1 here (we are not doing any reindexing in the benchmark tests)
TODO: If we write benchmarks for elastic reindex - this needs to be changed to get the reindex count from the index def node
*/
String remoteIndexName = ElasticIndexNameHelper.getElasticSafeIndexName(alias + "-1");
AcknowledgedResponse deleteIndexResponse = connection.getClient().indices().
delete(new DeleteIndexRequest(remoteIndexName), RequestOptions.DEFAULT);
if (!deleteIndexResponse.isAcknowledged()) {
LOG.warn("Delete index call not acknowledged for index " + remoteIndexName + " .Please check if remote index deleted or not.");
// get and delete the indexes which this alias is pointing to
GetAliasesRequest getAliasesRequest = new GetAliasesRequest(alias);
GetAliasesResponse aliasesResponse = connection.getClient().indices().getAlias(getAliasesRequest, RequestOptions.DEFAULT);
Map<String, Set<AliasMetadata>> aliases = aliasesResponse.getAliases();
for (String remoteIndexName : aliases.keySet()) {
AcknowledgedResponse deleteIndexResponse = connection.getClient().indices().
delete(new DeleteIndexRequest(remoteIndexName), RequestOptions.DEFAULT);
if (!deleteIndexResponse.isAcknowledged()) {
LOG.warn("Delete index call not acknowledged for index " + remoteIndexName + " .Please check if remote index deleted or not.");
}
}
}

Expand Down
Expand Up @@ -51,6 +51,11 @@ public class ElasticIndexDefinition extends IndexDefinition {
public static final String BULK_RETRIES_BACKOFF = "bulkRetriesBackoff";
public static final long BULK_RETRIES_BACKOFF_DEFAULT = 200;

/**
* Hidden property for storing a seed value to be used as suffix in remote index name.
*/
public static final String PROP_INDEX_NAME_SEED = ":nameSeed";

/**
* Node name under which various analyzers are configured
*/
Expand All @@ -71,8 +76,6 @@ public class ElasticIndexDefinition extends IndexDefinition {
isAnalyzable = type -> Arrays.binarySearch(NOT_ANALYZED_TYPES, type) < 0;
}

private final String remoteIndexName;

public final int bulkActions;
public final long bulkSizeBytes;
public final long bulkFlushIntervalMs;
Expand All @@ -84,9 +87,7 @@ public class ElasticIndexDefinition extends IndexDefinition {

public ElasticIndexDefinition(NodeState root, NodeState defn, String indexPath, String indexPrefix) {
super(root, defn, determineIndexFormatVersion(defn), determineUniqueId(defn), indexPath);
String indexSuffix = "-" + getReindexCount();
this.remoteAlias = ElasticIndexNameHelper.getIndexAlias(indexPrefix != null ? indexPrefix : "", getIndexPath());
this.remoteIndexName = ElasticIndexNameHelper.getElasticSafeIndexName(this.remoteAlias + indexSuffix);
this.bulkActions = getOptionalValue(defn, BULK_ACTIONS, BULK_ACTIONS_DEFAULT);
this.bulkSizeBytes = getOptionalValue(defn, BULK_SIZE_BYTES, BULK_SIZE_BYTES_DEFAULT);
this.bulkFlushIntervalMs = getOptionalValue(defn, BULK_FLUSH_INTERVAL_MS, BULK_FLUSH_INTERVAL_MS_DEFAULT);
Expand All @@ -109,16 +110,6 @@ public String getRemoteIndexAlias() {
return remoteAlias;
}

/**
* Returns the index identifier on the Elasticsearch cluster. Notice this can be different from the value returned
* from {@code getIndexName}. The index name shouldn't be used for index read or updates. Alias obtained from {@link #getRemoteIndexAlias()}
* should be used for such purposes.
* @return the Elasticsearch index identifier
*/
public String getRemoteIndexName() {
return remoteIndexName;
}

public Map<String, List<PropertyDefinition>> getPropertiesByName() {
return propertiesByName;
}
Expand Down
Expand Up @@ -19,6 +19,7 @@

import org.apache.jackrabbit.oak.commons.PathUtils;

import java.util.UUID;
import java.util.regex.Pattern;
import java.util.stream.Collectors;
import java.util.stream.StreamSupport;
Expand All @@ -39,6 +40,26 @@ public static String getIndexAlias(String indexPrefix, String indexPath) {
return getElasticSafeIndexName(indexPrefix + "." + indexPath);
}

/**
* Create a name for remote elastic index from given index definition and seed.
* @param indexDefinition elastic index definition to use
* @param seed seed to use
* @return remote elastic index name
*/
public static String getRemoteIndexName(ElasticIndexDefinition indexDefinition, long seed) {
return getElasticSafeIndexName(
Copy link
Author

@averma21 averma21 Jul 21, 2020

Choose a reason for hiding this comment

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

I can even make this method getElasticSafeIndexName private. Other than this class, it is being used in benchmark tests -

String remoteIndexName = ElasticIndexNameHelper.getElasticSafeIndexName(alias + "-1");

That logic won't work anyway because we won't have reindexCount as suffix. I think alias can directly be used for deletion there.

cc @fabriziofortino @nit0906

Copy link
Author

@averma21 averma21 Jul 21, 2020

Choose a reason for hiding this comment

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

Made the method private and adjusted benchmark tests.

indexDefinition.getRemoteIndexAlias() + "-" + Long.toHexString(seed));
}

/**
* Create a name for remote elastic index from given index definition and a randomly generated seed.
* @param indexDefinition elastic index definition to use
* @return remote elastic index name
*/
public static String getRemoteIndexName(ElasticIndexDefinition indexDefinition) {
return getRemoteIndexName(indexDefinition, UUID.randomUUID().getMostSignificantBits());
}

/**
* <ul>
* <li>abc -> abc</li>
Expand All @@ -48,7 +69,7 @@ public static String getIndexAlias(String indexPrefix, String indexPath) {
* <p>
* The resulting file name would be truncated to MAX_NAME_LENGTH
*/
public static String getElasticSafeIndexName(String indexPath) {
private static String getElasticSafeIndexName(String indexPath) {
String name = StreamSupport
.stream(PathUtils.elements(indexPath).spliterator(), false)
.limit(3) //Max 3 nodeNames including oak:index which is the immediate parent for any indexPath
Expand Down
Expand Up @@ -28,6 +28,7 @@
import org.jetbrains.annotations.Nullable;

import java.io.IOException;
import java.util.UUID;

class ElasticIndexEditorContext extends FulltextIndexEditorContext<ElasticDocument> {

Expand Down Expand Up @@ -61,7 +62,10 @@ public void enableReindexMode() {
// Now, that index definition _might_ have been migrated by super call, it would be ok to
// get writer and provision index settings and mappings
try {
getWriter().provisionIndex();
long seed = UUID.randomUUID().getMostSignificantBits();
// merge gets called on node store later in the indexing flow
definitionBuilder.setProperty(ElasticIndexDefinition.PROP_INDEX_NAME_SEED, seed);
averma21 marked this conversation as resolved.
Show resolved Hide resolved
getWriter().provisionIndex(seed);
} catch (IOException e) {
throw new IllegalStateException("Unable to provision index", e);
}
Expand Down
Expand Up @@ -34,8 +34,8 @@
*/
class ElasticIndexHelper {

public static CreateIndexRequest createIndexRequest(ElasticIndexDefinition indexDefinition) throws IOException {
final CreateIndexRequest request = new CreateIndexRequest(indexDefinition.getRemoteIndexName());
public static CreateIndexRequest createIndexRequest(String remoteIndexName, ElasticIndexDefinition indexDefinition) throws IOException {
final CreateIndexRequest request = new CreateIndexRequest(remoteIndexName);

// provision settings
request.settings(loadSettings(indexDefinition));
Expand Down
Expand Up @@ -18,6 +18,7 @@

import org.apache.jackrabbit.oak.plugins.index.elastic.ElasticConnection;
import org.apache.jackrabbit.oak.plugins.index.elastic.ElasticIndexDefinition;
import org.apache.jackrabbit.oak.plugins.index.elastic.ElasticIndexNameHelper;
import org.apache.jackrabbit.oak.plugins.index.elastic.util.ElasticIndexUtils;
import org.apache.jackrabbit.oak.plugins.index.search.spi.editor.FulltextIndexWriter;
import org.apache.jackrabbit.oak.spi.commit.CommitInfo;
Expand Down Expand Up @@ -97,29 +98,29 @@ public boolean close(long timestamp) {
return bulkProcessorHandler.close();
}

protected void provisionIndex() throws IOException {
protected void provisionIndex(long seed) throws IOException {
// check if index already exists
final String indexName = ElasticIndexNameHelper.getRemoteIndexName(indexDefinition, seed);
boolean exists = elasticConnection.getClient().indices().exists(
new GetIndexRequest(indexDefinition.getRemoteIndexName()), RequestOptions.DEFAULT
new GetIndexRequest(indexName), RequestOptions.DEFAULT
);
if (exists) {
LOG.info("Index {} already exists. Skip index provision", indexDefinition.getRemoteIndexName());
LOG.info("Index {} already exists. Skip index provision", indexName);
return;
}

final IndicesClient indicesClient = elasticConnection.getClient().indices();
final String indexName = indexDefinition.getRemoteIndexName();

// create the new index
final CreateIndexRequest request = ElasticIndexHelper.createIndexRequest(indexDefinition);
final CreateIndexRequest request = ElasticIndexHelper.createIndexRequest(indexName, indexDefinition);
try {
if (LOG.isDebugEnabled()) {
final String requestMsg = Strings.toString(request.toXContent(jsonBuilder(), EMPTY_PARAMS));
LOG.debug("Creating Index with request {}", requestMsg);
}
CreateIndexResponse response = indicesClient.create(request, RequestOptions.DEFAULT);
LOG.info("Updated settings for index {}. Response acknowledged: {}",
indexDefinition.getRemoteIndexName(), response.isAcknowledged());
indexName, response.isAcknowledged());
checkResponseAcknowledgement(response, "Create index call not acknowledged for index " + indexName);
} catch (ElasticsearchStatusException ese) {
// We already check index existence as first thing in this method, if we get here it means we have got into
Expand Down
Expand Up @@ -19,6 +19,7 @@
import com.fasterxml.jackson.databind.ObjectMapper;
import org.apache.jackrabbit.oak.api.Tree;
import org.apache.jackrabbit.oak.plugins.index.elastic.ElasticIndexDefinition;
import org.apache.jackrabbit.oak.plugins.index.elastic.ElasticIndexNameHelper;
import org.apache.jackrabbit.oak.plugins.index.elastic.util.ElasticIndexDefinitionBuilder;
import org.apache.jackrabbit.oak.plugins.index.search.util.IndexDefinitionBuilder;
import org.apache.jackrabbit.oak.spi.state.NodeState;
Expand Down Expand Up @@ -47,7 +48,7 @@ public void multiRulesWithSamePropertyNames() throws IOException {
ElasticIndexDefinition definition =
new ElasticIndexDefinition(nodeState, nodeState, "path", "prefix");

CreateIndexRequest request = ElasticIndexHelper.createIndexRequest(definition);
CreateIndexRequest request = ElasticIndexHelper.createIndexRequest(ElasticIndexNameHelper.getRemoteIndexName(definition), definition);

ObjectMapper mapper = new ObjectMapper();
Map<String, Object> jsonMap = mapper.readValue(request.mappings().streamInput(), Map.class);
Expand All @@ -70,7 +71,7 @@ public void multiRulesWithSamePropertyNamesDifferentTypes() throws IOException {
ElasticIndexDefinition definition =
new ElasticIndexDefinition(nodeState, nodeState, "path", "prefix");

ElasticIndexHelper.createIndexRequest(definition);
ElasticIndexHelper.createIndexRequest(ElasticIndexNameHelper.getRemoteIndexName(definition), definition);
}

@Test
Expand All @@ -85,7 +86,7 @@ public void oakAnalyzer() throws IOException {
ElasticIndexDefinition definition =
new ElasticIndexDefinition(nodeState, nodeState, "path", "prefix");

CreateIndexRequest request = ElasticIndexHelper.createIndexRequest(definition);
CreateIndexRequest request = ElasticIndexHelper.createIndexRequest(ElasticIndexNameHelper.getRemoteIndexName(definition), definition);

assertThat(request.settings().get("analysis.filter.oak_word_delimiter_graph_filter.preserve_original"), is("false"));

Expand Down Expand Up @@ -113,7 +114,7 @@ public void oakAnalyzerWithOriginalTerm() throws IOException {
ElasticIndexDefinition definition =
new ElasticIndexDefinition(nodeState, nodeState, "path", "prefix");

CreateIndexRequest request = ElasticIndexHelper.createIndexRequest(definition);
CreateIndexRequest request = ElasticIndexHelper.createIndexRequest(ElasticIndexNameHelper.getRemoteIndexName(definition), definition);

assertThat(request.settings().get("analysis.filter.oak_word_delimiter_graph_filter.preserve_original"), is("true"));
}
Expand All @@ -130,7 +131,7 @@ public void spellCheck() throws IOException {
ElasticIndexDefinition definition =
new ElasticIndexDefinition(nodeState, nodeState, "path", "prefix");

CreateIndexRequest request = ElasticIndexHelper.createIndexRequest(definition);
CreateIndexRequest request = ElasticIndexHelper.createIndexRequest(ElasticIndexNameHelper.getRemoteIndexName(definition), definition);

assertThat(request.settings().get("analysis.filter.shingle.type"), is("shingle"));
assertThat(request.settings().get("analysis.analyzer.trigram.type"), is("custom"));
Expand Down