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

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

Merged
merged 27 commits into from
Jun 12, 2018

Conversation

olcbean
Copy link
Contributor

@olcbean olcbean commented Feb 23, 2018

Add Get Aliases API to the high-level REST client

Relates to #27205

@elasticmachine
Copy link
Collaborator

Since this is a community submitted pull request, a Jenkins build has not been kicked off automatically. Can an Elastic organization member please verify the contents of this patch and then kick off a build manually?

1 similar comment
@elasticmachine
Copy link
Collaborator

Since this is a community submitted pull request, a Jenkins build has not been kicked off automatically. Can an Elastic organization member please verify the contents of this patch and then kick off a build manually?

@olcbean
Copy link
Contributor Author

olcbean commented Feb 23, 2018

@javanna could you have a look if I am on the right track with the get aliases.
Thanks!

return builder.build();
}

Set<String> indices = new HashSet<>();
Copy link
Contributor Author

Choose a reason for hiding this comment

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

pls ignore for now, still incomplete

Copy link
Member

@javanna javanna left a comment

Choose a reason for hiding this comment

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

hi @olcbean I left some comments but you are definitely on the right track. Thanks!

}

@Override
public XContentBuilder toXContent(XContentBuilder builder, Params params) throws IOException {
Copy link
Member

Choose a reason for hiding this comment

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

As odd as this may sound, I think that we need to add name to the request (similar to what we do with flatSettings in some other requests, otherwise there is no way to filter aliases out when calling this API using the high-level REST client? That should also help making this toXContent method more sane, I think.

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 do not think it is necessary. The filtering can be done by specifying the aliases in the request #aliases(String[]) , then they can be serialized as part of the url /<indices>/_alias/<aliases> and the rest layer will parse them as the name.

I introduce a setName in the request so you can have a look. Now there are two ways to 'filter' the returned aliases ( by specifying the aliases or the name )

Copy link
Member

Choose a reason for hiding this comment

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

you are right, it is not necessary!

String[] result = Strings.splitStringByCommaToArray(params.param("name"));
if (false == Strings.isAllOrWildcard(result)) {
aliasesNames = result;
}
Copy link
Member

Choose a reason for hiding this comment

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

with my suggestiona above, some of this should go back to the rest action where you can go back to calling e.g. paramAsStringArrayOrEmptyIfAll which are not available here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If this goes back to the RestAction then the name param will need to be modified ( by getting _all and * out ) and then simply access it here ? Or did you mean something else ?

Copy link
Member

Choose a reason for hiding this comment

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

ignore me :)

}
} else {
parser.skipChildren();
}
Copy link
Member

Choose a reason for hiding this comment

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

don't we need to also skipChildren if we find START_ARRAY?

@@ -41,7 +41,7 @@

import static java.util.Collections.emptySet;

public class AliasMetaData extends AbstractDiffable<AliasMetaData> {
public class AliasMetaData extends AbstractDiffable<AliasMetaData> implements ToXContent{
Copy link
Member

Choose a reason for hiding this comment

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

can we go for either ToXContentFragment or ToXContentObject?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

😅


@Override
protected Predicate<String> getRandomFieldsExcludeFilter() {
return p -> p.equals("") // do not add new indices
Copy link
Member

Choose a reason for hiding this comment

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

when is the field name empty? at the root of the object?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

For the top elements. Basically if it is not excluded, the elements added at this level are interpreted as a new index and as the index name is a free text field name, I cannot ignore it in the fromXContent.

Copy link
Member

Choose a reason for hiding this comment

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

makes sense. maybe expand a bit the comment?

@olcbean
Copy link
Contributor Author

olcbean commented Mar 20, 2018

@javanna I just pushed another commit, although the test are still incomplete. However I stumbled on some inconsistencies I like to get your thoughts about. Please see question inline ;)

}
}
}
*/
Copy link
Contributor Author

Choose a reason for hiding this comment

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

That is actually what ES Rest layer is returning in this case... And the high level client cannot parse the exception body here.
Is this the only action which wraps the response into the returned exception ? Should I try to parse the response here ?

Copy link
Member

Choose a reason for hiding this comment

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

this is a funny response, as the error may be mixed up with an actual response that holds aliases. We have similar situations in a few other places, for instance in GetResponse where the 404 for a document not found is actually a valid GetResponse rather than an exception. But this case is even weirder, as all the processing happens on the REST layer and the response itself doesn't have a place to store the exception if we wanted to parse it back. I wonder if it would be possible to move this logic to the transport action, and have a lightweight response with much less logic, which holds the final status code and eventual exceptions if there are along with the current content. That would make this request more similar to other cases where for instance we have results together with shard failures that are after all exceptions. What do you think?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Do you mean to change the response to something like :

{
  failures : { .... },
  "<index name>" : { ... }, 
  "<index name>" : { ... }
}

return 200 and ask the clients to parse the failures to see if there were exceptions ?
Or keep as it currently is and move the filtering logic to the transport?

