Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove type field from DocWriteRequest and associated Response objects #47671

Merged
merged 43 commits into from
Oct 11, 2019

Conversation

romseygeek
Copy link
Contributor

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

Relates to #41059

@romseygeek romseygeek added :Distributed Indexing/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
Copy link
Contributor Author

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.

@jpountz jpountz mentioned this pull request Oct 9, 2019
66 tasks
Copy link
Contributor

@jtibshirani jtibshirani left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@@ -532,7 +532,6 @@ PUT index/_doc/1
{
"_index": "index",
"_id": "1",
"_type": "_doc",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

*/
@Deprecated
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

++

@@ -684,7 +684,8 @@ private static void deleteAllILMPolicies() throws IOException {
Response response = adminClient().performRequest(new Request("GET", "/_ilm/policy"));
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() ||
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like some leftover code to work around test failures?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Got it, this change makes sense to me.

@@ -147,7 +147,7 @@ public void testGetOperationsExceedByteLimit() throws Exception {
final IndexShard indexShard = indexService.getShard(0);
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));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For my context, why did this number change?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the explanation.

We still need to check for 405 errors during mixed-cluster BWC tests

This reverts commit ed1f3fd.
Copy link
Contributor

@jtibshirani jtibshirani left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@romseygeek romseygeek merged commit 566e1b7 into elastic:master Oct 11, 2019
@romseygeek romseygeek deleted the types-removal/doc-write-request branch October 11, 2019 09:23
@romseygeek
Copy link
Contributor Author

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

howardhuanghua pushed a commit to TencentCloudES/elasticsearch that referenced this pull request Oct 14, 2019
elastic#47671)

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

Relates to elastic#41059
@pgomulka pgomulka mentioned this pull request Mar 25, 2020
66 tasks
pgomulka added a commit that referenced this pull request Mar 23, 2021
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
pgomulka added a commit to pgomulka/elasticsearch that referenced this pull request May 14, 2021
…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
pgomulka added a commit that referenced this pull request May 18, 2021
…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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
>breaking-java :Distributed Indexing/CRUD A catch all label for issues around indexing, updating and getting a doc by id. Not search. >refactoring v8.0.0-alpha1
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants