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

olcbean · 2018-02-23T11:57:00Z

Add Get Aliases API to the high-level REST client

Relates to #27205

elasticmachine · 2018-02-23T11:57:01Z

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?

elasticmachine · 2018-02-23T11:57:01Z

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 · 2018-02-23T12:03:06Z

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

olcbean · 2018-02-23T12:05:24Z

server/src/test/java/org/elasticsearch/action/admin/indices/alias/GetAliasesResponseTests.java

+            return builder.build();
+        }
+
+        Set<String> indices = new HashSet<>();


pls ignore for now, still incomplete

javanna

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

javanna · 2018-03-05T14:22:39Z

server/src/main/java/org/elasticsearch/action/admin/indices/alias/get/GetAliasesResponse.java

+    }
+
+    @Override
+    public XContentBuilder toXContent(XContentBuilder builder, Params params) throws IOException {


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.

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 )

you are right, it is not necessary!

javanna · 2018-03-05T14:27:51Z

server/src/main/java/org/elasticsearch/action/admin/indices/alias/get/GetAliasesResponse.java

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


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.

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 ?

ignore me :)

javanna · 2018-03-05T14:30:34Z

server/src/main/java/org/elasticsearch/action/admin/indices/alias/get/GetAliasesResponse.java

+                    }
+                } else {
+                    parser.skipChildren();
+                }


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

javanna · 2018-03-05T14:31:12Z

server/src/main/java/org/elasticsearch/cluster/metadata/AliasMetaData.java

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

 import static java.util.Collections.emptySet;

-public class AliasMetaData extends AbstractDiffable<AliasMetaData> {
+public class AliasMetaData extends AbstractDiffable<AliasMetaData> implements ToXContent{


can we go for either ToXContentFragment or ToXContentObject?

javanna · 2018-03-05T14:32:57Z

server/src/test/java/org/elasticsearch/action/admin/indices/alias/GetAliasesResponseTests.java

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


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

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.

makes sense. maybe expand a bit the comment?

olcbean · 2018-03-20T10:25:42Z

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

olcbean · 2018-03-20T10:33:44Z

client/rest-high-level/src/test/java/org/elasticsearch/client/IndicesClientIT.java

+           }
+         }
+        }
+        */


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 ?

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?

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 ).

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.

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 ?

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

olcbean · 2018-03-20T10:47:34Z

client/rest-high-level/src/main/java/org/elasticsearch/client/Request.java

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


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

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).

javanna

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

javanna · 2018-03-22T09:39:28Z

client/rest-high-level/src/main/java/org/elasticsearch/client/Request.java

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


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).

javanna · 2018-03-22T09:42:17Z

client/rest-high-level/src/main/java/org/elasticsearch/client/Request.java

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


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.

javanna · 2018-03-22T09:44:56Z

client/rest-high-level/src/main/java/org/elasticsearch/client/Request.java

+        return optional(params, null);
+    }
+
+    private static String[] optional(String[] params, String defaultParam) {


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.

javanna · 2018-03-22T09:50:33Z

client/rest-high-level/src/test/java/org/elasticsearch/client/IndicesClientIT.java

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


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?

javanna · 2018-03-22T10:49:11Z

client/rest-high-level/src/test/java/org/elasticsearch/client/IndicesClientIT.java

+           }
+         }
+        }
+        */


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?

javanna · 2018-03-22T10:56:50Z

server/src/main/java/org/elasticsearch/action/admin/indices/alias/get/GetAliasesRequest.java

+     * @param name value of the aliases names.
+     * @return this request
+     */
+    public GetAliasesRequest name(String[] name) {


I think that this can go away after all, unless I am missing something :)

javanna · 2018-03-22T10:57:20Z

server/src/main/java/org/elasticsearch/action/admin/indices/alias/get/GetAliasesResponse.java

+    }
+
+    @Override
+    public XContentBuilder toXContent(XContentBuilder builder, Params params) throws IOException {


you are right, it is not necessary!

javanna · 2018-03-22T10:59:43Z

server/src/main/java/org/elasticsearch/action/admin/indices/alias/get/GetAliasesResponse.java

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


ignore me :)

javanna · 2018-03-22T11:03:00Z

