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

HLREST: Add x-pack-info API #31870

Merged
merged 21 commits into from
Jul 8, 2018
Merged

HLREST: Add x-pack-info API #31870

merged 21 commits into from
Jul 8, 2018

Conversation

nik9000
Copy link
Member

@nik9000 nik9000 commented Jul 6, 2018

This is the first x-pack API we're adding to the high level REST client
so there is a lot to talk about here!

= Open source

The client for these APIs is open source. We're taking the previously
Elastic licensed files used for the Request and Response objects and
relicensing them under the Apache 2 license.

The implementation of these features is staying under the Elastic
license. This lines up with how the rest of the Elasticsearch language
clients work.

= Location of the new files

We're moving all of the Request and Response objects that we're
relicensing to the x-pack/protocol directory. We're adding a copy of
the Apache 2 license to the root fo the x-pack/protocol directory to
line up with the language in the root LICENSE.txt file. All files in
this directory will have the Apache 2 license header as well. We don't
want there to be any confusion. Even though the files are under the
x-pack directory, they are Apache 2 licensed.

We chose this particular directory layout because it keeps the X-Pack
stuff together and easier to think about.

= Location of the API in the REST client

We've been following the layout of the rest-api-spec files for other
APIs and we plan to do this for the X-Pack APIs with one exception:
we're dropping the xpack from the name of most of the APIs. So
xpack.graph.explore will become graph().explore() and
xpack.license.get will become license().get().

xpack.info and xpack.usage are special here though because they
don't belong to any proper category. For now I'm just calling
xpack.info xPackInfo() and intend to call usage xPackUsage though
I'm not convinced that this is the final name for them. But it does get
us started.

= Jars, jars everywhere!

This change makes the xpack:protocol project a compile scoped
dependency of the x-pack:plugin:core and client:rest-high-level
projects. I intend to keep it a compile scoped dependency of
x-pack:plugin:core but I intend to bundle the contents of the protocol
jar into the client:rest-high-level jar in a follow up. This change
has grown large enough at this point.

In that followup I'll address javadoc issues as well.

= Breaking-Java

This breaks that transport client by a few classes around. We've
traditionally been ok with doing this to the transport client.

Break it out from `License` which is already big and would take forever
to move to `x-pack-action`.
This is the first x-pack API we're adding to the high level REST client
so there is a lot to talk about here!

= Open source

The *client* for these APIs is open source. We're taking the previously
Elastic licensed files used for the `Request` and `Response` objects and
relicensing them under the Apache 2 license.

The implementation of these features is staying under the Elastic
license. This lines up with how the rest of the Elasticsearch language
clients work.

= Location of the new files

We're moving all of the `Request` and `Response` objects that we're
relicensing to the `x-pack/protocol` directory. We're adding a copy of
the Apache 2 license to the root fo the `x-pack/protocol` directory to
line up with the language in the root `LICENSE.txt` file. All files in
this directory will have the Apache 2 license header as well. We don't
want there to be any confusion. Even though the files are under the
`x-pack` directory, they are Apache 2 licensed.

We chose this particular directory layout because it keeps the X-Pack
stuff together and easier to think about.

= Location of the API in the REST client

We've been following the layout of the rest-api-spec files for other
APIs and we plan to do this for the X-Pack APIs with one exception:
we're dropping the `xpack` from the name of most of the APIs. So
`xpack.graph.explore` will become `graph().explore()` and
`xpack.license.get` will become `license().get()`.

`xpack.info` and `xpack.usage` are special here though because they
don't belong to any proper category. For now I'm just calling
`xpack.info` `xPackInfo()` and intend to call usage `xPackUsage` though
I'm not convinced that this is the final name for them. But it does get
us started.

= Jars, jars everywhere!

This change makes the `xpack:protocol` project a `compile` scoped
dependency of the `x-pack:plugin:core` and `client:rest-high-level`
projects. I intend to keep it a compile scoped dependency of
`x-pack:plugin:core` but I intend to bundle the contents of the protocol
jar into the `client:rest-high-level` jar in a follow up. This change
has grown large enough at this point.

In that followup I'll address javadoc issues as well.
@elasticmachine
Copy link
Collaborator

Pinging @elastic/es-core-infra

@@ -41,6 +41,7 @@ dependencies {
compile "org.elasticsearch.plugin:aggs-matrix-stats-client:${version}"
compile "org.elasticsearch.plugin:rank-eval-client:${version}"
compile "org.elasticsearch.plugin:lang-mustache-client:${version}"
compile project(':x-pack:protocol') // TODO bundle into the jar
Copy link
Member Author

Choose a reason for hiding this comment

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

Plan to do in a follow up.

* @return the response
* @throws IOException in case there is a problem sending the request or parsing back the response
*/
public XPackInfoResponse xPackInfo(XPackInfoRequest request, RequestOptions options) throws IOException {
Copy link
Member Author

Choose a reason for hiding this comment

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

See commit message for explanation of the location.

Copy link
Contributor

Choose a reason for hiding this comment

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

I see the point in this. Im not sure we should diverge from what the other language clients do here until we all do it, tho. Ill let you talk with @elastic/es-clients about it :)

Copy link
Member Author

Choose a reason for hiding this comment

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

I think I'll end up being one of the first of many rather than a deviation.

Copy link
Member

Choose a reason for hiding this comment

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

I would have added XPackClient and exposed it as part of the xpack namespace, that is aligned with what we did up until now and also in sync with the spec. I feel like I may have missed some discussions around this though. Maybe we plan on updating the spec as well at some point?

@@ -31,7 +36,6 @@ public void testPing() throws IOException {
assertTrue(highLevelClient().ping(RequestOptions.DEFAULT));
}

@SuppressWarnings("unchecked")
Copy link
Member Author

Choose a reason for hiding this comment

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

This just bothered me.

@@ -411,6 +411,7 @@ public String toString() {
INT_ARRAY(START_ARRAY, VALUE_NUMBER, VALUE_STRING),
BOOLEAN_ARRAY(START_ARRAY, VALUE_BOOLEAN),
OBJECT(START_OBJECT),
OBJECT_OR_NULL(START_OBJECT, VALUE_NULL),
Copy link
Member Author

Choose a reason for hiding this comment

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

Required to handle license coming back as null sometimes instead of just missing.

@@ -767,41 +768,6 @@ public Builder validate() {
}
}

public enum Status {
Copy link
Member Author

Choose a reason for hiding this comment

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

I extracted this from License.java because License.java is big and will be much harder to move.

@@ -48,36 +46,6 @@ public RestChannelConsumer doPrepareRequest(RestRequest request, XPackClient cli
client.prepareInfo()
.setVerbose(verbose)
.setCategories(categories)
.execute(new RestBuilderListener<XPackInfoResponse>(channel) {
Copy link
Member Author

Choose a reason for hiding this comment

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

Moved to XPackInfoResponse.

*/

/**
* Request and Response objects for the default distribution's Security
Copy link
Member Author

Choose a reason for hiding this comment

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

I wanted to make these to make it obvious to folks who do the next APIs where I planned for them to stick the files. Not that they have to follow my plan though. I just want to make it obvious that we had a plan.

import static org.elasticsearch.common.xcontent.ConstructingObjectParser.constructorArg;
import static org.elasticsearch.common.xcontent.ConstructingObjectParser.optionalConstructorArg;

public class XPackInfoResponse extends ActionResponse implements ToXContentObject {
Copy link
Member Author

Choose a reason for hiding this comment

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

Github didn't pick up that this is a move.... oh well.

Copy link
Contributor

Choose a reason for hiding this comment

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

you touched it last!

* licenseInfo is sort of "double optional" because it is
* optional but it can also be send as `null`.
*/
PARSER.declareField(optionalConstructorArg(), (p, v) -> {
Copy link
Member Author

Choose a reason for hiding this comment

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

ConstructingObjectParser worked almost perfectly here except for this tiny little wrinkle. It was easy enough to handle inline like this though.

builder.field("build", buildInfo, params);
}

EnumSet<XPackInfoRequest.Category> categories = XPackInfoRequest.Category
Copy link
Member Author

Choose a reason for hiding this comment

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

Before this change we'd read this information from the XPackInfoRequest object. With this change we parse it twice. I don't think that is a big loss though because this API isn't performance critical and toXContent can't see the request anyway.

@nik9000 nik9000 requested a review from hub-cap July 6, 2018 16:08
@cloudenv
Copy link

cloudenv commented Jul 6, 2018

Relicense everything or nothing.... have you ever read GPL? Have you got any idea how FOSS works? Based on this PR, I must say that answer to both questions is no

@nik9000
Copy link
Member Author

nik9000 commented Jul 6, 2018

Relicense everything or nothing.... have you ever read GPL? Have you got any idea how FOSS works? Based on this PR, I must say that answer to both questions is no

You reminded me to go an reread the GPL and Apache 2 license. It'd been a few years since I'd done more than glanced at them and it's been a few years since I've written GPLed software. I'm fairly sure this is all in compliance with the Apache 2 license but I admit I'm no expert.

I certainly feel better when my open source code is talking to other open source code. It just feels cleaner to me. More pure and more righteous. I spent a while writing GPLv2 code to talk Elasticsearch and never integrated with x-pack and liked it that way. But I don't think that is all open source is about. I feel like open source clients for non-open source servers is a fine place for open source software to be because more open source software and more open-ness is a good thing.

I know that isn't really a great explanation and that these Request and Response objects aren't much to be open sourcing but they are something. I feel like this is a step in the right direction and I feel justified in feeling that way.

Copy link
Contributor

@hub-cap hub-cap left a comment

Choose a reason for hiding this comment

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

Aside from the total weirdness of the null license, the default of no categories vs all categories, and the very few nits, this was a nice and tidy review. 👍

@@ -1065,6 +1068,19 @@ static Request deleteScript(DeleteStoredScriptRequest deleteStoredScriptRequest)
return request;
}

static Request xPackInfo(XPackInfoRequest infoRequest) {
Request request = new Request(HttpGet.METHOD_NAME, "/_xpack");
if (false == infoRequest.isVerbose()) {
Copy link
Contributor

Choose a reason for hiding this comment

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

you can use withHuman here

Copy link
Member Author

Choose a reason for hiding this comment

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

I can't because it is backwards! This API's defaults to human on the REST side if it isn't specified but withHuman is for APIs that are the other way around.

assertEquals("basic", info.getLicenseInfo().getMode());
assertEquals(LicenseStatus.ACTIVE, info.getLicenseInfo().getStatus());

FeatureSet graph = info.getFeatureSetsInfo().getFeatureSets().get("graph");
Copy link
Contributor

Choose a reason for hiding this comment

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

are we considering all of the description etc a contract that we need to validate does not change? The reason i ask is cuz maybe it does not make sense to test this much detail as to what the output of the strings are.. I get that we can easily check available/enabled, but id hate to see a test fail here cuz we changed the description (unless we view it as a contract)

Copy link
Member Author

Choose a reason for hiding this comment

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

I'll remove the words, yeah.

import static org.elasticsearch.common.xcontent.ConstructingObjectParser.constructorArg;
import static org.elasticsearch.common.xcontent.ConstructingObjectParser.optionalConstructorArg;

public class XPackInfoResponse extends ActionResponse implements ToXContentObject {
Copy link
Contributor

Choose a reason for hiding this comment

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

you touched it last!

* @return the response
* @throws IOException in case there is a problem sending the request or parsing back the response
*/
public XPackInfoResponse xPackInfo(XPackInfoRequest request, RequestOptions options) throws IOException {
Copy link
Contributor

Choose a reason for hiding this comment

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

I see the point in this. Im not sure we should diverge from what the other language clients do here until we all do it, tho. Ill let you talk with @elastic/es-clients about it :)

@nik9000 nik9000 merged commit fb27f3e into elastic:master Jul 8, 2018
nik9000 added a commit that referenced this pull request Jul 9, 2018
This is the first x-pack API we're adding to the high level REST client
so there is a lot to talk about here!

= Open source

The *client* for these APIs is open source. We're taking the previously
Elastic licensed files used for the `Request` and `Response` objects and
relicensing them under the Apache 2 license.

The implementation of these features is staying under the Elastic
license. This lines up with how the rest of the Elasticsearch language
clients work.

= Location of the new files

We're moving all of the `Request` and `Response` objects that we're
relicensing to the `x-pack/protocol` directory. We're adding a copy of
the Apache 2 license to the root fo the `x-pack/protocol` directory to
line up with the language in the root `LICENSE.txt` file. All files in
this directory will have the Apache 2 license header as well. We don't
want there to be any confusion. Even though the files are under the
`x-pack` directory, they are Apache 2 licensed.

We chose this particular directory layout because it keeps the X-Pack
stuff together and easier to think about.

= Location of the API in the REST client

We've been following the layout of the rest-api-spec files for other
APIs and we plan to do this for the X-Pack APIs with one exception:
we're dropping the `xpack` from the name of most of the APIs. So
`xpack.graph.explore` will become `graph().explore()` and
`xpack.license.get` will become `license().get()`.

`xpack.info` and `xpack.usage` are special here though because they
don't belong to any proper category. For now I'm just calling
`xpack.info` `xPackInfo()` and intend to call usage `xPackUsage` though
I'm not convinced that this is the final name for them. But it does get
us started.

= Jars, jars everywhere!

This change makes the `xpack:protocol` project a `compile` scoped
dependency of the `x-pack:plugin:core` and `client:rest-high-level`
projects. I intend to keep it a compile scoped dependency of
`x-pack:plugin:core` but I intend to bundle the contents of the protocol
jar into the `client:rest-high-level` jar in a follow up. This change
has grown large enough at this point.

In that followup I'll address javadoc issues as well.

= Breaking-Java

This breaks that transport client by a few classes around. We've
traditionally been ok with doing this to the transport client.
* @param options the request options (e.g. headers), use {@link RequestOptions#DEFAULT} if nothing needs to be customized
* @param listener the listener to be notified upon request completion
*/
public void xPackInfoAsync(XPackInfoRequest request, RequestOptions options,
Copy link
Member

Choose a reason for hiding this comment

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

do we need the async version of this? Maybe we could do without?

@javanna
Copy link
Member

javanna commented Jul 9, 2018

I hadn't realized that this PR was merged, so I left a couple of review comments but it's probably better to summarize them here: I think that it's super important that we follow the naming from our REST spec, which define the conventions that all our language clients follow. Like we would do with other plugins, we should have an XPackClient that folds all the xpack functionality and exposes it under the xpack namespace defined in the SPEC. Each product should have a specific client object (GraphClient) which is exposed through XPackClient using the product namespace (e.g. graph). The general methods like xpack.info and xpack.usage should be exposed by the XPackClient and not get mixed up in the ordinary RestHighLevelClient.

I am not sure whether we have plans to rename some of these namespaces but this is what is defined in our current SPEC and we should follow such conventions. If one day the SPEC change then all of the clients will have to adapt and potentially break existing applications.

@hub-cap
Copy link
Contributor

hub-cap commented Jul 9, 2018

hah, i totally deleted a comment about that @javanna and even researched the other clients, because i thought we would be doing the renaming portion on this client. Sorry about letting that slip by. 🤦‍♂️

nik9000 added a commit that referenced this pull request Jul 10, 2018
Originally I put the X-Pack info object into the top level rest client
object. I did that because we thought we'd like to squash `xpack` from
the name of the X-Pack APIs now that it is part of the default
distribution. We still kind of want to do that, but at least for now we
feel like it is better to keep the high level rest client aligned with
the other language clients like C# and Python. This shifts the X-Pack
info API to align with its json spec file.

Relates to #31870
nik9000 added a commit that referenced this pull request Jul 10, 2018
Originally I put the X-Pack info object into the top level rest client
object. I did that because we thought we'd like to squash `xpack` from
the name of the X-Pack APIs now that it is part of the default
distribution. We still kind of want to do that, but at least for now we
feel like it is better to keep the high level rest client aligned with
the other language clients like C# and Python. This shifts the X-Pack
info API to align with its json spec file.

Relates to #31870
dnhatn added a commit that referenced this pull request Jul 12, 2018
* master:
  [TEST] Mute SlackMessageTests.testTemplateRender
  Docs: Explain closing the high level client
  [ML] Re-enable memory limit integration tests (#31328)
  [test] disable packaging tests for suse boxes
  Add nio transport to security plugin (#31942)
  XContentTests : Insert random fields at random positions (#30867)
  Force execution of fetch tasks (#31974)
  Fix unreachable error condition in AmazonS3Fixture (#32005)
  Tests: Fix SearchFieldsIT.testDocValueFields (#31995)
  Add Expected Reciprocal Rank metric (#31891)
  [ML] Get ForecastRequestStats doc in RestoreModelSnapshotIT (#31973)
  SQL: Add support for single parameter text manipulating functions (#31874)
  [ML] Ensure immutability of MlMetadata (#31957)
  Tests: Mute SearchFieldsIT.testDocValueFields()
  muted tests due to #31940
  Work around reported problem in eclipse (#31960)
  Move build integration tests out of :buildSrc project (#31961)
  Tests: Remove use of joda time in some tests (#31922)
  [Test] Reactive 3rd party tests on CI (#31919)
  SQL: Support for escape sequences (#31884)
  SQL: HAVING clause should accept only aggregates (#31872)
  Docs: fix typo in datehistogram (#31972)
  Switch url repository rest tests to new style requests (#31944)
  Switch reindex tests to new style requests (#31941)
  Docs: Added note about cloud service to installation and getting started
  [DOCS] Removes alternative docker pull example (#31934)
  Add Snapshots Status API to High Level Rest Client (#31515)
  ingest: date_index_name processor template resolution (#31841)
  Test: fix null failure in watcher test (#31968)
  Switch test framework to new style requests (#31939)
  Switch low level rest tests to new style Requests (#31938)
  Switch high level rest tests to new style requests (#31937)
  [ML] Mute test failing due to Java 11 date time format parsing bug (#31899)
  [TEST] Mute SlackMessageTests.testTemplateRender
  Fix assertIngestDocument wrongfully passing (#31913)
  Remove unused reference to filePermissionsCache (#31923)
  rolling upgrade should use a replica to prevent relocations while running a scroll
  HLREST: Bundle the x-pack protocol project (#31904)
  Increase logging level for testStressMaybeFlush
  Added lenient flag for synonym token filter (#31484)
  [X-Pack] Beats centralized management: security role + licensing (#30520)
  HLRest: Move xPackInfo() to xPack().info() (#31905)
  Docs: add security delete role to api call table (#31907)
  [test] port archive distribution packaging tests (#31314)
  Watcher: Slack message empty text (#31596)
  [ML] Mute failing DetectionRulesIT.testCondition() test
  Fix broken NaN check in MovingFunctions#stdDev() (#31888)
  Date: Add DateFormatters class that uses java.time (#31856)
  [ML] Switch native QA tests to a 3 node cluster (#31757)
  Change trappy float comparison (#31889)
  Fix building AD URL from domain name (#31849)
  Add opaque_id to audit logging (#31878)
  re-enable backcompat tests
  add support for is_write_index in put-alias body parsing (#31674)
  Improve release notes script (#31833)
  [DOCS] Fix broken link in painless example
  Handle missing values in painless (#30975)
  Remove the ability to index or query context suggestions without context (#31007)
  Ingest: Enable Templated Fieldnames in Rename (#31690)
  [Docs] Fix typo in the Rollup API Quick Reference (#31855)
  Ingest: Add ignore_missing option to RemoveProc (#31693)
  Add template config for Beat state to X-Pack Monitoring (#31809)
  Watcher: Add ssl.trust email account setting (#31684)
  Remove link to oss-MSI (#31844)
  Painless: Restructure Definition/Whitelist (#31879)
  HLREST: Add x-pack-info API (#31870)
dnhatn added a commit that referenced this pull request Jul 12, 2018
* 6.x:
  Force execution of fetch tasks (#31974)
  [TEST] Mute SlackMessageTests.testTemplateRender
  Docs: Explain closing the high level client
  [test] disable java packaging tests for suse
  XContentTests : Insert random fields at random positions (#30867)
  Add Get Snapshots High Level REST API (#31980)
  Fix unreachable error condition in AmazonS3Fixture (#32005)
  [6.x][ML] Ensure immutability of MlMetadata (#31994)
  Add Expected Reciprocal Rank metric (#31891)
  SQL: Add support for single parameter text manipulating functions (#31874)
  muted tests due to #31940
  Work around reported problem in eclipse (#31960)
  Move build integration tests out of :buildSrc project (#31961)
  [Test] Reactive 3rd party tests on CI (#31919)
  Fix assertIngestDocument wrongfully passing (#31913) (#31951)
  SQL: Support for escape sequences (#31884)
  SQL: HAVING clause should accept only aggregates (#31872)
  Docs: fix typo in datehistogram (#31972)
  Switch url repository rest tests to new style requests (#31944)
  Switch reindex tests to new style requests (#31941)
  Switch test framework to new style requests (#31939)
  Docs: Added note about cloud service to installation and getting started
  [DOCS] Removes alternative docker pull example (#31934)
  ingest: date_index_name processor template resolution (#31841)
  Test: fix null failure in watcher test (#31968)
  Watcher: Slack message empty text (#31596)
  Switch low level rest tests to new style Requests (#31938)
  Switch high level rest tests to new style requests (#31937)
  HLREST: Bundle the x-pack protocol project (#31904)
  [ML] Mute test failing due to Java 11 date time format parsing bug (#31899)
  Increase logging level for testStressMaybeFlush
  rolling upgrade should use a replica to prevent relocations while running a scroll
  [test] port archive distribution packaging tests (#31314)
  HLRest: Move xPackInfo() to xPack().info() (#31905)
  Increase logging level for :qa:rolling-upgrade
  Backport: Add template config for Beat state to X-Pack Monitoring (#31809) (#31893)
  Fix building AD URL from domain name (#31849)
  Fix broken NaN check in MovingFunctions#stdDev() (#31888)
  Change trappy float comparison (#31889)
  Add opaque_id to audit logging (#31878)
  add support for is_write_index in put-alias body parsing (#31674)
  Ingest: Enable Templated Fieldnames in Rename (#31690) (#31896)
  Ingest: Add ignore_missing option to RemoveProc (#31693) (#31892)
  [Docs] Fix typo in the Rollup API Quick Reference (#31855)
  Watcher: Add ssl.trust email account setting (#31684)
  [PkiRealm] Invalidate cache on role mappings change (#31510)
  [Security] Check auth scheme case insensitively (#31490)
  HLREST: Add x-pack-info API (#31870)
  Remove link to oss-MSI (#31844)
  Painless: Restructure Definition/Whitelist (#31879)
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