For moving the logic to the transport, I am not really sure if it is possible. There was an issue and a fix which got reverted because of side effects ( issue : #27763, reverted PR : #28294 ).

Copy link
Member

@javanna javanna Mar 22, 2018

Choose a reason for hiding this comment

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

I mean to leave the final output the same but to refactor the response object. The PR that was reverted was changing things even deeper in the MetaData class. I am not sure if this change would end up causing the same problems, I hope not but it may be the case. I see there are subtleties that yield to a different output when comparing the transport client response with what we print out on the REST layer. Maybe then we need to keep what goes in the response object exactly the same, still adding room for errors and computing the status in the transport action, but leaving the filtering in the toXContent method. That should be possible but this is indeed tricky.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

hey @javanna, sorry I didn't get back on this one sooner.

If the status ( and the error msg ) are calculated on the transport layer, then they shoud also be serialized, shouldn't they ?

Copy link
Member

Choose a reason for hiding this comment

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

no worries, yes additional info added as part of the transport action execution should be serialized over transport.

} else {
params.withName(getAliasesRequest.name());
}
String endpoint = endpoint(optional(getAliasesRequest.indices(), "_all"), "_alias");
Copy link
Contributor Author

Choose a reason for hiding this comment

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

If there are no indices and no aliases specified ( /_alias ), then GetAllAliases action will triggered ( which returns a different response ). By using _all when there are no indices, we can use the GetAliases instead and get the same results ;)

Copy link
Member

Choose a reason for hiding this comment

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

I see! but although it is a different response on the REST layer, could its output be parsed into a GetAliasesResponse ?I think I would prefer if we go that way, to decouple the client from the server logic. As a client we hit the _alias endpoint, which is described in indices.get_alias spec file and has no required parameter. In the server though, depending on the parameters we will hit a different REST action (not sure why we need that, I find this confusing).

Copy link
Member

@javanna javanna left a comment

Choose a reason for hiding this comment

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

left a few comments thanks for your patience @olcbean , this is a tough one

} else {
params.withName(getAliasesRequest.name());
}
String endpoint = endpoint(optional(getAliasesRequest.indices(), "_all"), "_alias");
Copy link
Member

Choose a reason for hiding this comment

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

I see! but although it is a different response on the REST layer, could its output be parsed into a GetAliasesResponse ?I think I would prefer if we go that way, to decouple the client from the server logic. As a client we hit the _alias endpoint, which is described in indices.get_alias spec file and has no required parameter. In the server though, depending on the parameters we will hit a different REST action (not sure why we need that, I find this confusing).

if (false == CollectionUtils.isEmpty(getAliasesRequest.aliases())) {
params.withName(getAliasesRequest.aliases());
} else {
params.withName(getAliasesRequest.name());
Copy link
Member

Choose a reason for hiding this comment

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

I don't think we need this branch after all :) we can get rid of withName too, I didn't see that name goes into the request already as part of aliases.

return optional(params, null);
}

private static String[] optional(String[] params, String defaultParam) {
Copy link
Member

Choose a reason for hiding this comment

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

ideally we wouldn't need these two methods, we would just pass in the empty array, which makes us hit a different endpoint that returns a compatible response.

assertThat(exception.status(), equalTo(RestStatus.NOT_FOUND));
assertThat(exception.getMessage(), equalTo("Elasticsearch exception [type=index_not_found_exception, reason=no such index]"));

GetAliasesRequest getAliasesRequest2 = new GetAliasesRequest(alias);
Copy link
Member

Choose a reason for hiding this comment

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

would you mind wrapping these checks into curly brackets so you can declare again the exception variable as a new one and we make sure that there are no interactions between the different checks?

}
}
}
*/
Copy link
Member

Choose a reason for hiding this comment

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

this is a funny response, as the error may be mixed up with an actual response that holds aliases. We have similar situations in a few other places, for instance in GetResponse where the 404 for a document not found is actually a valid GetResponse rather than an exception. But this case is even weirder, as all the processing happens on the REST layer and the response itself doesn't have a place to store the exception if we wanted to parse it back. I wonder if it would be possible to move this logic to the transport action, and have a lightweight response with much less logic, which holds the final status code and eventual exceptions if there are along with the current content. That would make this request more similar to other cases where for instance we have results together with shard failures that are after all exceptions. What do you think?

* @param name value of the aliases names.
* @return this request
*/
public GetAliasesRequest name(String[] name) {
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 this can go away after all, unless I am missing something :)

}

@Override
public XContentBuilder toXContent(XContentBuilder builder, Params params) throws IOException {
Copy link
Member

Choose a reason for hiding this comment

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

you are right, it is not necessary!

String[] result = Strings.splitStringByCommaToArray(params.param("name"));
if (false == Strings.isAllOrWildcard(result)) {
aliasesNames = result;
}
Copy link
Member

Choose a reason for hiding this comment

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

ignore me :)


