GraphClient for the high level REST client and associated tests#32366
GraphClient for the high level REST client and associated tests#32366markharwood merged 3 commits intoelastic:masterfrom
Conversation
|
Pinging @elastic/es-core-infra |
|
Pinging @elastic/es-search-aggs |
5356df0 to
ea2f8df
Compare
|
test this please |
There was a problem hiding this comment.
I think it'd be nice if these were private. We've been trying to be more "standard-java" in the rest client.
There was a problem hiding this comment.
I think ObjectParser would be easier to review but this looks right so I'm fine with it.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
This one also looks right when I scan it but'd be easier to review as ObjectParser.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
11caa1d to
4891e85
Compare
|
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 |
03fccf1 to
0e54b45
Compare
7a10597 to
809a478
Compare
There was a problem hiding this comment.
thats a lot of blank lines!
|
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 :)809a478 to
9be83b1
Compare
|
Any other budding reviewers you can suggest, @hub-cap ? |
|
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 a comment
There was a problem hiding this comment.
Left two little picky things but LGTM anyway. Thanks for working through all of my requests!
There was a problem hiding this comment.
I think maybe this should be testGraphExplore now that we're removing the xpack in lots of places.
There was a problem hiding this comment.
Why not a ctor that takes StreamInput and Map?
9be83b1 to
e7360a8
Compare
|
Many thanks, @nik9000 |
Documentation yet to be done