REST high-level client: add put ingest pipeline API #30793

sohaibiftikhar · 2018-05-22T19:20:35Z

Relates to #27205

Relates to elastic#27205

imotov · 2018-05-22T20:16:04Z

@elasticmachine add to whitelist

elasticmachine · 2018-05-22T20:17:00Z

Pinging @elastic/es-core-infra

cbuescher

Hi @sohaibiftikhar, great PR, thanks! I did a first round of review and left a few comments but nothing big.

cbuescher · 2018-05-23T08:53:55Z

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

+     * Add a pipeline or update an existing pipeline in the cluster
+     * <p>
+     * See
+     * <a href="https://www.elastic.co/guide/en/elasticsearch/reference/current/tasks.html"> Task Management API on elastic.co</a>


nit: can you update the link to the add pipeline API?

Thanks for reviewing! I see I have made some stupid mistakes. Will do the change.

cbuescher · 2018-05-23T08:54:02Z

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

+     * Asynchronously add a pipeline or update an existing pipeline in the cluster
+     * <p>
+     * See
+     * <a href="https://www.elastic.co/guide/en/elasticsearch/reference/current/tasks.html"> Task Management API on elastic.co</a>


nit: can you update the link to the add pipeline API?

cbuescher · 2018-05-23T08:55:34Z

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

