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

sohaibiftikhar · 2018-05-24T19:13:45Z

Relates to #27205

Requires REST high-level client: add put ingest pipeline API #30793 to be merged first.

elasticmachine · 2018-05-24T19:30:56Z

Pinging @elastic/es-core-infra

imotov · 2018-05-24T19:31:27Z

@elasticmachine add to whitelist

sohaibiftikhar · 2018-05-25T10:56:08Z

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

TODO: Fix javadoc link.

sohaibiftikhar · 2018-05-25T11:09:22Z

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

Change this to some_pipeline_id for uniformity.

nik9000

I left just one question. Other than that it looks good!

nik9000 · 2018-05-25T17:21:24Z

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

What about making this an AbstractStreamableXContentTestCase like you did in #30793?

javanna · 2018-05-28T12:16:11Z

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

Along the lines of the other comment I left on #30793 , this API should be added under the ingest namespace rather than the cluster one.

I would prefer to pick up moving to a new namespace after this and #30865 have been merged. Is this okay? Or do I need to bundle it with this change?

sure fine with me

javanna

LGTM besides the comment I left on the API namespace

Relates to elastic#27205 (cherry picked from commit 696b0ab2d09d4d25806b3fbfd4f82b6fc9f7218b)

-- Also fixed strings -- removed getPipelineConfigs method and switched to pipelines() method.

javanna · 2018-05-29T13:07:35Z

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