private ImmutableOpenMap<String, List<AliasMetaData>> aliases = ImmutableOpenMap.of();
private RestStatus status;
Copy link
Member

Choose a reason for hiding this comment

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

it is pretty bad that the status is mutated once toXContent is called. I think moving some of the toXContent logic to the transport action would also help this, as status would ideally become a constructor argument. As far as I can see, the transport client's behaviour should not be affected by this.


@Override
protected Predicate<String> getRandomFieldsExcludeFilter() {
return p -> p.equals("") // do not add new indices
Copy link
Member

Choose a reason for hiding this comment

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

makes sense. maybe expand a bit the comment?

@olcbean olcbean changed the title [WIP] Add Get Aliases API to the high-level REST client Add Get Aliases API to the high-level REST client Apr 13, 2018
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]"));
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 am somehow stuck with this response in case of an error. I do not see an option which "makes sense".

A request with non existent alias results in :

{
  "error": "alias [<alias>] missing",
  "status": 404
}

For now I decided to map it to an exception. Basically if there are no aliases found then throw. If at least one alias is found, a response will be returned and its status must be checked ( to verify if all aliases have been found : OK , or if this is a "partial" response : NOT_FOUND)

Another option is to throw only if any of the indices do not exist. And for the aliases always to return a response ( even if none of the specified aliases exist ).

Yet another option is to never throw... ( which is not is sync with the rest of the apis ... )

Ideas how to proceed ?

Copy link
Member

Choose a reason for hiding this comment

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

this is a weird response. I think there isn't a perfect solution. I initially was going for your solution, only throw an error when no aliases are returned. But that way that there is a 404 that's ignored and a 404 that leads to an exception being thrown, which is confusing. Also, it makes the code messy as we end up having to ignore 404 in the client, yet we need to throw in some specific case exception in fromXContent. I would then go for never throwing. It is always possible to retrieve the returned status and error message, so there is no missing info. And I will open an issue to discuss how we can fix this response as a follow-up.

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 get alias response is really confusing ;)

To be honest, I went with the least disruption for the users I could come up with : parse the response if there are any aliases returned ( even if the response contains only 'partial' results and use the additional info such as error message and status if need be ). Confusing and hard to maintain on the server side, but easier for the users ;)

If 404 is ignored and no exception is thrown, then both an error message and an ES exception will need to be stored in the response object ;) and the users will need to first check the status and if it is 404, then check if there is an ES exception or a string exception with partial results.

@javanna shall I give this approach a try or should we wait for some directions from #30536 ?

Copy link
Member

Choose a reason for hiding this comment

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

no let's not wait, I would get this one in, for now let's never throw exception, users can read errors if they wish.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@javanna one more thing: if ES exception is added to the response, should it be serialized on the transport layer ? Maybe it would be better not to serialize it on the transport layer, but then the response tests could get tricky and maybe they will need to be split ( something similar to what has been done in #28892 )

Copy link
Member

Choose a reason for hiding this comment

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

I am not sure. At the moment we only have a string and a status, are you going for recreating an exception based on that info?

Copy link
Member

@javanna javanna left a comment

Choose a reason for hiding this comment

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

thanks a lot @olcbean I left a bunch of comments but this is not far at all, just a tricky one to figure out. Sorry about my delay in getting back to you. I took the liberty to merge master in and resolve conflicts. Let me know if I can help moving this forward.


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

Choose a reason for hiding this comment

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

maybe we should not call Strings.toString here, rather print what we want to print but not in json format? Seems risky to concatenate stuff like this hoping it will be valid json.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

toString() is not guaranteed to return a valid json, is it ? But you are right, I can clean this up.

() -> 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]"));
}
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'
{ }

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]"));
Copy link
Member

Choose a reason for hiding this comment

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

this is a weird response. I think there isn't a perfect solution. I initially was going for your solution, only throw an error when no aliases are returned. But that way that there is a 404 that's ignored and a 404 that leads to an exception being thrown, which is confusing. Also, it makes the code messy as we end up having to ignore 404 in the client, yet we need to throw in some specific case exception in fromXContent. I would then go for never throwing. It is always possible to retrieve the returned status and error message, so there is no missing info. And I will open an issue to discuss how we can fix this response as a follow-up.

}

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.OK);
}
if (message.get() == null) {
message.set("");
Copy link
Member

Choose a reason for hiding this comment

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

why do we set it to an empty string? I think in this case null would be ok?

out.writeInt(status.getStatus());
out.writeString(errorMsg);
} 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.

return Objects.hash(fromListAliasesToSet(aliases), status, errorMsg);
}

private ImmutableOpenMap<String, Set<AliasMetaData>> fromListAliasesToSet(ImmutableOpenMap<String, List<AliasMetaData>> list) {
Copy link
Member

Choose a reason for hiding this comment

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

make it static? is this conversion needed because we lose ordering of the aliases though they are stored in a list?

status = RestStatus.fromCode(parser.intValue());
}
} 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 (RestStatus.OK != status && aliasesBuilder.isEmpty()) {
throw new ElasticsearchStatusException(exceptionMsg, status);
}
Copy link
Member

