HLREST: Add Clear Roles Cache API by bleskes · Pull Request #34187 · elastic/elasticsearch

bleskes · 2018-10-01T16:01:29Z

Add's support to the Clear Roles Cache API to the High Level Rest Client.

Reviews should note the addition of the NodesResponseHeader utility class (I'm not sure it's in the right place) and also the fact this specific API has a weird response structure:

{
   _nodes: {
       total: 2,
       successful: 2
       failed: 0
   },
  nodes: {
       "id": {
          ...
        }
  }
}

I chose to the response object I added to have a getHeader() method to return the content of the _nodes section and a getNodes() method to return the node info (mimicking the response object in ES core). We can also choose to fold the header into the top level response class and expose total, successful, failed and failure directly (at the expense of the class not looking like the JSON)/

Relates to #29827

elasticmachine · 2018-10-01T16:01:30Z

Pinging @elastic/es-core-infra

jaymode

I left some small comments. It looks good and you beat me to getting my PR up with my own version of NodesResponseHeader that is very similar (I did it for the clear realm cache api)

jaymode · 2018-10-02T16:15:07Z

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

+/**
+ * The request used to clear the cache for native roles stored in an index.
+ */
+public final class ClearRolesCacheRequest implements Validatable, Closeable {


I don't think this class should be closeable

jaymode · 2018-10-02T16:15:23Z

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

+ */
+public final class ClearRolesCacheRequest implements Validatable, Closeable {
+
+    final String[] names;


make it private?

jaymode · 2018-10-02T16:15:43Z

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

+ * A utility class to parse the Nodes Header returned by
+ * {@link RestActions#buildNodesHeader(XContentBuilder, ToXContent.Params, BaseNodesResponse)}.
+ */
+public class NodesResponseHeader {


make it final?

Is this security related? or is it generic that APIs besides security would use it? If its only for security, pls drop it in security package, as I will be moving those clients into their respective packages soon.

This would be used by security, watcher, and a few cluster APIs (cluster stats, nodes info, and nodes stats).

jaymode · 2018-10-02T16:17:32Z

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

+
+    private final List<Node> nodes;
+    private final NodesResponseHeader header;
+    private ClusterName clusterName;


can we just make this a String so we do not need the ClusterName object from the server project? Also make it final?

jaymode · 2018-10-02T16:18:25Z

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

+    /**
+     * Get the {@link ClusterName} associated with all of the nodes.
+     *
+     * @return Never {@code null}.


I think we should enforce this in the constructor of the class with Objects.requireNonNull

++, ive been telling ppl to remove the validation method and put all the validation into the constructor. the old school validation is a relic of the old actions.

jaymode · 2018-10-02T16:19:48Z

...high-level/src/test/java/org/elasticsearch/client/documentation/SecurityDocumentationIT.java

+            // tag::clear-roles-cache-execute-async
+            client.security().clearRolesCacheAsync(request, RequestOptions.DEFAULT, listener); // <1>
+            // end::clear-roles-cache-execute-async
+        }


add a check that the request completes like assertTrue(latch.await(30L, TimeUnit.SECONDS));

hub-cap

+1 to jays comments, good work so far.

hub-cap · 2018-10-02T16:47:37Z

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

+ * A utility class to parse the Nodes Header returned by
+ * {@link RestActions#buildNodesHeader(XContentBuilder, ToXContent.Params, BaseNodesResponse)}.
+ */
+public class NodesResponseHeader {


Is this security related? or is it generic that APIs besides security would use it? If its only for security, pls drop it in security package, as I will be moving those clients into their respective packages soon.

hub-cap · 2018-10-02T16:48:44Z

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

+    /**
+     * Get the {@link ClusterName} associated with all of the nodes.
+     *
+     * @return Never {@code null}.


++, ive been telling ppl to remove the validation method and put all the validation into the constructor. the old school validation is a relic of the old actions.

hub-cap · 2018-10-02T16:50:48Z

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

+    }
+
+    @SuppressWarnings("unchecked")
+    private static final ConstructingObjectParser<ClearRolesCacheResponse, Void> PARSER =


all this stuff is typically up top of the class

jaymode · 2018-10-22T19:36:44Z

@hub-cap I picked this one up from @bleskes so I can unblock myself for clear realm caches. When you get a chance, can you please take a look?

hub-cap

I see updates to the Doc test, but i dont see any accompanying asciidoc changes, just making sure these are done properly. Other than that, looks good.

bleskes · 2018-10-23T09:45:47Z

@jaymode sorry. I didn't realize you were blocked on this one. Thanks for picking it up.

jaymode · 2018-10-24T15:15:30Z

@hub-cap good catch on the docs. I just added them

Adds support for the Clear Roles Cache API to the High Level Rest Client. As part of this a helper class, NodesResponseHeader, has been added that enables parsing the nodes header from responses that are node requests. Relates to #29827

bleskes added 4 commits September 30, 2018 23:02

add clear roles cache request to high level client

fc8c4c2

supress

c5f6be8

lint

e966203

fix parsing

5bb06b1

bleskes added >enhancement :Core/Java High Level REST Client labels Oct 1, 2018

bleskes requested a review from hub-cap October 1, 2018 16:01

elasticmachine mentioned this pull request Oct 1, 2018

Java high-level REST client completeness for xpack APIs #29827

Closed

98 tasks

jaymode self-requested a review October 2, 2018 02:30

bleskes added 2 commits October 2, 2018 07:54

Merge remote-tracking branch 'upstream/master' into clear_cache

01fd7fd

Merge remote-tracking branch 'upstream/master' into clear_cache

7f9659a

jaymode requested changes Oct 2, 2018

View reviewed changes

hub-cap reviewed Oct 2, 2018

View reviewed changes

jaymode added 2 commits October 22, 2018 13:13

Merge branch 'master' into clear_cache

6521201

address review feedback

80bba1c

hub-cap approved these changes Oct 22, 2018

View reviewed changes

Merge branch 'master' into clear_cache

c3ba5b9

jaymode added 2 commits October 24, 2018 08:43

Merge branch 'master' into clear_cache

1598d16

add docs

780a76b

jaymode approved these changes Oct 24, 2018

View reviewed changes

jaymode added 3 commits October 25, 2018 09:15

fix checkstyle

ae20137

Merge branch 'master' into clear_cache

d63d921

Merge branch 'master' into clear_cache

382cb79

jaymode merged commit a086c66 into elastic:master Oct 26, 2018

jaymode mentioned this pull request Oct 26, 2018

HLRest: add security authenticate API #33552

Merged

Conversation

bleskes commented Oct 1, 2018

Uh oh!

elasticmachine commented Oct 1, 2018

Uh oh!

jaymode left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hub-cap left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jaymode commented Oct 22, 2018

Uh oh!

hub-cap left a comment

Choose a reason for hiding this comment

Uh oh!

bleskes commented Oct 23, 2018

Uh oh!

jaymode commented Oct 24, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants