GraphClient for the high level REST client and associated tests #32366

markharwood · 2018-07-25T15:26:21Z

Documentation yet to be done

elasticmachine · 2018-07-25T15:26:23Z

Pinging @elastic/es-core-infra

elasticmachine · 2018-07-25T15:26:23Z

Pinging @elastic/es-search-aggs

markharwood · 2018-07-25T15:53:17Z

test this please

nik9000 · 2018-07-25T21:08:38Z

x-pack/protocol/src/main/java/org/elasticsearch/protocol/xpack/graph/Connection.java

+ * as a weight.
+ */
+public class Connection {
+    Vertex from;


I think it'd be nice if these were private. We've been trying to be more "standard-java" in the rest client.

nik9000 · 2018-07-25T21:10:06Z

x-pack/protocol/src/main/java/org/elasticsearch/protocol/xpack/graph/Connection.java

+            return new Connection(vertices.get(fromIndex), vertices.get(toIndex), weight, docCount);
+        }
+
+        static UnresolvedConnection fromXContent(XContentParser parser) throws IOException {


I think ObjectParser would be easier to review but this looks right so I'm fine with it.

nik9000 · 2018-07-25T21:13:56Z

x-pack/protocol/src/main/java/org/elasticsearch/protocol/xpack/graph/GraphExploreRequest.java

+    public XContentBuilder toXContent(XContentBuilder builder, Params params) throws IOException {
+        builder.startObject();
+
+        builder.startObject("controls"); //=== Start Controls


I've borrowed a style from dakrone that looks like

builder.startObject("controls"); { if (sampleSize != ... ) { builder.field("sample_size", sampleSize); } ... } builder.endObject();

It sort of replaces the need for //=== Start Controls.

nik9000 · 2018-07-25T21:14:40Z

x-pack/protocol/src/main/java/org/elasticsearch/protocol/xpack/graph/GraphExploreRequest.java

+        builder.endObject(); //=== End controls
+
+        for (Hop hop : hops) {
+            if (hop.parentHop != null) {


They look like a chain! Woah!

nik9000 · 2018-07-25T21:15:37Z

x-pack/protocol/src/main/java/org/elasticsearch/protocol/xpack/graph/GraphExploreResponse.java

+        List<Vertex> vertices = new ArrayList<>();
+        List<ShardSearchFailure> failures = new ArrayList<>();
+        List<Connection.UnresolvedConnection> unresolvedConnections = new ArrayList<>();
+        for (Token token = parser.nextToken(); token != Token.END_OBJECT; token = parser.nextToken()) {


This one also looks right when I scan it but'd be easier to review as ObjectParser.

I may be missing something but isn't ObjectParser more geared towards parsing "request" objects? Requests tend to have existing "Builder" classes with setter methods used by clients to express criteria and ObjectParser relies on registering setter methods.

Response objects tend to be immutable data objects so to accommodate ObjectParser we either end up adding "setter" methods to response objects (ugh) or creating non-public "ResponseBuilder" classes?

ConstructingObjectParser can mostly get the job done without needing any extra builders. But sometimes builders are easier to understand. The idea with using these parsers is that they are a ton easier to review for correctness, especially that they properly ignore new fields. It isn't required, but it does make it easier to be sure that it is all rigged up right.

hub-cap · 2018-08-03T00:41:14Z

I have just committed #32596 which changes the location of the xpack clients. Its very likely your tests wont compile now that the xpack portion of client.xpack().yourCommercialClient() is removed.

markharwood · 2018-08-14T16:47:47Z

Many thanks for the review, @nik9000 . I've switched the code over to ObjectParser.

Good to go @nik9000 @hub-cap ?

hub-cap · 2018-08-14T16:56:53Z

docs/java-rest/high-level/supported-apis.asciidoc

+
+
+
+


thats a lot of blank lines!

hub-cap · 2018-08-14T16:59:03Z

if @nik9000 is good then Im good too. I just did a super quick eyeball and didnt see anything insane. if its got an approval from him... i was out all last week so im still coming back up to speed :)

markharwood · 2018-08-16T17:11:30Z

Any other budding reviewers you can suggest, @hub-cap ?

hub-cap · 2018-08-17T14:22:06Z

hey, yea Ill just do a review in the next 24 hrs. Im trying to get the actions fixed up first. Id prefer to get that merged, but if u get an approval first then GOGOGO.

nik9000

Left two little picky things but LGTM anyway. Thanks for working through all of my requests!

nik9000 · 2018-08-20T18:12:29Z

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

@@ -2598,6 +2601,35 @@ public void testXPackPutWatch() throws Exception {
        request.getEntity().writeTo(bos);
        assertThat(bos.toString("UTF-8"), is(body));
    }
+
+    public void testXPackGraphExplore() throws Exception {


I think maybe this should be testGraphExplore now that we're removing the xpack in lots of places.

nik9000 · 2018-08-20T18:13:34Z

x-pack/protocol/src/main/java/org/elasticsearch/protocol/xpack/graph/Connection.java

+        this.docCount = docCount;
+    }
+
+    void readFrom(StreamInput in, Map<VertexId, Vertex> vertices) throws IOException {


Why not a ctor that takes StreamInput and Map?

markharwood · 2018-08-21T09:32:08Z

Many thanks, @nik9000
I addressed your review comments and will push when I get a green result here.

Backport of PR #32366 Part of #29827 work

markharwood added WIP :Core/Java High Level REST Client labels Jul 25, 2018

markharwood force-pushed the fix/29827Graph branch from 5356df0 to ea2f8df Compare July 25, 2018 15:35

nik9000 reviewed Jul 25, 2018

View reviewed changes

markharwood force-pushed the fix/29827Graph branch from 11caa1d to 4891e85 Compare August 2, 2018 10:50

markharwood force-pushed the fix/29827Graph branch 2 times, most recently from 03fccf1 to 0e54b45 Compare August 7, 2018 11:15

markharwood removed the WIP label Aug 8, 2018

markharwood force-pushed the fix/29827Graph branch 3 times, most recently from 7a10597 to 809a478 Compare August 14, 2018 12:47

elasticmachine mentioned this pull request Aug 14, 2018

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

Closed

98 tasks

hub-cap reviewed Aug 14, 2018

View reviewed changes

docs/java-rest/high-level/supported-apis.asciidoc Outdated

Copy link

Contributor

hub-cap Aug 14, 2018

Choose a reason for hiding this comment

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

thats a lot of blank lines!

markharwood force-pushed the fix/29827Graph branch from 809a478 to 9be83b1 Compare August 15, 2018 09:16

nik9000 approved these changes Aug 20, 2018

View reviewed changes

markharwood added 3 commits August 21, 2018 09:14

GraphClient for the high level REST client and associated tests.

c744bb4

Removed excess whitespace

0387a20

Picking up a couple of review comments

e7360a8

markharwood force-pushed the fix/29827Graph branch from 9be83b1 to e7360a8 Compare August 21, 2018 08:31

markharwood merged commit 38bdf9c into elastic:master Aug 21, 2018

markharwood mentioned this pull request Aug 21, 2018

GraphClient for the high level REST client and associated tests. #33025

Merged

markharwood added the v7.0.0 label Aug 21, 2018

markharwood added a commit that referenced this pull request Aug 22, 2018

HLRC: GraphClient and associated tests. (#33025)

62da081

Backport of PR #32366 Part of #29827 work

jpountz added >feature and removed :Analytics/Graph labels Jan 29, 2019

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

GraphClient for the high level REST client and associated tests #32366

GraphClient for the high level REST client and associated tests #32366

markharwood commented Jul 25, 2018

elasticmachine commented Jul 25, 2018

elasticmachine commented Jul 25, 2018

markharwood commented Jul 25, 2018

nik9000 Jul 25, 2018

nik9000 Jul 25, 2018

nik9000 Jul 25, 2018

nik9000 Jul 25, 2018

nik9000 Jul 25, 2018

markharwood Aug 8, 2018 •

edited

Loading

nik9000 Aug 9, 2018

hub-cap commented Aug 3, 2018

markharwood commented Aug 14, 2018

hub-cap Aug 14, 2018

hub-cap commented Aug 14, 2018

markharwood commented Aug 16, 2018

hub-cap commented Aug 17, 2018

nik9000 left a comment

nik9000 Aug 20, 2018

nik9000 Aug 20, 2018

markharwood commented Aug 21, 2018

GraphClient for the high level REST client and associated tests #32366

GraphClient for the high level REST client and associated tests #32366

Conversation

markharwood commented Jul 25, 2018

elasticmachine commented Jul 25, 2018

elasticmachine commented Jul 25, 2018

markharwood commented Jul 25, 2018

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

markharwood Aug 8, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hub-cap commented Aug 3, 2018

markharwood commented Aug 14, 2018

Choose a reason for hiding this comment

hub-cap commented Aug 14, 2018

markharwood commented Aug 16, 2018

hub-cap commented Aug 17, 2018

nik9000 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

markharwood commented Aug 21, 2018

markharwood Aug 8, 2018 •

edited

Loading