Choose a reason for hiding this comment

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

shouldn't this one already be covered by the previous if?

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 previous if is covering the case when an ES exception has been parsed ( a concrete index was not found ). This if is covering the case, when a string exception has been generated ( the concrete alias could not be found ).

assertThat(expectThrows.status(), equalTo(RestStatus.NOT_FOUND));
assertThat(expectThrows.getMessage(), equalTo("alias [aa] missing"));
}

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 we should also have some serialization test that cover the bw comp changes we are making.

@javanna
Copy link
Member

javanna commented May 11, 2018

Opened #30536 as a follow-up to discuss whether we want to make changes to the response of this API.

@olcbean
Copy link
Contributor Author

olcbean commented May 18, 2018

@javanna can you have another look ?

Copy link
Member

@javanna javanna left a comment

Choose a reason for hiding this comment

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

thanks a lot @olcbean I left a couple more comments, LGTM otherwise

} 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.errorMsg = errorMsg;
}
this.status = status == null ? RestStatus.OK : status;
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 ... )

@@ -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 ;)

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?

// only if the status is not OK, then there is an error msg in the response body
String errorMsg = RestStatus.OK == status ? (randomBoolean() ? null : "") : randomAlphaOfLengthBetween(0, 10);
return new GetAliasesResponse(createIndicesAliasesMap(0, 0).build(), status, errorMsg);
RestStatus status = randomFrom(/*RestStatus.OK, */RestStatus.NOT_FOUND);
Copy link
Member

Choose a reason for hiding this comment

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

is the commented out part some leftover?

protected Predicate<String> getRandomFieldsExcludeFilter() {
return p -> p.equals("") // do not add elements at the top-level as any element at this level is parsed as a new index
|| p.endsWith(".aliases") // do not add new alias
|| p.contains(".aliases."); // do not be testing the AlilasMetaData.fromContent
Copy link
Member

Choose a reason for hiding this comment

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

I am not following. If you test GetAliasesResponse, you end up testing everything that it holds as well? It may be a good idea to test some parts separately, but I think that should not affect the exclusions here?

@olcbean
Copy link
Contributor Author

olcbean commented Jun 5, 2018

Thanks @javanna, I also can reproduce the issue ( not 100% but quite reliably )

I think that the transport logic that checks whether every concrete alias is found ( and if not sets the status and the error message ) will need to go back to the rest layer #toXContent. The problem is that if an alias cannot be found on the transport layer, the status is set to NOT_FOUND. However if the response is synchronized by a 6.x node, the status is not written out and now a 7.0 node is interpreting the missing status as OK.

If the check is done by #toXContent this should be avoidable.

Let me know if you have any other idea ?

@javanna
Copy link
Member

javanna commented Jun 5, 2018

I managed to repro as well @olcbean . The initial issue was an NPE when reading the status in case it was not received, that would happen in a mixed cluster when you hit the newer node and the master is on the older version. The coordinating node would try to find out which status to return, hitting an NPE as the status was not serialized as part of the response, and would end up returning a valid response body but with 500 status code, quite odd. Tracing this was quite tricky.

Once I fixed that there are still failures on a mixed cluster, on one hand expected, on the other hand quite weird as status and error message may not be returned at all. Especially the status is a tough one as we may end up returning 200 from one node but 404 from another one.
I also realized that moving some of the logic to the transport action breaks things when x-pack is installed, because retrieving the requested aliases through request.aliases() is not exactly the same as reading them at REST from the RestRequest.
We may be able to work around these issues having some logic in the toXContent method, but then we would need to mutate the response once such method is called which is far from ideal.

This made me look back at why I asked you to move the logic from the REST layer to the transport layer. I think it was a mistake on my end, it made the PR bigger and I didn't realize how much more complicated things would become. I am not sure that making such change is worthwhile given the bw implications, and if we were to make it we should do so in a separate change. This API is already quite problematic and I feel like we should rework it and evaluate all the implications, but this PR should just add support for it as-is to the REST high-level client.

All that said, I was about to propose that I push what I have been working on to your remote, if you don't mind, and you could review it and help me out getting it in? I am sorry that I made you do all the bw testing, that was great, I should have realized earlier that this PR was going one step too far.

The idea I am experimenting with so far is to not reuse the GetAliasesResponse but rather add a new client specific response object that resembles the REST response better, so we can have status and message without affecting transport and the rest of Elasticsearch.

Also revert non bw compatible changes made to REST action and transport
action
@olcbean
Copy link
Contributor Author

olcbean commented Jun 6, 2018

All that said, I was about to propose that I push what I have been working on to your remote, if you don't mind, and you could review it and help me out getting it in?

Of course, it would be interesting to see what you are experimenting with ;)
What about closing this PR ( as it got rather long with a lot of back and forth ) and opening a new PR ? Or maybe you wanna keep the history at one place ?
I am fine either way.

