Deprecate types in explain requests. by jtibshirani · Pull Request #35611 · elastic/elasticsearch

jtibshirani · 2018-11-16T00:03:56Z

This PR introduces the new untyped endpoint {index}/{id}/_explain, and deprecates {index}/{type}/{id}/_explain.

Note that we currently have the following issue: if a document is created with type some_type, and we make an untyped explain request for that document, then it will not be found. This is because an untyped explain request uses the dummy type _doc, which does not match the existing type name some_type.

I didn't address the issue in this PR, since I suspect we want to step back and approach it more generally. Tagging @jpountz here, since he was planning to look into how to address this 'types mismatch'.

elasticmachine · 2018-11-16T00:03:58Z

Pinging @elastic/es-search-aggs

jpountz · 2018-11-16T17:50:57Z

{index}/{id}/_explain

I'm wondering whether that would be {index}/_explain/{id} instead, similarly to {index}/_doc/{id}.

I didn't address the issue in this PR, since I suspect we want to step back and approach it more generally

+1

pcsanwald · 2018-11-19T21:34:31Z

@jpountz in terms of swapping _explain/{id} for {id}/_explain, I feel we should address API consistency issues separately, unless we intend to address them all as part of this initiative. In terms of explaining to users what types removal means for them, it's much clearer if we are able to explain that in order types removal means they should remove the type name from the path where it is used. This feels like a much clearer explanation to me, and if I was auditing my app in preparation for a 7.0 upgrade, it would be way easier for me to assess the impact, vs this change and in addition, other path parameter changes.

jtibshirani · 2018-11-20T02:03:06Z

Thanks @jpountz and @pcsanwald, I opened an internal issue to try to get more clarity on this and other related points -- maybe we could move the discussion there for now, then I will report back here with our conclusions?

jtibshirani · 2018-12-10T21:35:58Z

After discussion, we've decided to go with the endpoint format {index}/_explain/{id}. I've updated the PR to use this format.

Removing the type component (as in {index}/{id}/_explain) didn’t seem viable, because

Unlike types, IDs are allowed to contain underscores, and there could be collisions with other endpoints like {index}/_doc/{id}.
It doesn’t work cleanly for all URLs, for example 'termvectors' where it introduces collisions between the different endpoints.

jtibshirani · 2018-12-10T22:23:23Z

@elasticmachine test this please

jpountz

I left some comments but it looks good in general. Thanks for tackling this!

jpountz · 2018-12-10T22:34:33Z

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

-            endpoint(explainRequest.index(), explainRequest.type(), explainRequest.id(), "_explain"));
+        String endpoint = explainRequest.isTypeless()
+            ? endpoint(explainRequest.index(), "_explain", explainRequest.id())
+            : endpoint(explainRequest.index(), explainRequest.id(), "_explain");


I would expect to have explainRequest.type() somewhere on this line?

Oops, I think I have been staring at this too long...

jpountz · 2018-12-10T22:36:32Z

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

    public void testExplain() throws IOException {
        String index = randomAlphaOfLengthBetween(3, 10);
-        String type = randomAlphaOfLengthBetween(3, 10);
+        String type = randomBoolean() ? null : randomAlphaOfLengthBetween(3, 10);


let's test both cases all the time?

Will do, that would've helped catch the issue above.

jpountz · 2018-12-10T22:37:44Z

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

 ==== Explain Request

-An `ExplainRequest` expects an `index`, a `type` and an `id` to specify a certain document,
+An `ExplainRequest` expects an `index`, and an `id` to specify a certain document,


remove comma after id?

jpountz · 2018-12-10T22:39:10Z

server/src/main/java/org/elasticsearch/action/explain/ExplainRequest.java

    }

+    public boolean isTypeless() {
+        return type.equals(MapperService.SINGLE_MAPPING_NAME);


should we protect against NPE?

jpountz · 2018-12-10T22:41:22Z

test/framework/src/main/java/org/elasticsearch/test/rest/TypesRemovalWarningsHandler.java

+    @Override
+    public boolean warningsShouldFailRequest(List<String> warnings) {
+        for (String warning : warnings) {
+            if (!warning.startsWith("[types removal]")) {


the more common idiom across the code base is to do == false

jtibshirani · 2018-12-10T23:07:30Z

Thanks @jpountz for the review! I've tried to address your comments.

…n-explain

markharwood · 2018-12-11T14:25:04Z

qa/full-cluster-restart/src/test/java/org/elasticsearch/upgrades/FullClusterRestartIT.java

+        explanationOptions.setWarningsHandler(TypesRemovalWarningsHandler.INSTANCE);
+        explanationRequest.setOptions(explanationOptions);
+
        String explanation = toStr(client().performRequest(explanationRequest));


This style of warnings handler helps suppress test failures but does not help assert that any of the expected warnings are actually being returned.
The #36443 PR aims to provide a WarningsHandler that can require the expected warnings materialise (given a same-version cluster) or are allowed in a multi-version cluster

Thanks @markharwood, that looks like a nice addition. In the next CRUD deprecation PR, maybe we can try migrating to that approach.

An update: In #36511 I tried switching to the VersionSensitiveWarningsHandler introduced in that PR. I'd be curious to get your feedback on how it's being used.

jtibshirani added :Search Foundations/Mapping Index mappings, including merging and defining field types >deprecation v7.0.0 labels Nov 16, 2018

jtibshirani mentioned this pull request Nov 16, 2018

Implementation tracking for 7.0 types deprecation. #35190

Closed

48 tasks

jtibshirani force-pushed the types-in-explain branch 4 times, most recently from 3a9af44 to 1083631 Compare November 16, 2018 17:37

jtibshirani force-pushed the types-in-explain branch from 1083631 to c7740d8 Compare December 10, 2018 21:17

Deprecate types in explain requests.

7a6f3db

jtibshirani force-pushed the types-in-explain branch from c7740d8 to 7a6f3db Compare December 10, 2018 21:43

jtibshirani requested review from markharwood and mayya-sharipova December 10, 2018 22:14

jpountz suggested changes Dec 10, 2018

View reviewed changes

Address review comments.

1153b71

jpountz approved these changes Dec 10, 2018

View reviewed changes

jtibshirani added 2 commits December 10, 2018 15:42

Merge remote-tracking branch 'upstream/master' into deprecate-types-i…

854d169

…n-explain

Make sure to skip typeless tests for versions before 7.0.

1bc912b

jtibshirani merged commit 8783105 into elastic:master Dec 11, 2018

jtibshirani deleted the types-in-explain branch December 11, 2018 03:45

martijnvg mentioned this pull request Dec 11, 2018

[CI] MixedClusterClientYamlTestSuiteIT explain/11_basic_with_types/Explain body without query element failure #36469

Closed

markharwood reviewed Dec 11, 2018

View reviewed changes

colings86 added the v7.0.0-beta1 label Feb 7, 2019

colings86 removed the v7.0.0 label Feb 7, 2019

Conversation

jtibshirani commented Nov 16, 2018

Uh oh!

elasticmachine commented Nov 16, 2018

Uh oh!

jpountz commented Nov 16, 2018

Uh oh!

pcsanwald commented Nov 19, 2018

Uh oh!

jtibshirani commented Nov 20, 2018

Uh oh!

jtibshirani commented Dec 10, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jtibshirani commented Dec 10, 2018

Uh oh!

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

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!

jtibshirani commented Dec 10, 2018

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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

jtibshirani commented Dec 10, 2018 •

edited

Loading