+     * See
+     * <a href="https://www.elastic.co/guide/en/elasticsearch/reference/current/tasks.html"> Task Management API on elastic.co</a>
+     */
+    public void putPipelineAsync(ListTasksRequest request, ActionListener<ListTasksResponse> listener, Header... headers) {


This method seems to be concerned with ListTasksRequest, I think it might have accidentally slipped in?

cbuescher · 2018-05-23T09:07:55Z

...level/src/test/java/org/elasticsearch/client/documentation/ClusterClientDocumentationIT.java

+        RestHighLevelClient client = highLevelClient();
+
+        {
+            // tag::put-pipeline-request


This tag seems to be missing its end

cbuescher · 2018-05-23T09:08:36Z

...level/src/test/java/org/elasticsearch/client/documentation/ClusterClientDocumentationIT.java

+            request.timeout("2m"); // <2>
+            // end::put-pipeline-request-timeout
+            request.masterNodeTimeout(TimeValue.timeValueMinutes(1)); // <1>
+            // tag::put-pipeline-request-masterTimeout


This tag seems to be in the wron place and missing its end.

cbuescher · 2018-05-23T09:17:40Z

server/src/main/java/org/elasticsearch/action/ingest/PutPipelineRequest.java

+    @Override
+    public XContentBuilder toXContent(XContentBuilder builder, Params params) throws IOException {
+        if (source != null) {
+            builder.rawValue(source.streamInput(), XContentType.JSON);


I think this should be the xContentType associated with the source.

cbuescher · 2018-05-23T09:23:23Z

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

+
+    public void testPutPipeline() throws IOException {
+        String id = "some_pipeline_id";
+        XContentBuilder pipelineBuilder = JsonXContent.contentBuilder();


Maybe use a random xContentType here

cbuescher · 2018-05-23T09:24:32Z

server/src/test/java/org/elasticsearch/action/ingest/PutPipelineRequestTests.java

@@ -43,4 +48,25 @@ public void testSerializationWithXContent() throws IOException {
        assertEquals(XContentType.JSON, serialized.getXContentType());
        assertEquals("{}", serialized.getSource().utf8ToString());
    }
+
+    public void testToXContent() throws IOException {
+        XContentBuilder pipelineBuilder = JsonXContent.contentBuilder();


Maybe use a random xContentType and adapt the expected value accordingly

cbuescher · 2018-05-23T09:34:42Z

server/src/main/java/org/elasticsearch/action/ingest/PutPipelineRequest.java

 import org.elasticsearch.common.xcontent.XContentHelper;
 import org.elasticsearch.common.xcontent.XContentType;

 import java.io.IOException;
 import java.util.Objects;

-public class PutPipelineRequest extends AcknowledgedRequest<PutPipelineRequest> {
+public class PutPipelineRequest extends AcknowledgedRequest<PutPipelineRequest> implements ToXContentObject {


I'm not entirely sure if we want to add ToXContent here. Other requests where the body consist of a BytesReference source (e.g. IndexRequest) don't seem to do this, instead we convert them to a ByteArrayEntity in RequestConverters. Not sure whats the benefit of either of those choices, maybe @javanna has an opinion on this.

I was kind of following the example of put mapping for this which does the same. If you guys think it should change I'll make it.

I see, I was curious. I don't have a strong opinion either way at the moment, interested in hearing the pros and cons that others might think of

I have no strong opinion either, it can be convenient to implement ToXContentObject I think. IndexRequest is a bit of a different one as the body is the document itself.

-- Fixed java doc for ClusterClient::putPipeline -- Fixed doc tags in ClusterClientDocumentationIT::testPutPipeline -- Fixed java doc for ClusterClient::putPipelineAsync -- Randomized xContentType in ClusterClientIT::putPipeline -- Randomized xContentType in PutPipelineTests::testToXContent

sohaibiftikhar · 2018-05-23T12:49:08Z

Failures seem unrelated. Was unable to reproduce locally.

cbuescher · 2018-05-23T13:02:41Z

@sohaibiftikhar thanks for the update, looks good to me, but will give everything a final look while tests are running. The failure you see seems unrelated indeed, but we still need a full CI build to pass. Usually it helps merging in the current version of master, hopefully the failure source is fixed in the meantime. Could you do that once more? Tests should start automatically I think.

…o put_pipeline

cbuescher

I left two very small remaining remarks about the docs, but the rest looks great. I think this is good, thanks a lot.
I will wait a bit to see if @nik9000 wants to take another look at this, otherwise I think this is ready for merging once the above things are adressed and CI build passes green.

cbuescher · 2018-05-23T14:02:55Z

docs/java-rest/high-level/cluster/put_pipeline.asciidoc

+["source","java",subs="attributes,callouts,macros"]
+--------------------------------------------------
+include-tagged::{doc-tests}/ClusterClientDocumentationIT.java[put-pipeline-execute]
+--------------------------------------------------


Super small follow up request: there is a callout in this exaple (1), but no explanation text. Could you either remove the callout or add some explanation? Otherwise it looks a bit funny on the rendered page, also seems to mess up the following examples.

cbuescher · 2018-05-23T14:03:54Z

...level/src/test/java/org/elasticsearch/client/documentation/ClusterClientDocumentationIT.java

+        {
+            // tag::put-pipeline-request
+            String source =
+                "{\"description\":\"some random set of processors\"," +


nit: can you remove "random" here? Its okay in tests I think, reads better without in documentation though. Just my 2cents though.

…o put_pipeline

sohaibiftikhar · 2018-05-24T14:21:53Z

@cbuescher CI looks good now.

nik9000

I left two minor things but this looks good to me!

nik9000 · 2018-05-24T14:58:20Z

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

+        pipelineBuilder.startObject().field(Pipeline.DESCRIPTION_KEY, "some random set of processors");
+        pipelineBuilder.startArray(Pipeline.PROCESSORS_KEY);
+        //Start first processor
+        pipelineBuilder.startObject();


I tend to like doing something like this:

builder.startObject(); { builder.field(Pipeline.DESCRIPTION_KEY, "some random set of processors"); builder.startArray(Pipeline.PROCESSORS_KEY); { pipelineBuilder.startObject().startObject("set").field("field", "foo").field("value", "bar").endObject().endObject(); pipelineBuilder.startObject().startObject("convert").field("field", "rank").field("type", "integer").endObject().endObject(); } }

The { and } makes block which beg for the "normal" indentation you'd get in json so it kind of lines up.

Thanks for the review. I only saw one thing to do (you mentioned two). Made the change for that.

nik9000 · 2018-05-24T14:59:17Z

...level/src/test/java/org/elasticsearch/client/documentation/ClusterClientDocumentationIT.java

+        RestHighLevelClient client = highLevelClient();
+
+        {
+            // tag::put-pipeline-request


nik9000 · 2018-05-24T15:01:04Z

Sorry I didn't get the ping earlier. I need to be better about emails.

nik9000 · 2018-05-24T23:08:54Z

@sohaibiftikhar Thanks for you patience with this one. I keep getting distracted with upgrade issues. I've merged this to master and am backporting it to 6.x now. That is pretty much just cherry-picking it back and re-running the tests. So in a few hours I ought to merge it to 6.x as well, so long as the tests run well.

* es/master: Move score script context from SearchScript to its own class (#30816) Fix bad version check writing Repository nodes (#30846) [docs] explainer for java packaging tests (#30825) Remove Throwable usage from transport modules (#30845) REST high-level client: add put ingest pipeline API (#30793) Update the version checks around ip_range bucket keys, now that the change was backported. Mute IndexMasterFailoverIT.testMasterFailoverDuringIndexingWithMappingChanges Use geohash cell instead of just a corner in geo_bounding_box (#30698) Limit user to single concurrent auth per realm (#30794) [Tests] Move templated _rank_eval tests (#30679) Security: fix dynamic mapping updates with aliases (#30787) Ensure that ip_range aggregations always return bucket keys. (#30701) Use remote client in TransportFieldCapsAction (#30838) Move Watcher versioning setting to meta field (#30832) [Docs] Explain incomplete dates in range queries (#30689) Move persistent task registrations to core (#30755) Decouple ClusterStateTaskListener & ClusterApplier (#30809) Send client headers from TransportClient (#30803) Packaging: Ensure upgrade_is_oss flag file is always deleted (#30732) Force stable file modes for built packages (#30823)

nik9000 · 2018-05-25T17:18:28Z

I wasn't able to get a clean build last night. I got one this morning but now the intake build is failing. I'm going to wait for that to go green. I think that is another hour and a half or so, assuming it doesn't fail again.

REST high-level client: add put ingest pipeline API Adds the put ingest pipeline API to the high level rest client.

nik9000 · 2018-05-25T18:51:09Z

Backported! Wow that took longer than it ought.....

javanna · 2018-05-28T12:15:24Z

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

+     * See
+     * <a href="https://www.elastic.co/guide/en/elasticsearch/reference/current/put-pipeline-api.html"> Put Pipeline API on elastic.co</a>
+     */
+    public void putPipelineAsync(PutPipelineRequest request, ActionListener<PutPipelineResponse> listener, Header... headers) {


sorry for being late to the party. I just realized that the spec here defines the API as ingest.put_pipeline. Instead of adding this API (and all of the other ingest API) to the cluster ones, we should create a new namespace called ingest, similar to what we recently did with snapshot. @sohaibiftikhar would you mind taking care of this ?

Sure. Should I combine this change with #30847 or put in a separate PR?

I would do it in a separate PR, thanks!

* 6.x: Fix double semicolon in import statement [TEST] Fix minor random bug from #30794 Enabling testing against an external cluster (#30885) SQL: Remove the last remaining server dependencies from jdbc (#30771) Add public key header/footer (#30877) Include size of snapshot in snapshot metadata (#29602) QA: Test template creation during rolling restart (#30850) REST high-level client: add put ingest pipeline API (#30793) Do not serialize basic license exp in x-pack info (#30848) [docs] explainer for java packaging tests (#30825) Verify signatures on official plugins (#30800) [DOCS] Document index name limitations (#30826) [Docs] Add reindex.remote.whitelist example (#30828)

REST high-level client: add put ingest pipeline API

2c5d70e

Relates to elastic#27205

imotov added >enhancement v7.0.0 :Core/Java High Level REST Client v6.4.0 labels May 22, 2018

imotov requested review from javanna and nik9000 May 22, 2018 20:17

cbuescher requested changes May 23, 2018

View reviewed changes

cbuescher self-assigned this May 23, 2018

javanna removed their request for review May 23, 2018 10:33

Merge branch 'master' of https://github.com/elastic/elasticsearch int…

42942b6

…o put_pipeline

cbuescher approved these changes May 23, 2018

View reviewed changes

sohaibiftikhar added 3 commits May 23, 2018 16:40

Fixed documentation strings

4c3c61a

Merge branch 'master' of https://github.com/elastic/elasticsearch int…

a11026b

…o put_pipeline

Merge branch 'master' of https://github.com/elastic/elasticsearch int…

aa0d270

…o put_pipeline

nik9000 approved these changes May 24, 2018

View reviewed changes

sohaibiftikhar added 2 commits May 24, 2018 18:04

fixed XContentBuilder indentation

e77799d

Fixed asciidoc response object reference

977c259

sohaibiftikhar force-pushed the put_pipeline branch from 9aaccfd to 977c259 Compare May 24, 2018 18:07

sohaibiftikhar mentioned this pull request May 24, 2018

REST high-level client: add get ingest pipeline API #30847

Merged

1 task

nik9000 merged commit 5a97423 into elastic:master May 24, 2018

nik9000 pushed a commit that referenced this pull request May 25, 2018

REST high-level client: add put ingest pipeline API (#30793)

b4195d3

REST high-level client: add put ingest pipeline API Adds the put ingest pipeline API to the high level rest client.

javanna mentioned this pull request May 28, 2018

Java high-level REST client completeness #27205

Closed

80 tasks

javanna reviewed May 28, 2018

View reviewed changes

sohaibiftikhar mentioned this pull request Jun 1, 2018

Moved pipeline APIs to ingest namespace #31027

Merged

1 task

sohaibiftikhar deleted the put_pipeline branch June 5, 2018 11:32

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

REST high-level client: add put ingest pipeline API #30793

REST high-level client: add put ingest pipeline API #30793

sohaibiftikhar commented May 22, 2018

imotov commented May 22, 2018

elasticmachine commented May 22, 2018

cbuescher left a comment

cbuescher May 23, 2018

sohaibiftikhar May 23, 2018

cbuescher May 23, 2018

cbuescher May 23, 2018

cbuescher May 23, 2018

nik9000 May 24, 2018

cbuescher May 23, 2018

cbuescher May 23, 2018

cbuescher May 23, 2018

cbuescher May 23, 2018

cbuescher May 23, 2018

sohaibiftikhar May 23, 2018

cbuescher May 23, 2018

javanna May 23, 2018

sohaibiftikhar commented May 23, 2018

cbuescher commented May 23, 2018

cbuescher left a comment

cbuescher May 23, 2018

cbuescher May 23, 2018

sohaibiftikhar commented May 24, 2018

nik9000 left a comment

nik9000 May 24, 2018

sohaibiftikhar May 24, 2018

nik9000 May 24, 2018

nik9000 commented May 24, 2018

nik9000 commented May 24, 2018

nik9000 commented May 25, 2018

nik9000 commented May 25, 2018

javanna May 28, 2018

sohaibiftikhar May 28, 2018 •

edited

Loading

javanna May 29, 2018

REST high-level client: add put ingest pipeline API #30793

REST high-level client: add put ingest pipeline API #30793

Conversation

sohaibiftikhar commented May 22, 2018

imotov commented May 22, 2018

elasticmachine commented May 22, 2018

cbuescher 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

sohaibiftikhar commented May 23, 2018

cbuescher commented May 23, 2018

cbuescher left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sohaibiftikhar commented May 24, 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

Choose a reason for hiding this comment

nik9000 commented May 24, 2018

nik9000 commented May 24, 2018

nik9000 commented May 25, 2018

nik9000 commented May 25, 2018

Choose a reason for hiding this comment

sohaibiftikhar May 28, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sohaibiftikhar May 28, 2018 •

edited

Loading