I am sorry that this one is taking that long but on the bright side it can help to clean the REST a little bit ;)

@javanna
Copy link
Member

javanna commented Jun 6, 2018

I pushed my latest commits to this PR @olcbean , mainly because I reused all of your code, I just moved it around a bit. Could you let me know what you think?

@nik9000 can you have a look as well, we are doing similar to what we did for synced flush. The response is not that complicated here but it's missing a couple of fields that would make things quite complicated if we added them to the original response.

@javanna
Copy link
Member

javanna commented Jun 6, 2018

retest this please

Copy link
Contributor Author

@olcbean olcbean left a comment

Choose a reason for hiding this comment

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

Thanks @javanna, LGTM and the bwc tests should pass now ;)
Left a few nits

The only thing is that now toXContent is implemented twice : in the client and in the rest action. Can we unify them ? Maybe something like toXContent ( status, errorMessage, aliases, builder, params ) that can be called by both

@@ -84,6 +83,8 @@ public RestChannelConsumer prepareRequest(final RestRequest request, final NodeC
getAliasesRequest.indicesOptions(IndicesOptions.fromRequest(request, getAliasesRequest.indicesOptions()));
getAliasesRequest.local(request.paramAsBoolean("local", getAliasesRequest.local()));

//we may want to move this logic to TransportGetAliasesAction but it is based on the original provided aliases, which will
//not always be available there (they may get replaced so retrieving request.aliases is not quite the same).
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Do you mean the conflict with how x-pack is using getAliases ( #29538 )

Copy link
Member

Choose a reason for hiding this comment

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

yea that is something that gets easily overlooked.


import static org.elasticsearch.common.xcontent.XContentParserUtils.ensureExpectedToken;

public class GetAliasesResponse extends ActionResponse implements StatusToXContentObject {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

What about extending the GetAliasesResponse ? I know it does not bring anything but could mark a dependency ( if something is changed on the server side, the REST client needs to be modified as well ... )

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 on the very long term the client will not depend on es core anymore, I was wondering if I should even extend ActionResponse, I would prefer not to extend the original GetAliasesResponse. We do need to figure out how to manage changes, hopefully tests fail if the REST response changes though.

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 am afraid that the tests will not catch a change. The parsing of a response is lenient : if there are unknown fields, no errors ;). What about making the tests stricter (supportsUnknownFields = false) ?

Copy link
Member

Choose a reason for hiding this comment

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

good point, even integration tests would work fine if we add fields. We have to address this (not necessarily in this PR though), I guess we have the same with synced flush which is also implemented with a client specific response. Let's see what Nik thinks about it.

}

@Override
public XContentBuilder toXContent(XContentBuilder builder, Params params) throws IOException {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Can we bind this to the rest action buildResponse ? Otherwise it would be easy to get out of sync if anything changes

Copy link
Member

Choose a reason for hiding this comment

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

this is here only for testing, it is not really needed as the response only needs to be parsed back. I do see how there is some duplication, but this toXContent is much more straight-forward than what the REST action currently does, so I am not sure what to do. how did you mean to bind the two?

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 meant that if code is shared between toXContent and REST buildResponse, then XContentTests is more realistic. I was thinking of something like toXContent ( status, errorMessage, aliases, builder, params ) which can be called after REST layer determined the status and the error message. Just a thought :)

Copy link
Member

Choose a reason for hiding this comment

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

If we wanted to we could move this class to the server and use it to build the response that we return to users. It is a good idea but I prefer not to move this thing into the server.

return new GetAliasesResponse(status, errorMessage, createIndicesAliasesMap(0, 5));
}

private static Map<String, Set<AliasMetaData>> createIndicesAliasesMap(int min, int max) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Maybe reuse the static utils from org.elasticsearch.action.admin.indices.alias.get.GetAliasesResponseTests ?

Copy link
Member

Choose a reason for hiding this comment

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

tried that, but I went for simple maps while the internal class uses ImmutableOpenMap

Copy link
Contributor Author

Choose a reason for hiding this comment

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

🤦 Missed that.
What about the createAliasMetaData and mutateAliasMetaData

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 tried that too but we would need to add a test dependency to get that and I didn't feel like we need it so badly.

@javanna
Copy link
Member

javanna commented Jun 6, 2018

retest this please

Copy link
Member

@nik9000 nik9000 left a comment

Choose a reason for hiding this comment

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

I left a few thoughts. I think we should document


public class GetAliasesResponse extends ActionResponse implements StatusToXContentObject {

private final RestStatus status;
Copy link
Member

Choose a reason for hiding this comment

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

It feels weird to have the status here. Everything else doesn't use it unless they get an exception.

Copy link
Member

Choose a reason for hiding this comment

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

I see that we do this because we don't throw exceptions on 404s because they could contain partial results. I think we should have documentation for this in the asciidoc because it is super uncommon for our APIs to do this.

Copy link
Member

Choose a reason for hiding this comment

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

See #30536. Man when I think of documenting this I think we better spend time on changing it asap :)

}

