Remove type field from DocWriteRequest and associated Response objects by romseygeek · Pull Request #47671 · elastic/elasticsearch

romseygeek · 2019-10-07T14:14:16Z

This commit removes the type field from index, update and delete requests, and their
associated responses.

Relates to #41059

…request-action

…ction' into types-removal/bulk-request-action

…rite-request

romseygeek · 2019-10-09T09:04:52Z

and now in 8.x we can simply remove the deprecated typed constructors and setters like IndexRequest#type.

You're right, I got confused by things not being deprecated on DocWriteRequest, but everything is indeed marked as deprecated in the concrete child classes, so no refactoring necessary.

jtibshirani

What a big effort! This looks good to me, I just left a couple questions/ comments.

jtibshirani · 2019-10-09T17:13:10Z

docs/reference/mapping/removal_of_types.asciidoc

 {
  "_index": "index",
  "_id": "1",
-  "_type": "_doc",


Should we leave this reference untouched, since it is supposed to describe the 7.x behavior? To avoid test failures, we could mark these snippets as not tested (as we do for some of the snippets that only make sense in 6.x).

Possibly, although I imagine we will remove this page entirely, and instead have something in the upgrade notes - @colings86 @jpountz do you have an opinion on what to do here?

jtibshirani · 2019-10-09T17:33:13Z

server/src/main/java/org/elasticsearch/action/delete/DeleteRequestBuilder.java

     * Sets the type of the document to delete.
+     * @deprecated types are being removed
     */
+    @Deprecated


It sounds like these methods will be removed in a follow-up PR?

jtibshirani · 2019-10-09T18:06:59Z

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

            policies = entityAsMap(response);
        } catch (ResponseException e) {
-            if (RestStatus.METHOD_NOT_ALLOWED.getStatus() == e.getResponse().getStatusLine().getStatusCode()) {
+            if (RestStatus.METHOD_NOT_ALLOWED.getStatus() == e.getResponse().getStatusLine().getStatusCode() ||


Looks like some leftover code to work around test failures?

So this was an interesting one; all the REST tests started failing with 400 errors, and it took me a while to track down what was happening. Previously, if SLM or ILM were not enabled, a call to GET _ilm/policy would be interpreted as an index matching {index}/{type}, which only has handlers registered for PUT and POST, and so would return a 406 Method not allowed error. Now, the {index}/{type} handler has gone away, and so we just get a 400 Bad Request instead. I don't think we need to check for 406 errors any more, so I'll try removing that line.

Looks like we still need to check for both due to BWC checks

Got it, this change makes sense to me.

jtibshirani · 2019-10-09T18:07:55Z

x-pack/plugin/ccr/src/test/java/org/elasticsearch/xpack/ccr/action/ShardChangesActionTests.java

        final Translog.Operation[] operations = ShardChangesAction.getOperations(indexShard, indexShard.getLastKnownGlobalCheckpoint(),
            0, 12, indexShard.getHistoryUUID(), new ByteSizeValue(256, ByteSizeUnit.BYTES));
-        assertThat(operations.length, equalTo(12));
+        assertThat(operations.length, equalTo(11));


For my context, why did this number change?

The test is checking what happens when documents stored in the translog take up more than a certain amount of space, in this case 256 bytes. Previously, the documents had a type of doc, but now they have a type of _doc (as a default - a future PR will remove type entirely from the doc stored in the translog), which is one byte longer, so we fit one fewer document in the same number of bytes.

Thanks for the explanation.

…rite-request

We still need to check for 405 errors during mixed-cluster BWC tests This reverts commit ed1f3fd.

jtibshirani

This looks good to me, apart from the question around removal_of_types.asciidoc.

romseygeek · 2019-10-11T09:24:29Z

Thanks @jtibshirani - I added an item to the meta issue to discuss what to do about the removal_of_types documentation.

elastic#47671) This commit removes the type field from index, update and delete requests, and their associated responses. Relates to elastic#41059

The types removal effort has removed the type from Index API in #47671 and from Get API in #46587 This commit allows to use 'typed' endpoints for the both Index and Get APIs relates compatible types-removal meta issue #54160

…tDeleteAction the previously removed typed enpotins for Update and Delete are retrofitted in this commit the commit that removed them elastic#47671 relates main meta issue elastic#51816 relates types removal issue elastic#54160

…tDeleteAction (#73115) the previously removed typed enpotins for Update and Delete are retrofitted in this commit the commit that removed them #47671 relates main meta issue #51816 relates types removal issue #54160

romseygeek added 26 commits September 23, 2019 19:39

Remove types from bulk request

94d7e14

Merge remote-tracking branch 'origin/master' into types-removal/bulk-…

b6f94f7

…request-action

tests

1f538a3

tests

748d1f6

tests

9ba9c59

tests

e63d109

compilation; remove type from BulkResponse.Failure

f03a740

tests

e8d9158

bulk monitoring

daee751

imports

842ecfd

Merge remote-tracking branch 'origin/master' into types-removal/bulk-…

d47c63a

…request-action

bulk tests

45c6f5e

tests

100019d

monitoring again

fdabe48

sake

9779d9b

yaml test

4345c6a

assertions

946c313

yaml test

5c2950f

wtf

ca03935

Merge branch 'master' into types-removal/bulk-request-action

7863b64

relax assertion

9baf0f5

Merge remote-tracking branch 'romseygeek/types-removal/bulk-request-a…

3b13ca2

…ction' into types-removal/bulk-request-action

Remove types from DocWriteRequest/IndexResponse

db1e223

Merge remote-tracking branch 'origin/master' into types-removal/doc-w…

219504f

…rite-request

tests

d5d878f

test failures

d280f75

romseygeek added :Distributed/CRUD A catch all label for issues around indexing, updating and getting a doc by id. Not search. >breaking-java >refactoring v8.0.0 labels Oct 7, 2019

romseygeek added 8 commits October 8, 2019 12:32

tests

64abbcc

tests

4a61b4b

Merge remote-tracking branch 'origin/master' into types-removal/doc-w…

55fb4c4

…rite-request

bats

d34189f

tests

75d3c8e

tests

9e2d404

tests

3777f10

more tests

e92fd44

watcher tests

a99a0de

jpountz mentioned this pull request Oct 9, 2019

Types removal in 8.0 #41059

Closed

66 tasks

jtibshirani reviewed Oct 9, 2019

View reviewed changes

romseygeek added 3 commits October 10, 2019 09:19

Merge remote-tracking branch 'origin/master' into types-removal/doc-w…

ef2aa73

…rite-request

Only need to check for 400 now

ed1f3fd

Revert "Only need to check for 400 now"

ffdfa0e

We still need to check for 405 errors during mixed-cluster BWC tests This reverts commit ed1f3fd.

jtibshirani approved these changes Oct 10, 2019

View reviewed changes

romseygeek merged commit 566e1b7 into elastic:master Oct 11, 2019

romseygeek deleted the types-removal/doc-write-request branch October 11, 2019 09:23

pgomulka mentioned this pull request Mar 25, 2020

Types removal - compatible changes #54160

Closed

66 tasks

pgomulka mentioned this pull request Feb 25, 2021

[REST Compatible API] Typed endpoints for Index and Get APIs #69131

Merged

pgomulka mentioned this pull request May 14, 2021

[Rest Api Compatibility] Typed endpoints for RestUpdateAction and RestDeleteAction #73115

Merged

jakelandis added v8.0.0-alpha1 and removed v8.0.0 labels Jul 26, 2021

Conversation

romseygeek commented Oct 7, 2019

Uh oh!

romseygeek commented Oct 9, 2019

Uh oh!

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

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 left a comment

Choose a reason for hiding this comment

Uh oh!

romseygeek commented Oct 11, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants