Add document _count API support to Rest High Level Client. #34267

mrdjen · 2018-10-03T12:12:22Z

Add count() api method, CountRequest and CountResponse classes to HLRC. Code in server module is unchanged.

Relates to #27205

Add `count()` api method, `CountRequest` and `CountResponse` classes to HLRC. Code in server module is unchanged. Relates to elastic#27205

elasticmachine · 2018-10-03T12:18:52Z

Pinging @elastic/es-core-infra

hub-cap · 2018-10-03T18:14:52Z

can you move the count package to core? Ive told a few people to drop the core (things like search, count, etc..) into the core package. None of those PRs have landed yet, so just create it in your PR. im reviewing it now, but i just wanted to ping you with this info so i dont forget :P

hub-cap

Great work so far. We caught you in the middle of some major changes, like the documentation change, so lets clean those up and then ill fire off the CI service against it. <3

hub-cap · 2018-10-03T18:16:10Z

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

@@ -749,6 +751,29 @@ public final void indexAsync(IndexRequest indexRequest, RequestOptions options,
                emptySet());
    }

+    /**
+     * Executes a count request using the Count API


We typically add a webpage to these javadocs. See others for examples

Addressed, let me know what you think.

hub-cap · 2018-10-03T18:18:30Z

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

+    }
+
+    @Override
+    public XContentBuilder toXContent(XContentBuilder builder, Params params) throws IOException {


We do not need toXContent in responses. This will affect your tests, and you can look at #33921 for a good example on how to structure the tests. A lot has been in flux, recently :P

Addressed, let me know what you think. XContentTester is quite useful!

hub-cap · 2018-10-03T18:20:20Z

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

+        }
+
+        @Override
+        public XContentBuilder toXContent(XContentBuilder builder, Params params) throws IOException {


see other msg about the toXContent in responses

hub-cap · 2018-10-03T18:21:05Z

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

+public class CountIT extends ESRestHighLevelClientTestCase {
+
+    @Before
+    public void indexDocuments() throws IOException {


All of this can go in the existing SearchIT, i would imagine.

Moved to SearchIT

hub-cap · 2018-10-03T18:21:57Z

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

+import static org.elasticsearch.test.EqualsHashCodeTestUtils.checkEqualsAndHashCode;
+
+//quite similar to SearchRequestTests as CountRequest wraps SearchRequest
+public class CountRequestTests extends ESTestCase {


lets move these in the same place as the request/response above.

hub-cap · 2018-10-03T18:22:35Z

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

            IllegalArgumentException exception = expectThrows(IllegalArgumentException.class, () -> RequestConverters.bulk(bulkRequest));
            assertEquals(
-                    "Mismatching content-type found for request with content-type [SMILE], " + "previous requests have content-type [JSON]",
-                    exception.getMessage());


pls dont mess with the existing spacing on this class, its causing a big diff :)

Uhm yes, this should be reverted now

hub-cap · 2018-10-03T18:23:43Z

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

@@ -648,7 +648,6 @@ public void testApiNamingConventions() throws Exception {
        //this list should be empty once the high-level client is feature complete
        String[] notYetSupportedApi = new String[]{
            "cluster.remote_info",
-            "count",


hub-cap · 2018-10-03T18:25:04Z

docs/java-rest/high-level/search/count.asciidoc

+[[java-rest-high-count]]
+=== Count API
+
+[[java-rest-high-document-count-request]]


we have very recently changed how we do these docs. Its much simpler now without a lot of copy/pasta. #34157 is a good example

I see. Changed the docs

mrdjen · 2018-10-04T12:29:42Z

Thanks for comments @hub-cap - I'm addressing these, can you please clarify

can you move the count package to core

Do you reffer to org.elasticsearch.client package in rest-high-level or shoud I create core and move the count package there? (As I don't see core)

mrdjen · 2018-10-04T13:46:06Z

OK, I get it, I will create core package

- move CountRequest and CountResponse into org.elasticsearch.client.core - remove ToXContent in CountResponse and use XContentTester in tests - remove CountIT and CountDocumentationIT classes, add the tests into SearchIT and SearchDocumentationIT - use new markdown and structure in count.asciidoc docs - add javadocs in RestHighLevelClient and revert existing spacing in RestHighLevelClientTests

mrdjen · 2018-10-09T10:13:27Z

Hi @hub-cap, I've addressed changes you've requested. Let me know what you think.

Yeah, I see there are some changes going on, this way at least it keeps me in the loop :D, and the XContentTester is useful.

hub-cap · 2018-10-15T19:46:25Z

Yes there is much in flight now. im having a look at your PR now :)

hub-cap

This is looking good. Lets clean up the little nits I mentioned and see if the magic incantations of the tests work.

hub-cap · 2018-10-15T19:50:17Z

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

@@ -485,9 +485,9 @@ public void testRethrottle() {
        List<Tuple<String, Supplier<Request>>> variants = new ArrayList<>();
        variants.add(new Tuple<String, Supplier<Request>>("_reindex", () -> RequestConverters.rethrottleReindex(rethrottleRequest)));
        variants.add(new Tuple<String, Supplier<Request>>("_update_by_query",
-            () -> RequestConverters.rethrottleUpdateByQuery(rethrottleRequest)));
+                () -> RequestConverters.rethrottleUpdateByQuery(rethrottleRequest)));


There are a lot of these space altered in this RequestConvertersTests.java class. Can you revert them as we dont need to alter them. Curse you, but I love you, IDE!

think it should be ok, please check https://github.com/elastic/elasticsearch/compare/master...mrdjen:hl_client_count_api-review#diff-dd0dd6a857a14996c88dd7820d6f8c7c

hub-cap · 2018-10-15T20:12:34Z

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

@@ -1233,4 +1235,71 @@ private static void assertSearchHeader(SearchResponse searchResponse) {
        assertEquals(0, searchResponse.getShardFailures().length);
        assertEquals(SearchResponse.Clusters.EMPTY, searchResponse.getClusters());
    }
+
+    //Count API IT tests start


you dont need these, its a big ole file but we dont do it with other sections

ok, removed

hub-cap · 2018-10-15T20:25:06Z

add to whitelist

hub-cap · 2018-10-15T20:32:41Z

Once the nits are addressed please also merge in the latest from master one more time like you've done before. It will be the best way to ensure the tests pass :)

Ill take care of backporting it once its merged. Thanks again for all the work you put in to this!

- remove comments in SearchIT

mrdjen · 2018-10-17T19:13:59Z

Hi @hub-cap I've addressed the nits you've requested,
Thanks

…count_api-review

Add `count()` api method, `CountRequest` and `CountResponse` classes to HLRC. Code in server module is unchanged. Relates to #27205

hub-cap · 2018-11-02T13:38:58Z

Code is merged and backported to 6.x. thanks much for the contribution! Sorry it took so long!!! :D

mrdjen · 2018-11-02T14:10:07Z

Hey great @hub-cap!
Np, I'm glad to contribute back!
Thank you as well and hope to contribute sometime again in the future :)

Add document _count API support to Rest High Level Client.

df32992

Add `count()` api method, `CountRequest` and `CountResponse` classes to HLRC. Code in server module is unchanged. Relates to elastic#27205

mrdjen mentioned this pull request Oct 3, 2018

Add CountRequest/Response classes to server and use them in RestCountAction (cat and doc). #32711

Closed

colings86 added the :Core/Java High Level REST Client label Oct 3, 2018

hub-cap added >enhancement v7.0.0 v6.5.0 labels Oct 3, 2018

hub-cap suggested changes Oct 3, 2018

View reviewed changes

mrdjen added 2 commits October 9, 2018 10:13

Merge branch 'master' into hl_client_count_api-review

096bda3

hub-cap reviewed Oct 15, 2018

View reviewed changes

mrdjen added 2 commits October 17, 2018 20:31

Merge branch 'master' into hl_client_count_api-review

0b8bc28

Address changes requested during review 34267

1935caf

- remove comments in SearchIT

hub-cap self-assigned this Oct 22, 2018

colings86 added v6.6.0 and removed v6.5.0 labels Oct 25, 2018

Merge remote-tracking branch 'upstream/master' into mrdjen/hl_client_…

c13f9e0

…count_api-review

hub-cap approved these changes Oct 30, 2018

View reviewed changes

hub-cap merged commit 34677b9 into elastic:master Nov 2, 2018

hub-cap added the backport pending label Nov 2, 2018

hub-cap pushed a commit that referenced this pull request Nov 2, 2018

HLRC: Add document _count API (#34267)

2efb892

Add `count()` api method, `CountRequest` and `CountResponse` classes to HLRC. Code in server module is unchanged. Relates to #27205

hub-cap removed the backport pending label Nov 2, 2018

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 document _count API support to Rest High Level Client. #34267

Add document _count API support to Rest High Level Client. #34267

mrdjen commented Oct 3, 2018

elasticmachine commented Oct 3, 2018

hub-cap commented Oct 3, 2018

hub-cap left a comment

hub-cap Oct 3, 2018

mrdjen Oct 9, 2018

hub-cap Oct 3, 2018

mrdjen Oct 9, 2018

hub-cap Oct 3, 2018

mrdjen Oct 9, 2018

hub-cap Oct 3, 2018

mrdjen Oct 9, 2018

hub-cap Oct 3, 2018

mrdjen Oct 9, 2018

hub-cap Oct 3, 2018

mrdjen Oct 9, 2018

hub-cap Oct 3, 2018

hub-cap Oct 3, 2018

mrdjen Oct 9, 2018

mrdjen commented Oct 4, 2018

mrdjen commented Oct 4, 2018

mrdjen commented Oct 9, 2018 •

edited

Loading

hub-cap commented Oct 15, 2018

hub-cap left a comment

hub-cap Oct 15, 2018

mrdjen Oct 17, 2018 •

edited

Loading

hub-cap Oct 15, 2018

mrdjen Oct 17, 2018

hub-cap commented Oct 15, 2018

hub-cap commented Oct 15, 2018

mrdjen commented Oct 17, 2018

hub-cap commented Nov 2, 2018

mrdjen commented Nov 2, 2018

Add document _count API support to Rest High Level Client. #34267

Add document _count API support to Rest High Level Client. #34267

Conversation

mrdjen commented Oct 3, 2018

elasticmachine commented Oct 3, 2018

hub-cap commented Oct 3, 2018

hub-cap 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

Choose a reason for hiding this comment

mrdjen commented Oct 4, 2018

mrdjen commented Oct 4, 2018

mrdjen commented Oct 9, 2018 • edited Loading

hub-cap commented Oct 15, 2018

hub-cap left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mrdjen Oct 17, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hub-cap commented Oct 15, 2018

hub-cap commented Oct 15, 2018

mrdjen commented Oct 17, 2018

hub-cap commented Nov 2, 2018

mrdjen commented Nov 2, 2018

mrdjen commented Oct 9, 2018 •

edited

Loading

mrdjen Oct 17, 2018 •

edited

Loading