Support both typed and typeless 'get mapping' requests in the HLRC.#37796
Support both typed and typeless 'get mapping' requests in the HLRC.#37796jtibshirani merged 11 commits intoelastic:masterfrom
Conversation
285d37e to
06d7f8a
Compare
06d7f8a to
acf286f
Compare
|
Pinging @elastic/es-search |
cbuescher
left a comment
There was a problem hiding this comment.
@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.
| * @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. |
| return request; | ||
| } | ||
|
|
||
| static Request getMappings(org.elasticsearch.action.admin.indices.mapping.get.GetMappingsRequest getMappingsRequest) { |
There was a problem hiding this comment.
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.
| import org.elasticsearch.client.Validatable; | ||
| import org.elasticsearch.common.Strings; | ||
|
|
||
| public class GetMappingsRequest extends TimedRequest implements Validatable { |
There was a problem hiding this comment.
since TimedRequest implement Validatable, we don't need it here again.
| } | ||
|
|
||
| @Override | ||
| public boolean equals(Object o) { |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
| new org.elasticsearch.action.admin.indices.mapping.get.GetMappingsRequest(); | ||
|
|
||
| String[] indices = Strings.EMPTY_ARRAY; | ||
| if (ESTestCase.randomBoolean()) { |
There was a problem hiding this comment.
nit: randomBoolean alone should work, no?
There was a problem hiding this comment.
👍 this was copy-pasted from the existing test, will clean this up a bit.
| if (ESTestCase.randomBoolean()) { | ||
| indices = RequestConvertersTests.randomIndicesNames(0, 5); | ||
| getMappingRequest.indices(indices); | ||
| } else if (ESTestCase.randomBoolean()) { |
| // tag::get-mappings-request-masterTimeout | ||
| request.masterNodeTimeout(TimeValue.timeValueMinutes(1)); // <1> | ||
| request.masterNodeTimeout("1m"); // <2> | ||
| request.setMasterTimeout(TimeValue.timeValueMinutes(1)); // <1> |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
| 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> |
| Map<String, MappingMetaData> allMappings = new HashMap<>(); | ||
| allMappings.put("index-" + randomAlphaOfLength(5), randomMappingMetaData()); | ||
| return new GetMappingsResponse(allMappings); | ||
| } |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
👍 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.
|
@elasticmachine run elasticsearch-ci/1 |
2 similar comments
|
@elasticmachine run elasticsearch-ci/1 |
|
@elasticmachine run elasticsearch-ci/1 |
| this::createParser, | ||
| GetMappingsResponseTests::createTestInstance, | ||
| GetMappingsResponseTests::toXContent, | ||
| GetMappingsResponseTests::fromXContent) |
There was a problem hiding this comment.
just curious as to why this is not calling the GetMappingResponse::fromXContent?
There was a problem hiding this comment.
ahh because of the type difference.
|
Thanks @cbuescher for the review, this should be ready for another look! |
cbuescher
left a comment
There was a problem hiding this comment.
@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.
|
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'.
From previous PRs, we've already added support for
include_type_nametothe get mapping API. We had also taken an approach to the HLRC where the
server-side
GetMappingResponse#fromXContentcould only handle typelessinput.
This PR updates the HLRC for 'get mapping' to be in line with our new approach:
client-side request and response objects. This new response only handles
typeless mapping definitions.
GetMappingResponseback to expecting typedmappings, and deprecate the corresponding method on the HLRC.
Finally, the PR also does some small, related clean-up around 'get field mappings'.