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

Enable must_exist parameter for update aliases API #11210

Merged
merged 14 commits into from
Jan 9, 2024
Merged
Show file tree
Hide file tree
Changes from 12 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
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -107,6 +107,7 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/),
- Implement on behalf of token passing for extensions ([#8679](https://github.com/opensearch-project/OpenSearch/pull/8679))
- Provide service accounts tokens to extensions ([#9618](https://github.com/opensearch-project/OpenSearch/pull/9618))
- [Streaming Indexing] Introduce new experimental server HTTP transport based on Netty 4 and Project Reactor (Reactor Netty) ([#9672](https://github.com/opensearch-project/OpenSearch/pull/9672))
- Enable must_exist parameter for update aliases API ([#11210](https://github.com/opensearch-project/OpenSearch/pull/11210))
- Add back half_float BKD based sort query optimization ([#11024](https://github.com/opensearch-project/OpenSearch/pull/11024))
- Request level coordinator slow logs ([#10650](https://github.com/opensearch-project/OpenSearch/pull/10650))
- Add template snippets support for field and target_field in KV ingest processor ([#10040](https://github.com/opensearch-project/OpenSearch/pull/10040))
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,131 @@
---
"Throw aliases missing exception when removing non-existing alias with setting must_exist to true":
- skip:
version: " - 2.99.99"
reason: "introduced in 3.0"

- do:
indices.create:
index: test_index

- do:
indices.exists_alias:
name: test_alias

- is_false: ''

- do:
catch: /aliases \[test_alias\] missing/
indices.update_aliases:
body:
actions:
- remove:
index: test_index
alias: test_alias
must_exist: true

- do:
catch: /aliases \[testAlias\*\] missing/
indices.update_aliases:
body:
actions:
- remove:
index: test_index
aliases: [ testAlias* ]
must_exist: true

gaobinlong marked this conversation as resolved.
Show resolved Hide resolved
---
"Throw aliases missing exception when all of the specified aliases are non-existing":
- skip:
version: " - 2.99.99"
reason: "introduced in 3.0"

- do:
indices.create:
index: test_index

- do:
indices.exists_alias:
name: test_alias

- is_false: ''

- do:
catch: /aliases \[test\_alias\] missing/
indices.update_aliases:
body:
actions:
- remove:
index: test_index
alias: test_alias

- do:
catch: /aliases \[test\_alias\*\] missing/
indices.update_aliases:
body:
actions:
- remove:
indices: [ test_index ]
aliases: [ test_alias* ]

---
"Remove successfully when some specified aliases are non-existing":
- skip:
version: " - 2.99.99"
reason: "introduced in 3.0"

- do:
indices.create:
index: test_index

- do:
indices.exists_alias:
name: test_alias

- is_false: ''

- do:
indices.update_aliases:
body:
actions:
- add:
indices: [ test_index ]
aliases: [ test_alias ]

- do:
indices.update_aliases:
body:
actions:
- remove:
index: test_index
aliases: [test_alias, test_alias1, test_alias2]
must_exist: false

- match: { acknowledged: true }

---
"Remove silently when all of the specified aliases are non-existing and must_exist is false":
- skip:
version: " - 2.99.99"
reason: "introduced in 3.0"

- do:
indices.create:
index: test_index

- do:
indices.exists_alias:
name: test_alias

- is_false: ''

- do:
indices.update_aliases:
body:
actions:
- remove:
index: test_index
aliases: [test_alias, test_alias1, test_alias2]
must_exist: false

- match: { acknowledged: true }
Original file line number Diff line number Diff line change
Expand Up @@ -220,9 +220,11 @@
ADD_PARSER.declareField(AliasActions::searchRouting, XContentParser::text, SEARCH_ROUTING, ValueType.INT);
ADD_PARSER.declareField(AliasActions::writeIndex, XContentParser::booleanValue, IS_WRITE_INDEX, ValueType.BOOLEAN);
ADD_PARSER.declareField(AliasActions::isHidden, XContentParser::booleanValue, IS_HIDDEN, ValueType.BOOLEAN);
ADD_PARSER.declareField(AliasActions::mustExist, XContentParser::booleanValue, MUST_EXIST, ValueType.BOOLEAN);
gaobinlong marked this conversation as resolved.
Show resolved Hide resolved
}
private static final ObjectParser<AliasActions, Void> REMOVE_PARSER = parser(REMOVE.getPreferredName(), AliasActions::remove);
static {
REMOVE_PARSER.declareField(AliasActions::mustExist, XContentParser::booleanValue, MUST_EXIST, ValueType.BOOLEAN);
}
private static final ObjectParser<AliasActions, Void> REMOVE_INDEX_PARSER = parser(
REMOVE_INDEX.getPreferredName(),
AliasActions::removeIndex
Expand Down Expand Up @@ -554,6 +556,9 @@
if (null != isHidden) {
builder.field(IS_HIDDEN.getPreferredName(), isHidden);
}
if (null != mustExist) {
builder.field(MUST_EXIST.getPreferredName(), mustExist);
}
builder.endObject();
builder.endObject();
return builder;
Expand Down Expand Up @@ -582,6 +587,8 @@
+ searchRouting
+ ",writeIndex="
+ writeIndex
+ ",mustExist="
+ mustExist
+ "]";
}

Expand All @@ -600,12 +607,13 @@
&& Objects.equals(indexRouting, other.indexRouting)
&& Objects.equals(searchRouting, other.searchRouting)
&& Objects.equals(writeIndex, other.writeIndex)
&& Objects.equals(isHidden, other.isHidden);
&& Objects.equals(isHidden, other.isHidden)
&& Objects.equals(mustExist, other.mustExist);
}

@Override
public int hashCode() {
return Objects.hash(type, indices, aliases, filter, routing, indexRouting, searchRouting, writeIndex, isHidden);
return Objects.hash(type, indices, aliases, filter, routing, indexRouting, searchRouting, writeIndex, isHidden, mustExist);

Check warning on line 616 in server/src/main/java/org/opensearch/action/admin/indices/alias/IndicesAliasesRequest.java

View check run for this annotation

Codecov / codecov/patch

server/src/main/java/org/opensearch/action/admin/indices/alias/IndicesAliasesRequest.java#L616

Added line #L616 was not covered by tests
}
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -50,6 +50,7 @@
import org.opensearch.cluster.metadata.MetadataIndexAliasesService;
import org.opensearch.cluster.service.ClusterService;
import org.opensearch.common.inject.Inject;
import org.opensearch.common.regex.Regex;
import org.opensearch.core.action.ActionListener;
import org.opensearch.core.common.io.stream.StreamInput;
import org.opensearch.core.index.Index;
Expand All @@ -59,6 +60,7 @@

import java.io.IOException;
import java.util.ArrayList;
import java.util.Arrays;
import java.util.Collections;
import java.util.HashSet;
import java.util.List;
Expand Down Expand Up @@ -220,12 +222,33 @@
// for DELETE we expand the aliases
String[] indexAsArray = { concreteIndex };
final Map<String, List<AliasMetadata>> aliasMetadata = metadata.findAliases(action, indexAsArray);
List<String> finalAliases = new ArrayList<>();
Set<String> finalAliases = new HashSet<>();
for (final List<AliasMetadata> curAliases : aliasMetadata.values()) {
for (AliasMetadata aliasMeta : curAliases) {
finalAliases.add(aliasMeta.alias());
}
}

// must_exist can only be set in the Remove Action in Update aliases API,
// we check the value here to make the behavior consistent with Delete aliases API
if (action.mustExist() != null) {
// if must_exist is false, we should make the remove action execute silently,
// so we return the original specified aliases to avoid AliasesNotFoundException
if (!action.mustExist()) {
return action.aliases();

Check warning on line 238 in server/src/main/java/org/opensearch/action/admin/indices/alias/TransportIndicesAliasesAction.java

View check run for this annotation

Codecov / codecov/patch

server/src/main/java/org/opensearch/action/admin/indices/alias/TransportIndicesAliasesAction.java#L238

Added line #L238 was not covered by tests
}

// if there is any non-existing aliases specified in the request and must_exist is true, throw exception in advance
if (finalAliases.isEmpty()) {
throw new AliasesNotFoundException(action.aliases());

Check warning on line 243 in server/src/main/java/org/opensearch/action/admin/indices/alias/TransportIndicesAliasesAction.java

View check run for this annotation

Codecov / codecov/patch

server/src/main/java/org/opensearch/action/admin/indices/alias/TransportIndicesAliasesAction.java#L243

Added line #L243 was not covered by tests
}
String[] nonExistingAliases = Arrays.stream(action.aliases())
.filter(originalAlias -> finalAliases.stream().noneMatch(finalAlias -> Regex.simpleMatch(originalAlias, finalAlias)))
.toArray(String[]::new);

Check warning on line 247 in server/src/main/java/org/opensearch/action/admin/indices/alias/TransportIndicesAliasesAction.java

View check run for this annotation

Codecov / codecov/patch

server/src/main/java/org/opensearch/action/admin/indices/alias/TransportIndicesAliasesAction.java#L245-L247

Added lines #L245 - L247 were not covered by tests
if (nonExistingAliases.length != 0) {
throw new AliasesNotFoundException(nonExistingAliases);

Check warning on line 249 in server/src/main/java/org/opensearch/action/admin/indices/alias/TransportIndicesAliasesAction.java

View check run for this annotation

Codecov / codecov/patch

server/src/main/java/org/opensearch/action/admin/indices/alias/TransportIndicesAliasesAction.java#L249

Added line #L249 was not covered by tests
}
}
return finalAliases.toArray(new String[0]);
} else {
// for ADD and REMOVE_INDEX we just return the current aliases
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,7 @@
import org.opensearch.action.admin.indices.alias.IndicesAliasesRequest;
import org.opensearch.common.Nullable;
import org.opensearch.core.common.Strings;
import org.opensearch.rest.action.admin.indices.AliasesNotFoundException;

/**
* Individual operation to perform on the cluster state as part of an {@link IndicesAliasesRequest}.
Expand Down Expand Up @@ -225,8 +226,8 @@ boolean removeIndex() {
@Override
boolean apply(NewAliasValidator aliasValidator, Metadata.Builder metadata, IndexMetadata index) {
if (false == index.getAliases().containsKey(alias)) {
if (mustExist != null && mustExist.booleanValue()) {
throw new IllegalArgumentException("required alias [" + alias + "] does not exist");
if (mustExist != null && mustExist) {
throw new AliasesNotFoundException(alias);
}
return false;
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -241,6 +241,7 @@ public void testParseRemove() throws IOException {
String[] indices = generateRandomStringArray(10, 5, false, false);
String[] aliases = generateRandomStringArray(10, 5, false, false);
XContentBuilder b = XContentBuilder.builder(randomFrom(XContentType.values()).xContent());
boolean mustExist = randomBoolean();
b.startObject();
{
b.startObject("remove");
Expand All @@ -255,6 +256,9 @@ public void testParseRemove() throws IOException {
} else {
b.field("alias", aliases[0]);
}
if (mustExist) {
b.field("must_exist", true);
}
}
b.endObject();
}
Expand All @@ -265,6 +269,9 @@ public void testParseRemove() throws IOException {
assertEquals(AliasActions.Type.REMOVE, action.actionType());
assertThat(action.indices(), equalTo(indices));
assertThat(action.aliases(), equalTo(aliases));
if (mustExist) {
assertThat(action.mustExist(), equalTo(true));
}
}
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,7 @@
import org.opensearch.common.util.set.Sets;
import org.opensearch.core.index.Index;
import org.opensearch.index.IndexNotFoundException;
import org.opensearch.rest.action.admin.indices.AliasesNotFoundException;
import org.opensearch.test.OpenSearchTestCase;
import org.opensearch.test.VersionUtils;

Expand Down Expand Up @@ -164,11 +165,11 @@ public void testMustExist() {

// Show that removing non-existing alias with mustExist == true fails
final ClusterState finalCS = after;
final IllegalArgumentException iae = expectThrows(
IllegalArgumentException.class,
final AliasesNotFoundException iae = expectThrows(
AliasesNotFoundException.class,
() -> service.applyAliasActions(finalCS, singletonList(new AliasAction.Remove(index, "test_2", true)))
);
assertThat(iae.getMessage(), containsString("required alias [test_2] does not exist"));
assertThat(iae.getMessage(), containsString("aliases [test_2] missing"));
}

public void testMultipleIndices() {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -102,6 +102,11 @@ public static AliasActions randomAliasAction(boolean useStringAsFilter) {
action.isHidden(randomBoolean());
}
}
if (action.actionType() == AliasActions.Type.REMOVE) {
if (randomBoolean()) {
action.mustExist(randomBoolean());
}
}
return action;
}

Expand Down
Loading