Add Close Index API to the high level REST client #27734

olcbean · 2017-12-08T20:39:31Z

Add _close to the high level REST client

Relates to #27205

minor formatting issues

elasticmachine · 2017-12-08T20:39:32Z

Since this is a community submitted pull request, a Jenkins build has not been kicked off automatically. Can an Elastic organization member please verify the contents of this patch and then kick off a build manually?

javanna

I left one small comment on testing, LGTM otherwise, thanks a lot @olcbean as usual, good stuff!

javanna · 2017-12-08T21:15:34Z

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

-            indices[i] = "index-" + randomAlphaOfLengthBetween(2, 5);
-        }
-        deleteIndexRequest.indices(indices);
+        String[] indices = IndicesClientIT.randomIndicesNames(0, 5);


I am not too happy with sharing this method between an integration test and this unit test. Furthermore, I have second thoughts on the randomization of indices in the open close index test. Maybe we should rather have randomization only here, as in a unit test it fits better, while in an integration test it can cause noise as there are many more moving parts.

olcbean · 2017-12-10T19:57:31Z

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

+     * See <a href="https://www.elastic.co/guide/en/elasticsearch/reference/current/indices-open-close.html">
+     * Close Index API on elastic.co</a>
+     */
+    public CloseIndexResponse closeIndex(CloseIndexRequest closeIndexRequest, Header... headers) throws IOException {


@javanna what about removing the *Index suffix from the method names in the IndicesClient - similar to what is done in the IndicesAdminClient, where one simply calls create to trigger a create index request or close to trigger a close index request ? Then the user will simply call client.indices().close() instead of client.indices().closeIndex().

that is a very good point @olcbean. I missed this subtle difference. Let's do it in a followup PR please.

javanna · 2017-12-11T10:59:46Z

test this please

javanna · 2017-12-11T13:29:00Z

one thing @olcbean I think we need add docs for the new method, do you have time to do it?

olcbean · 2017-12-11T17:42:30Z

@javanna can I also add docs for open index here, or better a separate PR ?

javanna · 2017-12-11T18:20:05Z

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

 include::scroll.asciidoc[]
+


do we need these extra empty lines?

not really... A good example is
https://github.com/elastic/elasticsearch/blob/master/docs/java-api/admin/indices/index.asciidoc ( where both approaches are present ;) )
Basically if there is no extra new line, the two terms are displayed on the same line.

javanna · 2017-12-11T18:22:54Z

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

+--------------------------------------------------
+include-tagged::{doc-tests}/IndicesClientDocumentationIT.java[close-index-request-masterTimeout]
+--------------------------------------------------
+<1> Timeout to connect to the master node as a `TimeValue`


can you also mention indices options in the docs please?

javanna · 2017-12-11T18:23:20Z

@olcbean oh oh did I merge that other PR without docs? :) please if you could add them as part of this PR it would be great. Thank you!

javanna · 2017-12-11T21:36:57Z

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

+--------------------------------------------------
+<1> Indicates whether all of the nodes have acknowledged the request
+<2> Indicates whether the requisite number of shard copies were started for
+each shard in the index before timing out


I have these errors when I build the docs:

asciidoc: WARNING: open_index.asciidoc: line 38: no callouts refer to list item 1
asciidoc: WARNING: open_index.asciidoc: line 40: no callouts refer to list item 2
asciidoc: WARNING: open_index.asciidoc: line 80: no callouts refer to list item 2

…to server)

javanna · 2018-01-16T15:25:17Z

test this please

javanna · 2018-01-16T15:26:12Z

hi @olcbean sorry about the delay, I just merged master in and took the liberty to push those changes to your remote. I also triggered a new build, I will merge this as soon as the build is green.

Add support for _close endpoint to the high level REST client Relates to #27205

javanna · 2018-01-17T14:53:43Z

thanks once again @olcbean , onto your next PR ;)

olcbean · 2018-01-17T18:52:55Z

