Support both typed and typeless 'get mapping' requests in the HLRC. #37796

jtibshirani · 2019-01-24T03:25:59Z

From previous PRs, we've already added support for include_type_name to
the get mapping API. We had also taken an approach to the HLRC where the
server-side GetMappingResponse#fromXContent could only handle typeless
input.

This PR updates the HLRC for 'get mapping' to be in line with our new approach:

Add a typeless 'get mappings' method to the Java HLRC, that accepts new
client-side request and response objects. This new response only handles
typeless mapping definitions.
Switch the old version of GetMappingResponse back to expecting typed
mappings, and deprecate the corresponding method on the HLRC.

Finally, the PR also does some small, related clean-up around 'get field mappings'.

elasticmachine · 2019-01-24T06:10:32Z

Pinging @elastic/es-search

cbuescher

@jtibshirani great PR, I left a few comments, nothing major except for the more general question about how we couple server/client side toXContent/fromXContent. That question shouldn't hold this PR though I think.

cbuescher · 2019-01-24T13:48:24Z

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

-     * @deprecated This method uses an old request object which still refers to types, a deprecated feature. The
-     * method {@link #getFieldMappingAsync(GetFieldMappingsRequest, RequestOptions, ActionListener)} should be used instead,
-     * which accepts a new request object.
+     * @deprecated This method uses an request and response objects which still refers to types, a deprecated feature.


nit: s/an/a

Oops, thanks!

cbuescher · 2019-01-24T13:49:23Z

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

+        return request;
+    }
+
+    static Request getMappings(org.elasticsearch.action.admin.indices.mapping.get.GetMappingsRequest getMappingsRequest) {


maybe add @deprecated even if we only use this internally. It makes it more obvious and serves as a grep-able reminder that we can remove this in 8.0.

cbuescher · 2019-01-24T13:50:40Z

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

+import org.elasticsearch.client.Validatable;
+import org.elasticsearch.common.Strings;
+
+public class GetMappingsRequest extends TimedRequest implements Validatable {


since TimedRequest implement Validatable, we don't need it here again.

cbuescher · 2019-01-24T13:54:16Z

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

+    }
+
+    @Override
+    public boolean equals(Object o) {


Although implementing equals/hashcode never hurts I think, it puts some additional burden on us in testing and maintaining it. I think I opted for starting with not implementing it as long as we don't need it, but I'd be interested in your opinion on this.

I added these methods to support the serialization/ deserialization tests in GetMappingsResponseTests. But I agree in this instance I should probably remove this and just define assertEqualInstances in the test -- because MappingMetaData stores the mappings as xContent, general-purpose equals/ hashCode methods are not so useful and potentially misleading.

cbuescher · 2019-01-24T13:58:04Z

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

+            new org.elasticsearch.action.admin.indices.mapping.get.GetMappingsRequest();
+
+        String[] indices = Strings.EMPTY_ARRAY;
+        if (ESTestCase.randomBoolean()) {


nit: randomBoolean alone should work, no?

👍 this was copy-pasted from the existing test, will clean this up a bit.

cbuescher · 2019-01-24T13:58:12Z

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

+        if (ESTestCase.randomBoolean()) {
+            indices = RequestConvertersTests.randomIndicesNames(0, 5);
+            getMappingRequest.indices(indices);
+        } else if (ESTestCase.randomBoolean()) {


cbuescher · 2019-01-24T14:05:27Z

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

@@ -602,8 +602,7 @@ public void testGetMapping() throws IOException {
            // end::get-mappings-request

            // tag::get-mappings-request-masterTimeout
-            request.masterNodeTimeout(TimeValue.timeValueMinutes(1)); // <1>
-            request.masterNodeTimeout("1m"); // <2>
+            request.setMasterTimeout(TimeValue.timeValueMinutes(1)); // <1>


Out of curiosity, did you remove the second setter on purpose? Might be a useful one in this case, but I'm also fine with keeping the new request lean from the start. Just wanted to know.

I went with the strategy we seem to be using for other HLRC requests, where we add these setters by extending TimedRequest. That parent class only includes one setter, and doesn't accept the string-based time representation.

cbuescher · 2019-01-24T14:05:45Z

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

-            ImmutableOpenMap<String, ImmutableOpenMap<String, MappingMetaData>> allMappings = getMappingResponse.mappings(); // <1>
-            MappingMetaData typeMapping = allMappings.get("twitter").get("_doc"); // <2>
-            Map<String, Object> mapping = typeMapping.sourceAsMap(); // <3>
+            Map<String, MappingMetaData> allMappings = getMappingResponse.mappings(); // <1>


Good point!

cbuescher · 2019-01-24T14:14:06Z

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

+        Map<String, MappingMetaData> allMappings = new HashMap<>();
+        allMappings.put("index-" + randomAlphaOfLength(5), randomMappingMetaData());
+        return new GetMappingsResponse(allMappings);
+    }


I left some of my thinking already in your review of #37778. Basically, what we really want to test is that we can parse all possible xContent outputs of the server-side response class. It would be great to tie them close together. Copying the randomization methods and converting the client-side instance to a server-side one in order to use its "toXContent" works but has the potential to deviate over time. We should probably discuss this in a larger context than this PR though if we keep continuing with this pattern on server/client side response parsing.

👍 I responded there. For now I'm planning on updating this test to generate a random server response, as opposed to a random client one.

…ed to a client one.

jtibshirani · 2019-01-24T23:31:53Z

@elasticmachine run elasticsearch-ci/1

jtibshirani · 2019-01-25T01:26:57Z

@elasticmachine run elasticsearch-ci/1

jtibshirani · 2019-01-25T06:09:26Z

@elasticmachine run elasticsearch-ci/1

hub-cap · 2019-01-25T15:52:46Z

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

+            this::createParser,
+            GetMappingsResponseTests::createTestInstance,
+            GetMappingsResponseTests::toXContent,
+            GetMappingsResponseTests::fromXContent)


just curious as to why this is not calling the GetMappingResponse::fromXContent?

ahh because of the type difference.

jtibshirani · 2019-01-25T16:10:35Z

Thanks @cbuescher for the review, this should be ready for another look!

cbuescher

@jtibshirani LGTM although I have to admit the whole dance with the server-side/client-side roundtrip testing is still a bit confusing. But at least I agree we are testing we can parse the output of the server-side response, which is the most important thing. We can stream-line this approach across a few of these tests when we get a few of them in.

jtibshirani · 2019-01-28T00:01:34Z

Thanks @cbuescher! I had a more in-depth discussion with @hub-cap about the best approach to testing these classes. I'll add a comment to our ongoing thread on your PR #37778 with the conclusions of that discussion, to try to clear up our confusion.

…lastic#37796) From previous PRs, we've already added support for include_type_name to the get mapping API. This PR adds a typeless 'get mappings' method to the Java HLRC, that accepts new client-side request and response objects. This new response only handles typeless mapping definitions. Finally, the PR also performs some small, related clean-up around 'get field mappings'.

jtibshirani added :Search Foundations/Mapping Index mappings, including merging and defining field types v7.0.0 >refactoring v6.7.0 labels Jan 24, 2019

jtibshirani requested a review from cbuescher January 24, 2019 03:26

jtibshirani force-pushed the get-mapping-hlrc branch from 285d37e to 06d7f8a Compare January 24, 2019 03:27

Support both typed and typeless 'get mapping' requests in the HLRC.

acf286f

jtibshirani force-pushed the get-mapping-hlrc branch from 06d7f8a to acf286f Compare January 24, 2019 03:45

jtibshirani requested a review from hub-cap January 24, 2019 03:50

jtibshirani added :Search Foundations/Mapping Index mappings, including merging and defining field types and removed :Search Foundations/Mapping Index mappings, including merging and defining field types labels Jan 24, 2019

cbuescher requested changes Jan 24, 2019

View reviewed changes

jtibshirani mentioned this pull request Jan 24, 2019

Implementation tracking for 7.0 types deprecation. #35190

Closed

48 tasks

jtibshirani added 8 commits January 24, 2019 11:25

Fix a typo in IndicesClient.

4e05f79

Add some missing deprecated annotations in IndicesRequestConverters.

28f425d

Minor clean up in GetMappingsRequest.

05c870d

Remove equals and hashCode from GetMappingsResponse.

06805cc

Some clean up in IndicesRequestConvertersTests.

3b02709

In GetMappingsResponseTests, generate a random server object as oppos…

bd476a0

…ed to a client one.

Fix more typos in IndicesClient.

811c473

Merge remote-tracking branch 'upstream/master' into get-mapping-hlrc

b2a19e4

hub-cap reviewed Jan 25, 2019

View reviewed changes

cbuescher approved these changes Jan 25, 2019

View reviewed changes

jtibshirani added 2 commits January 27, 2019 12:56

In GetMappingsResponseTests, switch to comparing client-side objects.

705880d

Merge remote-tracking branch 'upstream/master' into get-mapping-hlrc

e122a1c

jtibshirani merged commit b1735aa into elastic:master Jan 28, 2019

jtibshirani deleted the get-mapping-hlrc branch January 28, 2019 00:02

droberts195 mentioned this pull request Jan 28, 2019

FullClusterRestartIT tests and QueryBuilderBWCIT#testQueryBuilderBWC fail #37920

Closed

jtibshirani mentioned this pull request Jan 29, 2019

Support both typed and typeless 'get mapping' requests in the HLRC (backport to 6.x). #37950

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

Support both typed and typeless 'get mapping' requests in the HLRC. #37796

Support both typed and typeless 'get mapping' requests in the HLRC. #37796

jtibshirani commented Jan 24, 2019 •

edited

Loading

elasticmachine commented Jan 24, 2019

cbuescher left a comment

cbuescher Jan 24, 2019

jtibshirani Jan 24, 2019

cbuescher Jan 24, 2019

jtibshirani Jan 24, 2019

cbuescher Jan 24, 2019

jtibshirani Jan 24, 2019

cbuescher Jan 24, 2019

jtibshirani Jan 24, 2019

cbuescher Jan 24, 2019

jtibshirani Jan 24, 2019

cbuescher Jan 24, 2019

cbuescher Jan 24, 2019

jtibshirani Jan 24, 2019

cbuescher Jan 24, 2019

cbuescher Jan 24, 2019

jtibshirani Jan 24, 2019

jtibshirani commented Jan 24, 2019

jtibshirani commented Jan 25, 2019

jtibshirani commented Jan 25, 2019

hub-cap Jan 25, 2019

hub-cap Jan 25, 2019

jtibshirani commented Jan 25, 2019

cbuescher left a comment

jtibshirani commented Jan 28, 2019

Support both typed and typeless 'get mapping' requests in the HLRC. #37796

Support both typed and typeless 'get mapping' requests in the HLRC. #37796

Conversation

jtibshirani commented Jan 24, 2019 • edited Loading

elasticmachine commented Jan 24, 2019

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jtibshirani commented Jan 24, 2019

jtibshirani commented Jan 25, 2019

jtibshirani commented Jan 25, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jtibshirani commented Jan 25, 2019

cbuescher left a comment

Choose a reason for hiding this comment

jtibshirani commented Jan 28, 2019

jtibshirani commented Jan 24, 2019 •

edited

Loading