+            clonePipelines.add(createRandomPipeline("pipeline_" + clonePipelines.size() + 1));
+            return new GetPipelineResponse(clonePipelines);
+        } catch (IOException e) {
+            return null;


rather throw UncheckedIOException ?

Thanks for the tip. I didn't know these existed.

javanna · 2018-05-29T13:07:47Z

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

+    @Override
+    protected GetPipelineResponse mutateInstance(GetPipelineResponse response) {
+        try {
+            ArrayList<PipelineConfiguration> clonePipelines = new ArrayList<>(response.pipelines());


nit: can you declare it as a list on the left side?

You mean left side?

yes :) I have edited my comment right after posting it :)

javanna · 2018-05-29T13:08:25Z

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

+        } catch (IOException e) {
+            // If we fail to construct an instance we return `null` which would make the user of this method
+            // fail the test.
+            return null;


I would rather throw UncheckedIOException here

javanna · 2018-05-29T13:09:24Z

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

+        int result = 1;
+        for (PipelineConfiguration pipeline: pipelines) {
+            // We only take the sum here to ensure that the order does not matter.
+            result += (pipeline == null ? 0 : pipeline.getConfigAsMap().hashCode());


shall we add equals and hashcode to PipelineConfiguration ?

It is already there. Unfortunately, it uses the BytesReference to do this. This depends on the XContentType and hence is not useful for comparison. Do you want me to change the implementation there?

I think we could, I am assuming that we need these only for tests and making equals/hashcode work with the map representation would not hurt. You can try and run IngestMetadaTests, PipelineConfigurationTests, PipelineExecutionServiceTests and PipelineStoreTests to check whether they remain green after the change.

Cool thanks! Will do the change.

sohaibiftikhar · 2018-05-29T13:26:43Z

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

    }

    public GetPipelineResponse(List<PipelineConfiguration> pipelines) {
        this.pipelines = pipelines;


@javanna Should I change this constructor to Set<PipelineConfiguration> instead of List<PipelineConfiguration> to keep it more in line with the actual JSON response? Then we don't need to do tricky stuff in the equals and hashcode. I did not do this before because PipelineConfiguration did not have the ideal equals and hashcode.

I would be ok with that, but let's first see whether tests are fine with this change, my assumptions may be wrong :)

-- also throw UncheckedIOException instead of returning null

javanna · 2018-05-30T10:25:02Z

@sohaibiftikhar when I build the docs I get

asciidoc: WARNING: get_pipeline.asciidoc: line 14: callout refers to non-existent list item 2

could you please look into that?

sohaibiftikhar · 2018-05-30T11:07:12Z

@javanna I am confused. Line 14 is a blank line. Plus there is no list item <2> for that part. I could not replicate it as well. Could you give me the exact command to replicate this?

sohaibiftikhar · 2018-05-30T11:19:05Z

Okay, I found it. The end tag had a mistake. Still would be helpful to get the command though. Thanks!

javanna · 2018-05-30T11:35:55Z

here is the command that I run (you need to check out the docs repo first) from the root of my elasticsearch checkout:

/path/to/elastic/docs/build_docs.pl --doc docs/java-rest/index.asciidoc --chunk 1 --out ~/temp/asciidoc  --open

javanna · 2018-05-30T12:25:12Z

retest this please

javanna · 2018-05-30T14:19:15Z

@sohaibiftikhar would you mind pulling master in please?

javanna · 2018-05-30T18:43:53Z

nevermind, I took care of it, hopefully tests get green, failures were unrelated to your changes.

javanna · 2018-05-31T06:58:14Z

retest this please

javanna · 2018-05-31T14:05:52Z

retest this please

sohaibiftikhar · 2018-05-31T16:20:10Z

works finally!

javanna · 2018-06-01T06:55:50Z

thanks @sohaibiftikhar !

Relates to #27205

* 6.x: Adjust BWC version on client features Introduce client feature tracking (#31020) [DOCS] Make geoshape docs less memory hungry (#31014) Fix handling of percent-encoded spaces in Windows batch files (#31034) [Docs] Fix a typo in Create Index naming limitation (#30891) REST high-level client: add delete ingest pipeline API (#30865) Ensure that index_prefixes settings cannot be changed (#30967) REST high-level client: add get ingest pipeline API (#30847) Cross Cluster Search: preserve remote status code (#30976) High-level client: list tasks failure to not lose nodeId (#31001) Refactor Sniffer and make it testable (#29638) [ML][TEST] Fix bucket count assertion in all tests in ModelPlotsIT (#31026) Add an option to split keyword field on whitespace at query time (#30691) Allow rollup job creation only if cluster is x-pack ready (#30963) Fix interoperability with < 6.3 transport clients (#30971) [Tests] Fix alias names in PutIndexTemplateRequestTests (#30960) [DOCS] Fixes links (#31011) Watcher: Give test a little more time

* master: Avoid randomization bug in FeatureAwareTests Adjust BWC version on client features Add TRACE, CONNECT, and PATCH http methods (#31035) Adjust BWC version on client features [DOCS] Make geoshape docs less memory hungry (#31014) Fix handling of percent-encoded spaces in Windows batch files (#31034) [Docs] Fix a typo in Create Index naming limitation (#30891) Introduce client feature tracking (#31020) Ensure that index_prefixes settings cannot be changed (#30967) REST high-level client: add delete ingest pipeline API (#30865) [ML][TEST] Fix bucket count assertion in all tests in ModelPlotsIT (#31026) Allow rollup job creation only if cluster is x-pack ready (#30963) Fix interoperability with < 6.3 transport clients (#30971) Add an option to split keyword field on whitespace at query time (#30691) [Tests] Fix alias names in PutIndexTemplateRequestTests (#30960) REST high-level client: add get ingest pipeline API (#30847) Cross Cluster Search: preserve remote status code (#30976) High-level client: list tasks failure to not lose nodeId (#31001) [DOCS] Fixes links (#31011) Watcher: Give test a little more time Reuse expiration date of trial licenses (#30950) Remove unused query methods from MappedFieldType. (#30987) Transport client: Don't validate node in handshake (#30737) [DOCS] Clarify not all PKCS12 usable as truststores (#30750) HLRest: Allow caller to set per request options (#30490) Remove version read/write logic in Verify Response (#30879) [DOCS] Update readme for testing x-pack code snippets (#30696) Ensure intended key is selected in SamlAuthenticatorTests (#30993) Core: Remove RequestBuilder from Action (#30966)

imotov requested review from javanna and nik9000 May 24, 2018 19:30

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

sohaibiftikhar force-pushed the get_pipeline branch from 696b0ab to 7e97845 Compare May 25, 2018 08:29

sohaibiftikhar commented May 25, 2018

View reviewed changes

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

Copy link

Contributor Author

sohaibiftikhar May 25, 2018

Choose a reason for hiding this comment

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

TODO: Fix javadoc link.

sohaibiftikhar commented May 25, 2018

View reviewed changes

sohaibiftikhar mentioned this pull request May 25, 2018

REST high-level client: add delete ingest pipeline API #30865

Merged

1 task

nik9000 reviewed May 25, 2018

View reviewed changes

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

javanna approved these changes May 28, 2018

View reviewed changes

sohaibiftikhar mentioned this pull request May 28, 2018

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

Merged

Added get pipeline API

da82960

Relates to elastic#27205 (cherry picked from commit 696b0ab2d09d4d25806b3fbfd4f82b6fc9f7218b)

sohaibiftikhar force-pushed the get_pipeline branch 2 times, most recently from ecb9bf9 to 8e2aeec Compare May 29, 2018 12:28

extended AbstractStreamableXContentTestCase

e0288f1

-- Also fixed strings -- removed getPipelineConfigs method and switched to pipelines() method.

sohaibiftikhar force-pushed the get_pipeline branch from 8e2aeec to e0288f1 Compare May 29, 2018 12:49

javanna reviewed May 29, 2018

View reviewed changes

sohaibiftikhar commented May 29, 2018

View reviewed changes

Changed equals and hashcode method for PipelineConfiguration

64985cf

-- also throw UncheckedIOException instead of returning null

Fixed get-pipeline-request tag in ClusterClientDocumentationIT

3d46fde

Merge branch 'master' into get_pipeline

6d2e3e0

javanna added 2 commits May 31, 2018 12:10

Merge branch 'master' into get_pipeline

a7a27ae

Merge branch 'master' into get_pipeline

f798622

javanna merged commit 80d20a9 into elastic:master Jun 1, 2018

javanna added the backport pending label Jun 1, 2018

javanna pushed a commit that referenced this pull request Jun 1, 2018

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

7d955f8

Relates to #27205

javanna removed the backport pending label Jun 1, 2018

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

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

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

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

Uh oh!

Conversation

sohaibiftikhar commented May 24, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

elasticmachine commented May 24, 2018

Uh oh!

imotov commented May 24, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nik9000 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!

javanna 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!

javanna May 29, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sohaibiftikhar May 29, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

sohaibiftikhar May 29, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

sohaibiftikhar May 29, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

javanna commented May 30, 2018

Uh oh!

sohaibiftikhar commented May 30, 2018

Uh oh!

sohaibiftikhar commented May 30, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

javanna commented May 30, 2018

Uh oh!

javanna commented May 30, 2018

Uh oh!

javanna commented May 30, 2018

Uh oh!

javanna commented May 30, 2018

Uh oh!

javanna commented May 31, 2018

Uh oh!

javanna commented May 31, 2018

Uh oh!

sohaibiftikhar commented May 24, 2018 •

edited

Loading

javanna May 29, 2018 •

edited

Loading

sohaibiftikhar May 29, 2018 •

edited

Loading

sohaibiftikhar May 29, 2018 •

edited

Loading

sohaibiftikhar May 29, 2018 •

edited

Loading

sohaibiftikhar commented May 30, 2018 •

edited

Loading