@Override
public XContentBuilder toXContent(XContentBuilder builder, Params params) throws IOException {
Copy link
Member

Choose a reason for hiding this comment

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

If we wanted to we could move this class to the server and use it to build the response that we return to users. It is a good idea but I prefer not to move this thing into the server.

if ((token = parser.nextToken()) != Token.FIELD_NAME) {
ensureExpectedToken(Token.VALUE_NUMBER, token, parser::getTokenLocation);
status = RestStatus.fromCode(parser.intValue());
}
Copy link
Member

Choose a reason for hiding this comment

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

I see why you can't use ObjectParser here. declareNamedObjects is the closest thing to this but it isn't right.

Could you add error handling for some of this? Like, we should throw an exception if the status comes back as something other than a START_OBJECT I think.

Copy link
Member

Choose a reason for hiding this comment

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

I don't get what you mean, can you elaborate?

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 this if statement confused me:

if ((token = parser.nextToken()) != Token.FIELD_NAME) {

I find myself asking "how can this be a field name and, if it is, what will we do?" Or, "what happens if someone sends and "error": ["an", "array"]. I just get bugged by hand written parsing code because there are so many cases to think through. And I get that we are dealing with the response from the server which we mostly control, but I think we're better safe than sorry here. I think this one is missing some exception handling cases.

Copy link
Member

Choose a reason for hiding this comment

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

ok I see what you mean, in the high-level REST client we are rather lenient, in this case we should either throw or most likely skipChildren if we find a start_array

exceptionMessage = parser.text();
} else if (token == Token.START_OBJECT) {
parser.nextToken();
exceptionMessage = ElasticsearchException.innerFromXContent(parser, true).getMessage();
Copy link
Member

Choose a reason for hiding this comment

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

We don't want the whole exception?

Copy link
Member

Choose a reason for hiding this comment

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

it would be good to throw, but the problem is that this is in reality a 404, so for which 404s do we throw and for which we don't? We went for never throwing then, but the response doesn't have a place to hold the whole exception. Shall we add that at this point? This response is probably going to look even weirder.

Copy link
Member

Choose a reason for hiding this comment

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

one alternative is what @olcbean tried before, that I didn't like at first: throw exception in the fromXContent when we find the complete exception, that is the only way to differentiate between the different 404 responses I think. Let me know what you prefer.

Copy link
Member

Choose a reason for hiding this comment

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

We could save the exception as a member variable. It is weird but the API is weird.

Copy link
Member

Choose a reason for hiding this comment

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

then you would either have the message or the exception?


public void testFromXContentWithElasticsearchException() throws IOException {
String xContent =
"{" +
Copy link
Member

Choose a reason for hiding this comment

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

Can you line the "s up?

Copy link
Member

Choose a reason for hiding this comment

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

❤️

--------------------------------------------------
include-tagged::{doc-tests}/IndicesClientDocumentationIT.java[get-alias-request-indicesOptions]
--------------------------------------------------
<1> Setting `IndicesOptions` controls how unavailable indices are resolved and
Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure "unavailable indices" makes sense here.

Copy link
Member

Choose a reason for hiding this comment

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

why?

Copy link
Member

Choose a reason for hiding this comment

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

Because we're getting aliases, not indices.

@@ -65,5 +65,4 @@ protected void masterOperation(GetAliasesRequest request, ClusterState state, Ac
ImmutableOpenMap<String, List<AliasMetaData>> result = state.metaData().findAliases(request.aliases(), concreteIndices);
listener.onResponse(new GetAliasesResponse(result));
}

Copy link
Member

Choose a reason for hiding this comment

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

Revert this?

@@ -349,5 +349,4 @@ public static AliasMetaData fromXContent(XContentParser parser) throws IOExcepti
return builder.build();
}
}

Copy link
Member

Choose a reason for hiding this comment

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

Revert this?

@@ -89,8 +89,8 @@ public RestResponse buildResponse(final GetIndexResponse response, final XConten
return new BytesRestResponse(OK, builder);
}

private void writeAliases(final List<AliasMetaData> aliases, final XContentBuilder builder,
final Params params) throws IOException {
private void writeAliases(final List<AliasMetaData> aliases, final XContentBuilder builder, final Params params)
Copy link
Member

Choose a reason for hiding this comment

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

The old way was easier to read.

@@ -60,7 +60,7 @@ private XContentTestUtils() {


/**
* Compares to maps generated from XContentObjects. The order of elements in arrays is ignored.
* Compares two maps generated from XContentObjects. The order of elements in arrays is ignored.
Copy link
Member

Choose a reason for hiding this comment

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

👍

@javanna
Copy link
Member

javanna commented Jun 6, 2018

add to whitelist

@javanna
Copy link
Member

javanna commented Jun 7, 2018

I have addressed some comments @nik9000 and replied to some others. Let me know how this looks now.

@javanna
Copy link
Member

javanna commented Jun 11, 2018

@nik9000 hopefully I have addressed all your comments ;)


public void testFromXContentWithElasticsearchException() throws IOException {
String xContent =
"{" +
Copy link
Member

Choose a reason for hiding this comment

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

❤️

@javanna javanna merged commit 7d7ead9 into elastic:master Jun 12, 2018
@javanna
Copy link
Member

javanna commented Jun 12, 2018

thanks a lot @olcbean for your patience here, so happy this is finally in ;) thanks @nik9000 as well for your review!

javanna pushed a commit that referenced this pull request Jun 13, 2018
Given the weirdness of the response returned by the get alias API, we went for a client specific response, which allows us to hold the error message, exception and status returned as part of the response together with aliases. See #30536 .

Relates to #27205
dnhatn added a commit that referenced this pull request Jun 14, 2018
* master:
  Remove RestGetAllAliasesAction (#31308)
  Temporary fix for broken build
  Reenable Checkstyle's unused import rule (#31270)
  Remove remaining unused imports before merging #31270
  Fix non-REST doc snippet
  [DOC] Extend SQL docs
  Immediately flush channel after writing to buffer (#31301)
  [DOCS] Shortens ML API intros
  Use quotes in the call invocation (#31249)
  move security ingest processors to a sub ingest directory (#31306)
  Add 5.6.11 version constant.
  Fix version detection.
  SQL: Whitelist SQL utility class for better scripting (#30681)
  [Docs] All Rollup docs experimental, agg limitations, clarify DeleteJob (#31299)
  CCS: don't proxy requests for already connected node (#31273)
  Mute ScriptedMetricAggregatorTests testSelfReferencingAggStateAfterMap
  [test] opensuse packaging turn up debug logging
  Add unreleased version 6.3.1
  Removes experimental tag from scripted_metric aggregation (#31298)
  [Rollup] Metric config parser must use builder so validation runs (#31159)
  [ML] Check licence when datafeeds use cross cluster search  (#31247)
  Add notion of internal index settings (#31286)
  Test: Remove broken yml test feature (#31255)
  REST hl client: cluster health to default to cluster level (#31268)
  [ML] Update test thresholds to account for changes to memory control (#31289)
  Log warnings when cluster state publication failed to some nodes (#31233)
  Fix AntFixture waiting condition (#31272)
  Ignore numeric shard count if waiting for ALL (#31265)
  [ML] Implement new rules design (#31110)
  index_prefixes back-compat should test 6.3 (#30951)
  Core: Remove plain execute method on TransportAction (#30998)
  Update checkstyle to 8.10.1 (#31269)
  Set analyzer version in PreBuiltAnalyzerProviderFactory (#31202)
  Modify pipelining handlers to require full requests (#31280)
  Revert upgrade to Netty 4.1.25.Final (#31282)
  Use armored input stream for reading public key (#31229)
  Fix Netty 4 Server Transport tests. Again.
  REST hl client: adjust wait_for_active_shards param in cluster health (#31266)
  REST high-level Client: remove deprecated API methods (#31200)
  [DOCS] Mark SQL feature as experimental
  [DOCS] Updates machine learning custom URL screenshots (#31222)
  Fix naming conventions check for XPackTestCase
  Fix security Netty 4 transport tests
  Fix race in clear scroll (#31259)
  [DOCS] Clarify audit index settings when remote indexing (#30923)
  Delete typos in SAML docs (#31199)
  REST high-level client: add Cluster Health API (#29331)
  [ML][TEST] Mute tests using rules (#31204)
  Support RequestedAuthnContext (#31238)
  SyncedFlushResponse to implement ToXContentObject (#31155)
  Add Get Aliases API to the high-level REST client (#28799)
  Remove some line length supressions (#31209)
  Validate xContentType in PutWatchRequest. (#31088)
  [INGEST] Interrupt the current thread if evaluation grok expressions take too long (#31024)
  Suppress extras FS on caching directory tests
  Revert "[DOCS] Added 6.3 info & updated the upgrade table. (#30940)"
  Revert "Fix snippets in upgrade docs"
  Fix snippets in upgrade docs
  [DOCS] Added 6.3 info & updated the upgrade table. (#30940)
  LLClient: Support host selection (#30523)
  Upgrade to Netty 4.1.25.Final (#31232)
  Enable custom credentials for core REST tests (#31235)
  Move ESIndexLevelReplicationTestCase to test framework (#31243)
  Encapsulate Translog in Engine (#31220)
  HLRest: Add get index templates API (#31161)
  Remove all unused imports and fix CRLF (#31207)
  [Tests] Fix self-referencing tests
  [TEST] Fix testRecoveryAfterPrimaryPromotion
  [Docs] Remove mention pattern files in Grok processor (#31170)
  Use stronger write-once semantics for Azure repository (#30437)
  Don't swallow exceptions on replication (#31179)
  Limit the number of concurrent requests per node (#31206)
  Call ensureNoSelfReferences() on _agg state variable after scripted metric agg script executions (#31044)
  Move java version checker back to its own jar (#30708)
  [test] add fix for rare virtualbox error (#31212)
dnhatn added a commit that referenced this pull request Jun 14, 2018
* 6.x:
  SQL: Fix build on Java 10
  [Tests] Mutualize fixtures code in BaseHttpFixture (#31210)
  [TEST] Fix RemoteClusterClientTests#testEnsureWeReconnect
  [ML] Update test thresholds to account for changes to memory control (#31289)
  Reenable Checkstyle's unused import rule (#31270)
  [ML] Check licence when datafeeds use cross cluster search  (#31247)
  Fix non-REST doc snippet
  [DOC] Extend SQL docs
  [DOCS] Shortens ML API intros
  Use quotes in the call invocation (#31249)
  move security ingest processors to a sub ingest directory (#31306)
  SQL: Whitelist SQL utility class for better scripting (#30681)
  Add 5.6.11 version constant.
  Fix version detection.
  [Docs] All Rollup docs experimental, agg limitations, clarify DeleteJob (#31299)
  Add missing release notes.
  Security: fix token bwc with pre 6.0.0-beta2 (#31254)
  Fix compilation error in UpdateSettingsIT (#31304)
  Test: Remove broken yml test feature (#31255)
  Add unreleased version 6.3.1
  [Rollup] Metric config parser must use builder so validation runs (#31159)
  Removes experimental tag from scripted_metric aggregation (#31298)
  [DOCS] Removes coming tag from 6.3.0 release notes
  6.3 release notes.
  Add notion of internal index settings (#31286)
  REST high-level client: add Cluster Health API (#29331)
  Remove leftover usage of deprecated client API
  SyncedFlushResponse to implement ToXContentObject (#31155)
  Add Get Aliases API to the high-level REST client (#28799)
  HLRest: Add get index templates API (#31161)
  Log warnings when cluster state publication failed to some nodes (#31233)
  Fix AntFixture waiting condition (#31272)
  [TEST] Mute RecoveryIT.testHistoryUUIDIsGenerated
  Ignore numeric shard count if waiting for ALL (#31265)
  Update checkstyle to 8.10.1 (#31269)
  Set analyzer version in PreBuiltAnalyzerProviderFactory (#31202)
  Revert upgrade to Netty 4.1.25.Final (#31282)
  Use armored input stream for reading public key (#31229)
  [DOCS] Added 'fail_on_unsupported_field' param to MLT. Closes #28008 (#31160)
  Fix Netty 4 Server Transport tests. Again.
  [DOCS] Fixed typo.
  [DOCS] Added release highlights for 6.3 (#31256)
  [DOCS] Mark SQL feature as experimental
  [DOCS] Updates machine learning custom URL screenshots (#31222)
  Fix naming conventions check for XPackTestCase
  Fix security Netty 4 transport tests
  Fix race in clear scroll (#31259)
  [DOCS] Clarify audit index settings when remote indexing (#30923)
  [ML][TEST] Mute tests using rules (#31204)
  Support RequestedAuthnContext (#31238)
  Validate xContentType in PutWatchRequest. (#31088)
  [INGEST] Interrupt the current thread if evaluation grok expressions take too long (#31024)
  Upgrade to Netty 4.1.25.Final (#31232)
  Suppress extras FS on caching directory tests
  Revert "[DOCS] Added 6.3 info & updated the upgrade table. (#30940)"
  Revert "Fix snippets in upgrade docs"
  Fix snippets in upgrade docs
  [DOCS] Added 6.3 info & updated the upgrade table. (#30940)
  Enable custom credentials for core REST tests (#31235)
  Move ESIndexLevelReplicationTestCase to test framework (#31243)
  Encapsulate Translog in Engine (#31220)
  [DOCS] Adds machine learning 6.3.0 release notes (#31217)
  Remove all unused imports and fix CRLF (#31207)
  [TEST] Fix testRecoveryAfterPrimaryPromotion
  [Docs] Remove mention pattern files in Grok processor (#31170)
  Use stronger write-once semantics for Azure repository (#30437)
  Don't swallow exceptions on replication (#31179)
  Compliant SAML Response destination check (#31175)
  Move java version checker back to its own jar (#30708)
  TEST:  Retry synced-flush if ongoing ops on primary (#30978)
  [test] add fix for rare virtualbox error (#31212)
@olcbean
Copy link
Contributor Author

olcbean commented Jun 14, 2018

🍻
Thanks @javanna for taking this one over the finish line ;)

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

Successfully merging this pull request may close these issues.

6 participants