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.

Sign up for GitHub

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

Add Get Aliases API to the high-level REST client #28799

Merged
merged 27 commits into from
Jun 12, 2018
Merged

Add Get Aliases API to the high-level REST client #28799

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
27 commits
Select commit Hold shift + click to select a range
6c01eeb
Add Get Aliases API to the high-level RESR client
olcbean Feb 22, 2018
1571cb3
next iteration : still work needed in GetAliasesResponseTests
olcbean Mar 19, 2018
a33ea0f
addressing some comments
olcbean Apr 10, 2018
a919d73
Merge remote-tracking branch 'origin/master' into hlrc_get_alias
olcbean Apr 13, 2018
baed505
chmod
olcbean Apr 13, 2018
f74d0b5
Merge branch 'master' into hlrc_get_alias
javanna May 11, 2018
7720b71
addressing reviewers comments
olcbean May 18, 2018
7238098
addressing reviewers comments
olcbean May 22, 2018
e2ffdab
insert random data into AliasMetaData
olcbean May 23, 2018
9a2079f
adding serialization bwc tests
olcbean May 25, 2018
df3b2fb
adding a serialization test to verify that a response serialized by 6…
olcbean May 28, 2018
0e3ad61
clean up
olcbean May 29, 2018
519992b
clean up
olcbean May 29, 2018
11811ef
Merge branch 'master' into hlrc_get_alias
javanna May 30, 2018
474238b
Merge branch 'master' into hlrc_get_alias
javanna May 30, 2018
a59a026
Merge branch 'master' into hlrc_get_alias
javanna May 31, 2018
b210f49
Merge branch 'master' into hlrc_get_alias
javanna May 31, 2018
5b3096b
Merge branch 'master' into hlrc_get_alias
javanna Jun 5, 2018
8660d43
fix bw comp issues
javanna Jun 5, 2018
f21b3ca
remove unclear if
javanna Jun 5, 2018
fa9d4e1
introduce client specific GetAliasesResponse
javanna Jun 5, 2018
939ae4d
Merge branch 'master' into hlrc_get_alias
javanna Jun 6, 2018
035d823
address review comments
javanna Jun 6, 2018
be1daf7
Merge branch 'master' into hlrc_get_alias
javanna Jun 6, 2018
5da9ed6
fix javadocs
javanna Jun 7, 2018
5bcc1ce
address review comments
javanna Jun 11, 2018
2e2217a
Merge branch 'master' into hlrc_get_alias
javanna Jun 11, 2018
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
45 changes: 27 additions & 18 deletions client/rest-high-level/src/test/java/org/elasticsearch/client/IndicesClientIT.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -50,10 +50,10 @@
import org.elasticsearch.action.admin.indices.refresh.RefreshResponse;
import org.elasticsearch.action.admin.indices.rollover.RolloverRequest;
import org.elasticsearch.action.admin.indices.rollover.RolloverResponse;
import org.elasticsearch.action.admin.indices.settings.put.UpdateSettingsRequest;
import org.elasticsearch.action.admin.indices.settings.put.UpdateSettingsResponse;
import org.elasticsearch.action.admin.indices.settings.get.GetSettingsRequest;
import org.elasticsearch.action.admin.indices.settings.get.GetSettingsResponse;
import org.elasticsearch.action.admin.indices.settings.put.UpdateSettingsRequest;
import org.elasticsearch.action.admin.indices.settings.put.UpdateSettingsResponse;
import org.elasticsearch.action.admin.indices.shrink.ResizeRequest;
import org.elasticsearch.action.admin.indices.shrink.ResizeResponse;
import org.elasticsearch.action.admin.indices.shrink.ResizeType;
Expand Down Expand Up @@ -846,33 +846,42 @@ public void testGetAliasesNonExistentIndexOrAlias() throws IOException {
String index = "index";
{
GetAliasesRequest getAliasesRequest = new GetAliasesRequest().indices(index);
ElasticsearchException exception = expectThrows(ElasticsearchException.class,
() -> execute(getAliasesRequest, highLevelClient().indices()::getAlias, highLevelClient().indices()::getAliasAsync));
assertThat(exception.status(), equalTo(RestStatus.NOT_FOUND));
assertThat(exception.getMessage(), equalTo("Elasticsearch exception [type=index_not_found_exception, reason=no such index]"));
GetAliasesResponse getAliasesResponse = execute(getAliasesRequest, highLevelClient().indices()::getAlias,
highLevelClient().indices()::getAliasAsync);
assertThat(getAliasesResponse.status(), equalTo(RestStatus.NOT_FOUND));
assertThat(getAliasesResponse.errorMessage(),
equalTo("Elasticsearch exception [type=index_not_found_exception, reason=no such index]"));
}
{
GetAliasesRequest getAliasesRequest = new GetAliasesRequest(alias);
ElasticsearchException exception = expectThrows(ElasticsearchException.class,
() -> execute(getAliasesRequest, highLevelClient().indices()::getAlias, highLevelClient().indices()::getAliasAsync));
assertThat(exception.status(), equalTo(RestStatus.NOT_FOUND));
assertThat(exception.getMessage(), equalTo("Elasticsearch exception [type=exception, reason=alias [" + alias + "] missing]"));
GetAliasesResponse getAliasesResponse = execute(getAliasesRequest, highLevelClient().indices()::getAlias,
highLevelClient().indices()::getAliasAsync);
assertThat(getAliasesResponse.status(), equalTo(RestStatus.NOT_FOUND));
assertThat(getAliasesResponse.errorMessage(), equalTo("alias [" + alias + "] missing"));
}
createIndex(index, Settings.EMPTY);
client().performRequest(HttpPut.METHOD_NAME, index + "/_alias/" + alias);
{
GetAliasesRequest getAliasesRequest = new GetAliasesRequest().indices(index, "non_existent_index");
ElasticsearchException exception = expectThrows(ElasticsearchException.class,
() -> execute(getAliasesRequest, highLevelClient().indices()::getAlias, highLevelClient().indices()::getAliasAsync));
assertThat(exception.status(), equalTo(RestStatus.NOT_FOUND));
assertThat(exception.getMessage(), equalTo("Elasticsearch exception [type=index_not_found_exception, reason=no such index]"));
GetAliasesResponse getAliasesResponse = execute(getAliasesRequest, highLevelClient().indices()::getAlias,
highLevelClient().indices()::getAliasAsync);
assertThat(getAliasesResponse.status(), equalTo(RestStatus.NOT_FOUND));
assertThat(getAliasesResponse.errorMessage(),
equalTo("Elasticsearch exception [type=index_not_found_exception, reason=no such index]"));
}
{
GetAliasesRequest getAliasesRequest = new GetAliasesRequest().indices(index, "non_existent_index").aliases(alias);
ElasticsearchException exception = expectThrows(ElasticsearchException.class,
() -> execute(getAliasesRequest, highLevelClient().indices()::getAlias, highLevelClient().indices()::getAliasAsync));
assertThat(exception.status(), equalTo(RestStatus.NOT_FOUND));
assertThat(exception.getMessage(), equalTo("Elasticsearch exception [type=index_not_found_exception, reason=no such index]"));
GetAliasesResponse getAliasesResponse = execute(getAliasesRequest, highLevelClient().indices()::getAlias,
highLevelClient().indices()::getAliasAsync);
assertThat(getAliasesResponse.status(), equalTo(RestStatus.NOT_FOUND));
assertThat(getAliasesResponse.errorMessage(),
equalTo("Elasticsearch exception [type=index_not_found_exception, reason=no such index]"));
}
{
GetAliasesRequest getAliasesRequest = new GetAliasesRequest().indices("non_existent_index*");
GetAliasesResponse getAliasesResponse = execute(getAliasesRequest, highLevelClient().indices()::getAlias,
highLevelClient().indices()::getAliasAsync);
assertThat(getAliasesResponse.getAliases().size(), equalTo(0));
}
Copy link
Member

Choose a reason for hiding this comment

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

I noticed that when a wildcard expressions is provided for the indices, and no indices match, we return 200 and an empty json object. Can we test that too?

curl 'localhost:9200/nothing*/_alias?pretty'
{ }

{
GetAliasesRequest getAliasesRequest = new GetAliasesRequest().indices(index).aliases(alias, "non_existent_alias");
Expand Down
69 changes: 26 additions & 43 deletions ...er/src/main/java/org/elasticsearch/action/admin/indices/alias/get/GetAliasesResponse.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,6 @@
import com.carrotsearch.hppc.cursors.ObjectObjectCursor;

import org.elasticsearch.ElasticsearchException;
import org.elasticsearch.ElasticsearchStatusException;
import org.elasticsearch.Version;
import org.elasticsearch.action.ActionResponse;
import org.elasticsearch.cluster.metadata.AliasMetaData;
Expand Down Expand Up @@ -51,23 +50,16 @@ public class GetAliasesResponse extends ActionResponse implements StatusToXConte

private ImmutableOpenMap<String, List<AliasMetaData>> aliases = ImmutableOpenMap.of();
private RestStatus status = RestStatus.OK;
private String errorMsg = "";
private String errorMessage;

public GetAliasesResponse(ImmutableOpenMap<String, List<AliasMetaData>> aliases, RestStatus status, String errorMsg) {
public GetAliasesResponse(ImmutableOpenMap<String, List<AliasMetaData>> aliases, RestStatus status, String errorMessage) {
this.aliases = aliases;
if (status == null) {
this.status = RestStatus.OK;
}
this.status = status;
if (errorMsg == null) {
this.errorMsg = "";
} else {
this.errorMsg = errorMsg;
}
this.status = status == null ? RestStatus.OK : status;
Copy link
Member

Choose a reason for hiding this comment

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

when do we get null here? I would prefer to have this logic (default is OK, but also 404 can be provided) in one place, while we have it now in the transport action too. I wonder when this if is needed.

Copy link
Member

Choose a reason for hiding this comment

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

did you have a chance to look into this super small comment?

this.errorMessage = errorMessage;
}

public GetAliasesResponse(ImmutableOpenMap<String, List<AliasMetaData>> aliases) {
Copy link
Member

Choose a reason for hiding this comment

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

would it be possible to remove this constructor? It seems that it's only used in tests and we could rather use the above constructor and provide null arguments to it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I would prefer not to as I do not know whether users are using it or not ( it is part of the public API ... )

this(aliases, RestStatus.OK, "");
this(aliases, RestStatus.OK, null);
}

GetAliasesResponse() {
Expand All @@ -82,13 +74,13 @@ public ImmutableOpenMap<String, List<AliasMetaData>> getAliases() {
return aliases;
}

public String errorMsg() {
return errorMsg;
public String errorMessage() {
return errorMessage;
}

@Override
public String toString() {
return Strings.toString(this, true, true) + ", status:" + status + ", errorMsg:\"" + errorMsg + "\"";
return Strings.toString(this, true, true);
}

@Override
Expand All @@ -107,10 +99,10 @@ public void readFrom(StreamInput in) throws IOException {
}
aliases = aliasesBuilder.build();
if (in.getVersion().onOrAfter(Version.V_7_0_0_alpha1)) {
// if (in.getVersion().onOrAfter(Version.V_6_3_0)) {
// if (in.getVersion().onOrAfter(Version.V_6_4_0)) {
status = RestStatus.fromCode(in.readInt());
Copy link
Member

Choose a reason for hiding this comment

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

should we use RestStatus#readFrom and RestStatus#writeTo instead?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Interesting, RestStatus#readFrom is serializing the status as a string ;)

if (in.readBoolean()) {
status = RestStatus.fromCode(in.readInt());
errorMsg = in.readString();
errorMessage = in.readString();
}
Copy link
Member

@javanna javanna May 29, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

nit: readOptionalString?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

🤦

}
}
Expand All @@ -127,11 +119,11 @@ public void writeTo(StreamOutput out) throws IOException {
}
}
if (out.getVersion().onOrAfter(Version.V_7_0_0_alpha1)) {
// if (out.getVersion().onOrAfter(Version.V_6_3_0)) {
if (status != RestStatus.OK) {
// if (out.getVersion().onOrAfter(Version.V_6_4_0)) {
out.writeInt(status.getStatus());
if (null != errorMessage) {
out.writeBoolean(true);
out.writeInt(status.getStatus());
out.writeString(errorMsg);
out.writeString(errorMessage);
Copy link
Member

Choose a reason for hiding this comment

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

use out.writeOptionalString here?

} else {
out.writeBoolean(false);
Copy link
Member

Choose a reason for hiding this comment

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

why the boolean flag? we could otherwise always serialize the status and serialize the error as an optional string (as I asked above to accept null as well)?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The boolean flag signals if a status and a string are following.
In this case we can always serialized the status and the error message as an optional String. The advantage of using the boolean flag is that new fields can be easily appended to the response.
Basically if in a later version the response needs to be extended with another optional String, it would be virtually impossible to distinguish if the String is 'errorMessage' or the new String field.

}
Expand All @@ -149,15 +141,15 @@ public boolean equals(Object o) {
GetAliasesResponse that = (GetAliasesResponse) o;
return Objects.equals(fromListAliasesToSet(aliases), fromListAliasesToSet(that.aliases))
&& Objects.equals(status, that.status)
&& Objects.equals(errorMsg, that.errorMsg);
&& Objects.equals(errorMessage, that.errorMessage);
}

@Override
public int hashCode() {
return Objects.hash(fromListAliasesToSet(aliases), status, errorMsg);
return Objects.hash(fromListAliasesToSet(aliases), status, errorMessage);
}

private ImmutableOpenMap<String, Set<AliasMetaData>> fromListAliasesToSet(ImmutableOpenMap<String, List<AliasMetaData>> list) {
private static ImmutableOpenMap<String, Set<AliasMetaData>> fromListAliasesToSet(ImmutableOpenMap<String, List<AliasMetaData>> list) {
ImmutableOpenMap.Builder<String, Set<AliasMetaData>> builder = ImmutableOpenMap.builder();
list.forEach(e -> builder.put(e.key, new HashSet<>(e.value)));
return builder.build();
Expand All @@ -182,12 +174,12 @@ public XContentBuilder toXContent(XContentBuilder builder, Params params) throws
builder.startObject();
{
if (status != null && RestStatus.OK != status) {
builder.field("error", errorMsg);
builder.field("error", errorMessage);
builder.field("status", status.getStatus());
}

for (final ObjectObjectCursor<String, List<AliasMetaData>> entry : aliases) {
if (namesProvided == false || indicesToDisplay.contains(entry.key)) {
if (false == namesProvided || indicesToDisplay.contains(entry.key)) {
builder.startObject(entry.key);
{
builder.startObject("aliases");
Expand Down Expand Up @@ -215,11 +207,10 @@ public static GetAliasesResponse fromXContent(XContentParser parser) throws IOEx

String currentFieldName;
Token token;
String exceptionMsg = null;
String exceptionMessage = null;
RestStatus status = RestStatus.OK;
ElasticsearchException exception = null;

while ((token = parser.nextToken()) != Token.END_OBJECT) {
while (parser.nextToken() != Token.END_OBJECT) {
if (parser.currentToken() == Token.FIELD_NAME) {
currentFieldName = parser.currentName();

Expand All @@ -231,10 +222,10 @@ public static GetAliasesResponse fromXContent(XContentParser parser) throws IOEx
} else if ("error".equals(currentFieldName)) {
if ((token = parser.nextToken()) != Token.FIELD_NAME) {
Copy link
Member

Choose a reason for hiding this comment

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

this resembles what ElasticsearchException#failureFromXContent does. but it will build an exception in the first case with a string message only. That's why I was hoping that we could use an exception in every case, but I don't think it would work at transport, as such exception would be serialized then completely different when calling toXContent on it. There's generateFailureXContent that is similar to what we would need, yet not quite what we need. I think what you have is good, unless you want to reuse the parsing code that we already have and then take the string out of the parsed exception which is what you need. Not sure really.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the tip! I will have a look and if I can reuse the failureFromXContent or generateFailureXContent.

if (token == Token.VALUE_STRING) {
exceptionMsg = parser.text();
exceptionMessage = parser.text();
} else if (token == Token.START_OBJECT) {
token = parser.nextToken();
exception = ElasticsearchException.innerFromXContent(parser, true);
parser.nextToken();
exceptionMessage = ElasticsearchException.innerFromXContent(parser, true).getMessage();
}
}
} else {
Expand All @@ -246,15 +237,7 @@ public static GetAliasesResponse fromXContent(XContentParser parser) throws IOEx
}
}
}
if (exception != null) {
throw new ElasticsearchStatusException(exception.getMessage(), status, exception.getCause());
}
if (RestStatus.OK != status && aliasesBuilder.isEmpty()) {
throw new ElasticsearchStatusException(exceptionMsg, status);
}
GetAliasesResponse getAliasesResponse = new GetAliasesResponse(aliasesBuilder.build(), status, exceptionMsg);

return getAliasesResponse;
return new GetAliasesResponse(aliasesBuilder.build(), status, exceptionMessage);
}

private static List<AliasMetaData> parseAliases(XContentParser parser) throws IOException {
Expand Down
18 changes: 6 additions & 12 deletions ...main/java/org/elasticsearch/action/admin/indices/alias/get/TransportGetAliasesAction.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -78,8 +78,8 @@ protected void masterOperation(GetAliasesRequest request, ClusterState state, Ac
String[] concreteIndices = indexNameExpressionResolver.concreteIndexNames(state, request);
ImmutableOpenMap<String, List<AliasMetaData>> result = state.metaData().findAliases(request.aliases(), concreteIndices);

SetOnce<String> message = new SetOnce<>();
SetOnce<RestStatus> status = new SetOnce<>();
String message = null;
RestStatus status = RestStatus.OK;
if (false == Strings.isAllOrWildcard(request.aliases())) {
String[] aliasesNames = Strings.EMPTY_ARRAY;

Expand Down Expand Up @@ -113,21 +113,15 @@ protected void masterOperation(GetAliasesRequest request, ClusterState state, Ac

difference.removeAll(matches);
if (false == difference.isEmpty()) {
Copy link
Member

Choose a reason for hiding this comment

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

I think that I would find this more readable if we declared message and status down here and assigned them in an if/else. In that case I would go back to not using SetOnce.

status.set(RestStatus.NOT_FOUND);
status = RestStatus.NOT_FOUND;
if (difference.size() == 1) {
message.set(String.format(Locale.ROOT, "alias [%s] missing", toNamesString(difference.iterator().next())));
message = String.format(Locale.ROOT, "alias [%s] missing", toNamesString(difference.iterator().next()));
} else {
message.set(String.format(Locale.ROOT, "aliases [%s] missing", toNamesString(difference.toArray(new String[0]))));
message = String.format(Locale.ROOT, "aliases [%s] missing", toNamesString(difference.toArray(new String[0])));
}
}
}
if (status.get() == null) {
status.set(RestStatus.OK);
}
if (message.get() == null) {
message.set("");
}
listener.onResponse(new GetAliasesResponse(result, status.get(), message.get()));
listener.onResponse(new GetAliasesResponse(result, status, message));
}

private static String toNamesString(final String... names) {
Copy link
Member

Choose a reason for hiding this comment

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

nit: would it be possible to use Strings.collectionToCommaDelimitedString(difference) instead ?

Expand Down
Loading