server/src/main/java/org/elasticsearch/action/admin/indices/alias/get/GetAliasesResponse.java


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


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.

javanna · 2018-03-22T11:04:18Z

server/src/test/java/org/elasticsearch/action/admin/indices/alias/GetAliasesResponseTests.java

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


makes sense. maybe expand a bit the comment?

implemented response/error handling

olcbean · 2018-04-13T12:41:13Z

client/rest-high-level/src/test/java/org/elasticsearch/client/IndicesClientIT.java

+            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]"));


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 ?

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.

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 ?

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.

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

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?

javanna

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.

javanna · 2018-05-04T12:26:33Z

server/src/main/java/org/elasticsearch/action/admin/indices/alias/get/GetAliasesResponse.java

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


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.

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

javanna · 2018-05-11T13:06:27Z

client/rest-high-level/src/test/java/org/elasticsearch/client/IndicesClientIT.java

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


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' { }

javanna · 2018-05-11T13:23:44Z

client/rest-high-level/src/test/java/org/elasticsearch/client/IndicesClientIT.java

+            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]"));


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.

javanna · 2018-05-11T13:27:37Z

...rc/main/java/org/elasticsearch/action/admin/indices/alias/get/TransportGetAliasesAction.java

+            }
+
+            difference.removeAll(matches);
+            if (false == difference.isEmpty()) {


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.

javanna · 2018-05-11T13:27:53Z

...rc/main/java/org/elasticsearch/action/admin/indices/alias/get/TransportGetAliasesAction.java

+            status.set(RestStatus.OK);
+        }
+        if (message.get() == null) {
+            message.set("");


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

javanna · 2018-05-11T13:58:14Z

server/src/main/java/org/elasticsearch/action/admin/indices/alias/get/GetAliasesResponse.java

+                out.writeInt(status.getStatus());
+                out.writeString(errorMsg);
+            } else {
+                out.writeBoolean(false);


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)?

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.

javanna · 2018-05-11T13:59:55Z

server/src/main/java/org/elasticsearch/action/admin/indices/alias/get/GetAliasesResponse.java

+        return Objects.hash(fromListAliasesToSet(aliases), status, errorMsg);
+    }
+
+    private ImmutableOpenMap<String, Set<AliasMetaData>> fromListAliasesToSet(ImmutableOpenMap<String, List<AliasMetaData>> list) {


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

javanna · 2018-05-11T14:04:16Z

server/src/main/java/org/elasticsearch/action/admin/indices/alias/get/GetAliasesResponse.java

+                        status = RestStatus.fromCode(parser.intValue());
+                    }
+                } else if ("error".equals(currentFieldName)) {
+                    if ((token = parser.nextToken()) != Token.FIELD_NAME) {


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.

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

javanna · 2018-05-11T14:06:31Z

server/src/main/java/org/elasticsearch/action/admin/indices/alias/get/GetAliasesResponse.java

+        }
+        if (RestStatus.OK != status && aliasesBuilder.isEmpty()) {
+            throw new ElasticsearchStatusException(exceptionMsg, status);
+        }


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

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 ).

javanna · 2018-05-11T14:07:55Z

server/src/test/java/org/elasticsearch/action/admin/indices/alias/GetAliasesResponseTests.java

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


I think that we should also have some serialization test that cover the bw comp changes we are making.

javanna · 2018-05-11T14:59:27Z

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

olcbean · 2018-05-18T16:12:44Z

@javanna can you have another look ?

javanna

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

javanna · 2018-05-22T09:35:18Z

server/src/main/java/org/elasticsearch/action/admin/indices/alias/get/GetAliasesResponse.java

-        } else {
-            this.errorMsg = errorMsg;
-        }
+        this.status = status == null ? RestStatus.OK : status;


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.

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

javanna · 2018-05-22T09:36:02Z

server/src/main/java/org/elasticsearch/action/admin/indices/alias/get/GetAliasesResponse.java

-            this.errorMsg = errorMsg;
-        }
+        this.status = status == null ? RestStatus.OK : status;
+        this.errorMessage = errorMessage;
    }

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


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.

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

javanna · 2018-05-22T09:38:28Z

server/src/main/java/org/elasticsearch/action/admin/indices/alias/get/GetAliasesResponse.java

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


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

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