@javanna thanks! As this is now merged I will go ahead and clean up the API : remove the Index suffixes (#27734 (comment)). AFAICS this was the last PRs using the Index suffix, so it should be pretty straight forward.

As the high level client has been released, I guess the deleteIndex should be deprecated first?

javanna · 2018-01-17T20:38:42Z

@olcbean I already took care of the suffix, see #28263 .

olcbean · 2018-01-18T11:31:05Z

@javanna Grumble. I was told to wait for this PR to be merged before proceeding to the clean up as to keep the PRs simple, consistent with the code base till then and as self contained as possible.

I must admit that the dynamics around most of my recent PRs is rather discouraging. There were multiple occurrences in which I was told to wait and in the mean time a core member would sweep in and implement the exact agreed upon changes.

I still find the project interesting, and would like to continue contributing further in my spare time ( perhaps even more involved changesets - #27163 (comment) is still waiting for a green light ). But the current trend seems to limit contributions to the most inconsequential ones and outright prevents non-members working incrementally within the same area.

Is there a way to clarify the policy in regards to my observations above: in other words which kinds of contributions are actually welcome, and which kinds are considered too much of an overhead?

javanna · 2018-01-18T15:30:31Z

I am really sorry @olcbean . My bad, I genuinely thought that taking care of the rename would be a relief to you, also I needed to look into it myself as I was not sure about potential deprecation, backporting, and we needed to discuss that internally.

I am not sure when something like this happened before, if it did that is definitely annoying and not fair to you. I will pay more attention in the future to these aspects.

I can imagine that some frustration comes from a delay in merging some of your PRs recently also, for that I apologize once again, we will try to do better there.

It is indeed a challenge to have external contributors work on big changes, especially when we don't have clear agreement internally on how to move forward. I wouldn't translate that though to saying that this prevents external contributors to work incrementally within the same area, I am actually very happy with your progress and your various PRs that improve step by step the client as well as other areas of Elasticsearch. It is good to know that you would like to work on something more involving, I/we will keep that in mind and ping you whenever something appealing shows up (cc @clintongormley .

As for your request for some policy, we don't have such a policy and we tend to handle that case by case. In a lot of cases we worry a lot about potentially having external contributors spend a lot of time on something that is controversial and that we are not sure will be merged (that is the case for #27163 (comment)). In general, on big changes it is always good to ping us before starting so we can advice on how to move forward etc.

Thanks for raising your concerns, that helps making things better on our end!

olcbean · 2018-01-18T17:45:54Z

Thank you @javanna for the detailed response. I can see how much of this was unintentional. I hope my venting wasn't taken the wrong way ;)

I want to address a specific point towards the end: I certainly did not mean to imply that the project needs a complex written down policy regarding contributions - that'd be rather unpleasant for everyone involved. Having awareness within the team that sometimes things go sideways is more than enough.

Thanks!

javanna · 2018-01-19T10:16:39Z

No worries @olcbean nothing was taken the wrong way, we appreciate you reaching out with your concerns.

olcbean added 2 commits December 8, 2017 19:14

remove redundant import-s

68666a9

minor formatting issues

Add Close Index API to the high level REST client

ee8d666

javanna self-requested a review December 8, 2017 21:10

javanna added :Java High Level REST Client >enhancement v7.0.0 v6.2.0 labels Dec 8, 2017

javanna self-assigned this Dec 8, 2017

javanna approved these changes Dec 8, 2017

View reviewed changes

tests clean up : requests to multiple indices done from UT instead of IT

1b86d80

olcbean commented Dec 10, 2017

View reviewed changes

adding docs for close index

4760c5d

olcbean force-pushed the hlrc_close_index branch from e1b114f to 4760c5d Compare December 11, 2017 17:20

javanna reviewed Dec 11, 2017

View reviewed changes

add docs for open index

4c2626c

olcbean force-pushed the hlrc_close_index branch from 6255108 to 4c2626c Compare December 11, 2017 20:09

javanna reviewed Dec 11, 2017

View reviewed changes

olcbean and others added 3 commits December 12, 2017 11:38

fix IndicesClientDocumentionIT for open index and acknowledged shards

e2767be

Merge branch 'master' into hlrc_close_index

c36d286

amended wait for active shards explanation

623da6a

elastic deleted a comment from elasticmachine Jan 16, 2018

move CloseIndexResponseTests to server module (core has been renamed …

015811c

…to server)

javanna merged commit b98514c into elastic:master Jan 17, 2018

javanna added the backport pending label Jan 17, 2018

javanna pushed a commit that referenced this pull request Jan 17, 2018

Add Close Index API to the high level REST client (#27734)

dd92fb6

Add support for _close endpoint to the high level REST client Relates to #27205

javanna removed the backport pending label Jan 17, 2018

jtibshirani mentioned this pull request Apr 23, 2018

Add support for field capabilities to the high-level REST client. #29664

Merged

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

Add Close Index API to the high level REST client #27734

Add Close Index API to the high level REST client #27734

olcbean commented Dec 8, 2017

elasticmachine commented Dec 8, 2017

javanna left a comment

javanna Dec 8, 2017 •

edited

Loading

olcbean Dec 10, 2017

javanna Dec 11, 2017 •

edited

Loading

javanna commented Dec 11, 2017

javanna commented Dec 11, 2017

olcbean commented Dec 11, 2017

javanna Dec 11, 2017

olcbean Dec 11, 2017

javanna Dec 11, 2017

javanna commented Dec 11, 2017

javanna Dec 11, 2017

javanna commented Jan 16, 2018

javanna commented Jan 16, 2018

javanna commented Jan 17, 2018

olcbean commented Jan 17, 2018

javanna commented Jan 17, 2018

olcbean commented Jan 18, 2018

javanna commented Jan 18, 2018

olcbean commented Jan 18, 2018

javanna commented Jan 19, 2018

Add Close Index API to the high level REST client #27734

Add Close Index API to the high level REST client #27734

Conversation

olcbean commented Dec 8, 2017

elasticmachine commented Dec 8, 2017

javanna left a comment

Choose a reason for hiding this comment

javanna Dec 8, 2017 • edited Loading

Choose a reason for hiding this comment

olcbean Dec 10, 2017

Choose a reason for hiding this comment

javanna Dec 11, 2017 • edited Loading

Choose a reason for hiding this comment

javanna commented Dec 11, 2017

javanna commented Dec 11, 2017

olcbean commented Dec 11, 2017

javanna Dec 11, 2017

Choose a reason for hiding this comment

olcbean Dec 11, 2017

Choose a reason for hiding this comment

javanna Dec 11, 2017

Choose a reason for hiding this comment

javanna commented Dec 11, 2017

javanna Dec 11, 2017

Choose a reason for hiding this comment

javanna commented Jan 16, 2018

javanna commented Jan 16, 2018

javanna commented Jan 17, 2018

olcbean commented Jan 17, 2018

javanna commented Jan 17, 2018

olcbean commented Jan 18, 2018

javanna commented Jan 18, 2018

olcbean commented Jan 18, 2018

javanna commented Jan 19, 2018

javanna Dec 8, 2017 •

edited

Loading

javanna Dec 11, 2017 •

edited

Loading