Types removal - deprecate types in _bulk apis #36549

markharwood · 2018-12-12T14:53:56Z

Deprecate transport client APIs with document types.
Deprecate REST endpoints that have types in URLs or bodies.

Some remaining work (e.g. CRUDDocumentationIT.testBulk code examples) depends on @mayya-sharipova 's changes to IndexRequest for the singular index apis.

elasticmachine · 2018-12-12T14:53:58Z

Pinging @elastic/es-search

markharwood · 2018-12-18T09:56:58Z

run gradle build tests

markharwood · 2018-12-18T11:22:25Z

run gradle build tests

markharwood · 2018-12-20T14:58:06Z

run gradle build tests

markharwood · 2018-12-20T15:16:32Z

run gradle build tests

markharwood · 2019-01-03T14:06:34Z

The last remaining test failure is a rolling upgrade test (IndexingIT) which looks to be failing when a 6.x node is given a types removal warning by a 7.x node.
I've opened #37105 in the 6.x branch to address this which when comitted should hopefully give us a green build here.

jtibshirani

What a huge effort!! I left a few preliminary comments as I got up to speed with the approach.

jtibshirani · 2019-01-04T06:26:05Z

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

+            }
+        }
+        {
+            //Check that untyped document additions and untyped global inherit the established custom type


Would you be able to explain a bit what 'inherit the custom type' means here? I'm a little unclear on why the custom type would be relevant, since we're setting both the global and local types to null.

It means the custom document type introduced to the mapping by the earlier code in this test. I'll add a comment.

jtibshirani · 2019-01-04T06:30:38Z

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

@@ -55,7 +55,7 @@ public void testReindex() throws IOException {
                RestStatus.OK,
                highLevelClient().bulk(
                    bulkRequest,
-                    RequestOptions.DEFAULT
+                    expectWarnings(BulkRequest.TYPES_DEPRECATION_MESSAGE)


I think that since the reindex PR (#36823) was merged, this is no longer necessary.

Yep, in fact this false expectation causes an error. Will fix

jtibshirani · 2019-01-04T06:33:18Z

...est-high-level/src/test/java/org/elasticsearch/client/documentation/CRUDDocumentationIT.java

@@ -686,7 +686,7 @@ public void testBulk() throws Exception {
                    .source(XContentType.JSON,"field", "baz"));
            // end::bulk-request
            // tag::bulk-execute
-            BulkResponse bulkResponse = client.bulk(request, RequestOptions.DEFAULT);
+            BulkResponse bulkResponse = client.bulk(request, RequestOptions.DEFAULT); 


Small comment -- some trailing whitespace here.

jtibshirani · 2019-01-04T06:34:24Z

...t-high-level/src/test/java/org/elasticsearch/client/documentation/RollupDocumentationIT.java

@@ -103,7 +103,7 @@ public void setUpDocs() throws IOException {
                .endObject());
            bulkRequest.add(indexRequest);
        }
-        BulkResponse bulkResponse = highLevelClient().bulk(bulkRequest, RequestOptions.DEFAULT);
+        BulkResponse bulkResponse = highLevelClient().bulk(bulkRequest, expectWarnings(BulkRequest.TYPES_DEPRECATION_MESSAGE));


Would it be possible to modify the bulk call instead of expecting a warning here?

jtibshirani · 2019-01-04T06:41:50Z

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

@@ -495,6 +496,9 @@ public void testRollover() throws IOException {
        Request bulkRequest = new Request("POST", "/" + index + "_write/doc/_bulk");
        bulkRequest.setJsonEntity(bulk.toString());
        bulkRequest.addParameter("refresh", "");
+        bulkRequest.setOptions(expectVersionSpecificWarnings((v)->{


I think this can just be expectWarnings (same as the call below).

jtibshirani · 2019-01-04T06:48:44Z

server/src/main/java/org/elasticsearch/rest/action/document/RestBulkAction.java

@@ -76,6 +80,8 @@ public RestChannelConsumer prepareRequest(final RestRequest request, final NodeC
        String defaultType = request.param("type");
        if (defaultType == null) {
            defaultType = MapperService.SINGLE_MAPPING_NAME;
+        } else {
+            deprecationLogger.deprecatedAndMaybeLog("bulk_with_types", BulkRequest.TYPES_DEPRECATION_MESSAGE);            


Small comment: would it make sense to move the deprecation message to this action class, for consistency with the other APIs?

If that's the convention, sure, I can make that change

markharwood · 2019-01-04T21:50:32Z

run gradle build tests

jtibshirani

This is looking good to me! I will wait on the build results before hitting 'approve', in case another issue comes up or you'd like another pair of eyes on it (some of the changes around defaults in IndexRequest are a bit subtle).

jtibshirani · 2019-01-04T22:41:57Z

...est-high-level/src/test/java/org/elasticsearch/client/BulkRequestWithGlobalParametersIT.java

+
+    private BulkResponse bulkWithTypes(BulkRequest request) throws IOException {
+        BulkResponse bulkResponse = execute(request, highLevelClient()::bulk, highLevelClient()::bulkAsync, 
+                expectVersionSpecificWarnings((v)->{v.current(BulkRequest.TYPES_DEPRECATION_MESSAGE);}));


One more instance of expectVersionSpecificWarnings that can be updated to expectWarnings.

jtibshirani · 2019-01-04T22:42:13Z

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

@@ -1070,6 +1072,9 @@ private void checkSnapshot(String snapshotName, int count, Version tookOnVersion
        Request writeToRestoredRequest = new Request("POST", "/restored_" + index + "/doc/_bulk");
        writeToRestoredRequest.addParameter("refresh", "true");
        writeToRestoredRequest.setJsonEntity(bulk.toString());
+        writeToRestoredRequest.setOptions(expectVersionSpecificWarnings((v)->{


Same thought here.

jtibshirani · 2019-01-04T22:49:04Z

server/src/main/java/org/elasticsearch/action/index/IndexRequest.java

@@ -82,7 +83,7 @@
     */
    static final int MAX_SOURCE_LENGTH_IN_TOSTRING = 2048;

-    private String type = MapperService.SINGLE_MAPPING_NAME;
+    private String type;


It might be useful to add a comment to these files IndexRequest, UpdateRequest, and DeleteRequest) explaining the different states of the type parameter, as it is not very obvious to a newcomer. I really should've done this in my initial PRs that began to modify these files, apologies for punting on that.

jtibshirani · 2019-01-04T22:49:41Z

@elasticmachine run gradle build tests 1

jtibshirani

Thanks @markharwood, this looks good to me. My only other thought is that it'd be good to update the PR description with an explanation of how bulk, index, update, and delete requests have changed (perhaps mentioning defaultTypeIfNull).

jtibshirani · 2019-01-07T19:34:34Z

server/src/test/java/org/elasticsearch/rest/action/document/RestBulkActionTests.java

@@ -51,7 +51,7 @@ public void testBulkPipelineUpsert() throws Exception {
        new RestBulkAction(settings(Version.CURRENT).build(), mock(RestController.class))


In other PRs we have added a dedicated test here to ensure we're emitting deprecation warnings. However, there are several integration tests in this PR that maked typed calls and check for deprecation warnings, so I don't think it's strictly necessary.

markharwood · 2019-01-08T18:52:22Z

run gradle build tests

cbuescher

@markharwood I did a first quick pass ignoring the changes in test classes, will take a look at those also if needed

cbuescher · 2019-01-09T16:32:20Z

server/src/main/java/org/elasticsearch/action/bulk/BulkRequest.java

+    /**
+     * @deprecated Types are in the process of being removed. Use {@link #BulkRequest(String)} instead
+     */
+    @Deprecated
    public BulkRequest(@Nullable String globalIndex, @Nullable String globalType) {


It looks like CRUDDocumentationIT is using this deprecatd ctor still (amongst other). Since its code that is contained in the docs, should we move that to using the typeless constructor?

I'm not sure - it's documentation showing how index and type can be associated with the BulkRequest and "inherited" by an IndexRequest objects that are missing these values.

I suppose it's a general point if we want documentation to include supported-yet-deprecated features? I can pull the docs/code if we think that's the right thing. Thoughts? cc @jtibshirani

We've been removing documentation for deprecated features, with the understanding that users can refer to the previous version's documentation (where the feature is not deprecated).

Thanks for the clarification, I think you are refering to https://www.elastic.co/guide/en/elasticsearch/client/java-rest/master/java-rest-high-document-bulk.html#CO102-1. I would be in favour of removing the mentioning of the type there as well and changing to the other ctor then.

cbuescher · 2019-01-09T16:43:38Z

server/src/main/java/org/elasticsearch/action/DocWriteRequest.java

+     * or empty
+     * @return the Request
+     */
+    T defaultTypeIfNull(String defaultType);


Can you briefly explain why this is added? Only to improve my understanding of the PR.

Bulk requests can specify a default index and type which is automatically filled-in on any contained Index/Update/DeleteRequests that are missing these properties. A form of inheritance.

This is the method where any blank choice of type is overridden with the BulkRequest's choice of type. This code became necessary after a different PR switched the Index/Update/DeleteRequests to default to _doc instead of null - all the bulk type inheritance tests failed because we couldn't differentiate between objects where the user had explicitly defined a choice of _doc and those where the user had not made an explicit choice. Here we have to revert back to holding a null for this setting by default.

Thanks for the explanation, think I got it.

server/src/main/java/org/elasticsearch/action/bulk/BulkRequestBuilder.java

server/src/main/java/org/elasticsearch/action/bulk/BulkRequest.java

cbuescher · 2019-01-09T16:50:05Z

server/src/main/java/org/elasticsearch/action/index/IndexRequest.java

@@ -152,9 +154,6 @@ public IndexRequest(String index, String type, String id) {
    @Override
    public ActionRequestValidationException validate() {
        ActionRequestValidationException validationException = super.validate();
-        if (type == null) {
-            validationException = addValidationError("type is missing", validationException);
-        }


Any reason this is removed here but still validated in DeleteRequest?

No, I can't think why they should differ

server/src/main/java/org/elasticsearch/action/index/IndexRequest.java

server/src/main/java/org/elasticsearch/action/update/UpdateRequest.java

cbuescher · 2019-01-09T17:58:13Z

@markharwood rest of the PR (test changes) look good to me btw.

cbuescher

@markharwood thanks for the update, LGTM.

Added warnings checks to existing tests Added “defaultTypeIfNull” to DocWriteRequest interface so that Bulk requests can override a null choice of document type with any global custom choice.

markharwood added WIP :Search Foundations/Mapping Index mappings, including merging and defining field types v7.0.0 >refactoring labels Dec 12, 2018

markharwood force-pushed the fix/35190bulk branch 6 times, most recently from ea45401 to e339b40 Compare December 17, 2018 16:43

jtibshirani mentioned this pull request Dec 17, 2018

Implementation tracking for 7.0 types deprecation. #35190

Closed

48 tasks

markharwood force-pushed the fix/35190bulk branch 2 times, most recently from a940e53 to 177e75a Compare December 18, 2018 09:22

markharwood force-pushed the fix/35190bulk branch 6 times, most recently from f578b66 to c0e198f Compare December 20, 2018 14:23

markharwood force-pushed the fix/35190bulk branch 3 times, most recently from c36372f to c0b5dea Compare December 24, 2018 10:42

markharwood force-pushed the fix/35190bulk branch 3 times, most recently from 46ceae8 to e3233f3 Compare January 2, 2019 17:06

markharwood force-pushed the fix/35190bulk branch 2 times, most recently from d69dec4 to 49b3568 Compare January 3, 2019 10:28

jtibshirani mentioned this pull request Jan 3, 2019

Types removal - forward-compatible code for bulk API change coming in 7.0 #37105

Closed

jtibshirani reviewed Jan 4, 2019

View reviewed changes

markharwood force-pushed the fix/35190bulk branch from 9609212 to a448066 Compare January 4, 2019 16:24

jtibshirani reviewed Jan 4, 2019

View reviewed changes

markharwood force-pushed the fix/35190bulk branch from 21d2f8d to 32a917a Compare January 7, 2019 13:54

jtibshirani approved these changes Jan 7, 2019

View reviewed changes

markharwood force-pushed the fix/35190bulk branch 3 times, most recently from 2955a7f to 4bea050 Compare January 8, 2019 15:59

jtibshirani requested a review from cbuescher January 8, 2019 18:24

markharwood force-pushed the fix/35190bulk branch 2 times, most recently from f0d8a00 to c2a16b7 Compare January 9, 2019 11:14

cbuescher reviewed Jan 9, 2019

View reviewed changes

markharwood force-pushed the fix/35190bulk branch 2 times, most recently from facae87 to 89aa779 Compare January 10, 2019 10:44

cbuescher approved these changes Jan 10, 2019

View reviewed changes

markharwood force-pushed the fix/35190bulk branch from 4ee1cf8 to 0f17436 Compare January 10, 2019 15:36

Added deprecation warnings to _bulk apis

82e72df

Added warnings checks to existing tests Added “defaultTypeIfNull” to DocWriteRequest interface so that Bulk requests can override a null choice of document type with any global custom choice.

markharwood force-pushed the fix/35190bulk branch from 0f17436 to 82e72df Compare January 10, 2019 16:16

markharwood merged commit 4344305 into elastic:master Jan 10, 2019

dliappis mentioned this pull request Jan 14, 2019

Indexing throughput drop after types removal issues deprecation messages #37411

Closed

colings86 added v7.0.0-beta1 and removed v7.0.0 labels Feb 7, 2019

		@@ -51,7 +51,7 @@ public void testBulkPipelineUpsert() throws Exception {
		new RestBulkAction(settings(Version.CURRENT).build(), mock(RestController.class))

Types removal - deprecate types in _bulk apis #36549

Types removal - deprecate types in _bulk apis #36549

Conversation

markharwood commented Dec 12, 2018

elasticmachine commented Dec 12, 2018

markharwood commented Dec 18, 2018

markharwood commented Dec 18, 2018

markharwood commented Dec 20, 2018

markharwood commented Dec 20, 2018

markharwood commented Jan 3, 2019

jtibshirani 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

markharwood commented Jan 4, 2019

jtibshirani 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

jtibshirani commented Jan 4, 2019

jtibshirani left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

markharwood commented Jan 8, 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

cbuescher commented Jan 9, 2019

cbuescher left a comment

Choose a reason for hiding this comment