javanna · 2018-05-22T09:39:44Z

server/src/main/java/org/elasticsearch/action/admin/indices/alias/get/GetAliasesResponse.java

                out.writeBoolean(true);
-                out.writeInt(status.getStatus());
-                out.writeString(errorMsg);
+                out.writeString(errorMessage);


use out.writeOptionalString here?

javanna · 2018-05-22T09:41:05Z

server/src/test/java/org/elasticsearch/action/admin/indices/alias/GetAliasesResponseTests.java

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


is the commented out part some leftover?

javanna · 2018-05-22T09:44:17Z

server/src/test/java/org/elasticsearch/action/admin/indices/alias/GetAliasesResponseTests.java

+    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


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 · 2018-06-05T15:59:37Z

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 · 2018-06-05T18:58:40Z

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 · 2018-06-06T07:47:58Z

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 · 2018-06-06T08:07:13Z

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 · 2018-06-06T08:28:01Z

retest this please

olcbean

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

olcbean · 2018-06-06T09:00:41Z

server/src/main/java/org/elasticsearch/rest/action/admin/indices/RestGetAliasesAction.java

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


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

yea that is something that gets easily overlooked.

olcbean · 2018-06-06T09:20:00Z

client/rest-high-level/src/main/java/org/elasticsearch/client/GetAliasesResponse.java

+
+import static org.elasticsearch.common.xcontent.XContentParserUtils.ensureExpectedToken;
+
+public class GetAliasesResponse extends ActionResponse implements StatusToXContentObject {


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 ... )

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.

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) ?

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.

olcbean · 2018-06-06T09:26:47Z

client/rest-high-level/src/main/java/org/elasticsearch/client/GetAliasesResponse.java

+    }
+
+    @Override
+    public XContentBuilder toXContent(XContentBuilder builder, Params params) throws IOException {


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

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?

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 :)

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.

olcbean · 2018-06-06T09:28:34Z

client/rest-high-level/src/test/java/org/elasticsearch/client/GetAliasesResponseTests.java

+        return new GetAliasesResponse(status, errorMessage, createIndicesAliasesMap(0, 5));
+    }
+
+    private static Map<String, Set<AliasMetaData>> createIndicesAliasesMap(int min, int max) {


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

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

🤦 Missed that.
What about the createAliasMetaData and mutateAliasMetaData

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 · 2018-06-06T11:35:25Z

retest this please

nik9000

I left a few thoughts. I think we should document

nik9000 · 2018-06-06T13:44:12Z

client/rest-high-level/src/main/java/org/elasticsearch/client/GetAliasesResponse.java

+
+public class GetAliasesResponse extends ActionResponse implements StatusToXContentObject {
+
+    private final RestStatus status;


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

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.

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

nik9000 · 2018-06-06T13:45:21Z

client/rest-high-level/src/main/java/org/elasticsearch/client/GetAliasesResponse.java

+    }
+
+    @Override
+    public XContentBuilder toXContent(XContentBuilder builder, Params params) throws IOException {


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.

nik9000 · 2018-06-06T13:54:57Z

client/rest-high-level/src/main/java/org/elasticsearch/client/GetAliasesResponse.java

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


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.

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

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.

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

nik9000 · 2018-06-06T13:55:33Z

client/rest-high-level/src/main/java/org/elasticsearch/client/GetAliasesResponse.java

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


We don't want the whole exception?

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.

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.

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

then you would either have the message or the exception?

nik9000 · 2018-06-06T13:56:10Z

client/rest-high-level/src/test/java/org/elasticsearch/client/GetAliasesResponseTests.java

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


Can you line the "s up?

nik9000 · 2018-06-06T15:15:13Z

docs/java-rest/high-level/indices/get_alias.asciidoc

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


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

Because we're getting aliases, not indices.

nik9000 · 2018-06-06T15:15:53Z

...rc/main/java/org/elasticsearch/action/admin/indices/alias/get/TransportGetAliasesAction.java

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


Revert this?

nik9000 · 2018-06-06T15:15:59Z

server/src/main/java/org/elasticsearch/cluster/metadata/AliasMetaData.java

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


Revert this?

nik9000 · 2018-06-06T15:16:28Z

server/src/main/java/org/elasticsearch/rest/action/admin/indices/RestGetAllAliasesAction.java

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


The old way was easier to read.

nik9000 · 2018-06-06T15:16:51Z

test/framework/src/main/java/org/elasticsearch/test/XContentTestUtils.java

@@ -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.


javanna · 2018-06-06T19:18:33Z

add to whitelist

javanna · 2018-06-07T14:06:00Z

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

javanna · 2018-06-11T19:39:57Z

@nik9000 hopefully I have addressed all your comments ;)

nik9000 · 2018-06-11T19:51:22Z

client/rest-high-level/src/test/java/org/elasticsearch/client/GetAliasesResponseTests.java

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


javanna · 2018-06-12T08:27:08Z

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

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

* 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)

* 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 · 2018-06-14T19:23:59Z

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

Add Get Aliases API to the high-level RESR client

6c01eeb

olcbean commented Feb 23, 2018

View reviewed changes

javanna added :Core/Java High Level REST Client >enhancement labels Feb 27, 2018

javanna self-requested a review February 27, 2018 20:03

javanna mentioned this pull request Feb 27, 2018

Java high-level REST client completeness #27205

Closed

80 tasks

javanna self-assigned this Mar 5, 2018

javanna requested changes Mar 5, 2018

View reviewed changes

next iteration : still work needed in GetAliasesResponseTests

1571cb3

olcbean commented Mar 20, 2018

View reviewed changes

javanna requested changes Mar 22, 2018

View reviewed changes

olcbean added 2 commits April 13, 2018 12:36

addressing some comments

a33ea0f

implemented response/error handling

Merge remote-tracking branch 'origin/master' into hlrc_get_alias

a919d73

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

olcbean commented Apr 13, 2018

View reviewed changes

olcbean and others added 2 commits April 13, 2018 17:27

chmod

baed505

Merge branch 'master' into hlrc_get_alias

f74d0b5

javanna requested changes May 11, 2018

View reviewed changes

javanna mentioned this pull request May 11, 2018

Revise get aliases response #30536

Closed

javanna mentioned this pull request May 15, 2018

Do not return all indices if a specific alias is requested via get aliases api. #29538

Merged

addressing reviewers comments

7720b71

javanna requested changes May 22, 2018

View reviewed changes

addressing reviewers comments

7238098

introduce client specific GetAliasesResponse

fa9d4e1

Also revert non bw compatible changes made to REST action and transport action

Merge branch 'master' into hlrc_get_alias

939ae4d

olcbean commented Jun 6, 2018

View reviewed changes

nik9000 reviewed Jun 6, 2018

View reviewed changes

address review comments

035d823

javanna added 2 commits June 6, 2018 23:37

Merge branch 'master' into hlrc_get_alias

be1daf7

fix javadocs

5da9ed6

javanna added 2 commits June 11, 2018 21:34

address review comments

5bcc1ce

Merge branch 'master' into hlrc_get_alias

2e2217a

nik9000 approved these changes Jun 11, 2018

View reviewed changes

javanna merged commit 7d7ead9 into elastic:master Jun 12, 2018

javanna added the backport pending label Jun 12, 2018

javanna removed the backport pending label Jun 13, 2018

olcbean mentioned this pull request Jun 21, 2018

[docs] High-level REST client : incomplete docs #31499

Closed

colings86 added v7.0.0-beta1 and removed v7.0.0 labels Feb 7, 2019


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


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

		public class GetAliasesResponse extends ActionResponse implements StatusToXContentObject {


		public class GetAliasesResponse extends ActionResponse implements StatusToXContentObject {

		private final RestStatus status;

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

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

Conversation

olcbean commented Feb 23, 2018

elasticmachine commented Feb 23, 2018

elasticmachine commented Feb 23, 2018

olcbean commented Feb 23, 2018

Choose a reason for hiding this comment

javanna left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

olcbean commented Mar 20, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

javanna Mar 22, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

javanna left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

javanna left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

javanna commented May 11, 2018

olcbean commented May 18, 2018

javanna left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

olcbean commented Jun 5, 2018

olcbean commented Mar 20, 2018 •

edited

Loading

javanna Mar 22, 2018 •

edited

Loading