Deprecate types in get field mapping API #37667

mayya-sharipova · 2019-01-21T17:58:37Z

Add deprecation warning to RestGetFieldMappingAction
Add two new java HRLC classes GetFieldMappingsRequest and
GetFieldMappingsResponse. These classes use new typeless forms
of a request and response, and differ in that from the server
versions.

Relates to #35190

elasticmachine · 2019-01-21T17:59:24Z

Pinging @elastic/es-search

- Add deprecation warning to RestGetFieldMappingAction - Add two new java HRLC classes GetFieldMappingsRequest and GetFieldMappingsResponse. These classes use new typeless forms of a request and response, and differ in that from the server versions. Relates to elastic#35190

jtibshirani

Thanks @mayya-sharipova, this is looking really nice to me! I'll add @hub-cap as a reviewer as he might want to follow along with the HLRC changes.

jtibshirani · 2019-01-22T00:31:09Z

...rc/test/java/org/elasticsearch/rest/action/admin/indices/RestGetFieldMappingActionTests.java

@@ -40,6 +40,24 @@ public void setUpAction() {
        new RestGetFieldMappingAction(Settings.EMPTY, controller());
    }

+    public void testIncludeTypeName() {
+        Map<String, String> params = new HashMap<>();
+        params.put(INCLUDE_TYPE_NAME_PARAMETER, "true");


Maybe we could randomly choose either "true" or "false" here, since both options should produce a deprecation message.

jtibshirani · 2019-01-22T00:34:48Z

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

+
+    private IndicesOptions indicesOptions = IndicesOptions.strictExpandOpen();
+
+    public GetFieldMappingsRequest() {}


Small comment: do we need this constructor?

is nothing actually required here in the request? If not then it makes sense to remove it

jtibshirani · 2019-01-22T00:44:41Z

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

+    }
+
+
+    /** returns the retrieved field mapping. The return map keys are index and field (as specified in the request). */


Small comment, we could format this comment more as typical javadoc (capital letters, newline after opening /**).

jtibshirani · 2019-01-22T00:48:56Z

...high-level/src/test/java/org/elasticsearch/client/indices/GetFieldMappingsResponseTests.java

+                final String mapping = randomBoolean() ? "{\"type\":\"string\"}" : "{\"type\":\"keyword\"}";
+                FieldMappingMetaData metaData =
+                    new FieldMappingMetaData("my field", new BytesArray(mapping));
+                fieldMappings.put("field" + k, metaData);


Small comment: I think it's more common to use randomAlphaOfLength(...) instead of hardcoding names like "index" + i" and "my field". Also would it make sense to sometimes choose FieldMappingMetaData.NULL as well?

@jtibshirani Thanks for the comment. I have removed FieldMappingMetaData.NULL in the client GetFieldMappingsResponse as it is not necessary. An empty FieldMappingMetaData will indicate that field(s) are not found.

hub-cap

That is some hairy parsing logic from the old code :) You might add a note to the old ones that mentions the new client side ones and how changes to the old parsing logic shouldnt differ from the client side ones as well, just so people know if they change it they should also change these... OH I CANT WAIT TILL WE ARE GENERATING THESE :P

Im LGTM once @jtibshirani is LGTM w the PR.

hub-cap · 2019-01-22T15:10:32Z

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

+
+    private IndicesOptions indicesOptions = IndicesOptions.strictExpandOpen();
+
+    public GetFieldMappingsRequest() {}


is nothing actually required here in the request? If not then it makes sense to remove it

hub-cap · 2019-01-22T15:17:42Z

...high-level/src/test/java/org/elasticsearch/client/indices/GetFieldMappingsResponseTests.java

+                final String mapping = randomBoolean() ? "{\"type\":\"string\"}" : "{\"type\":\"keyword\"}";
+                FieldMappingMetaData metaData =
+                    new FieldMappingMetaData("my field", new BytesArray(mapping));
+                fieldMappings.put("field" + k, metaData);


mayya-sharipova · 2019-01-22T21:18:08Z

@jtibshirani and @hub-cap thanks for the review. I have addressed all your comments.

@jtibshirani This is ready for another review, whenever you have time.

jtibshirani

Looks good to me as well, just left a couple non-critical comments.

jtibshirani · 2019-01-22T21:23:15Z

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

+/** Request the mappings of specific fields */
+public class GetFieldMappingsRequest implements Validatable {
+
+    protected boolean local = false;


Small comment, can this be private?

jtibshirani · 2019-01-22T21:30:05Z

...high-level/src/test/java/org/elasticsearch/client/indices/GetFieldMappingsResponseTests.java

+                int fields = randomInt(10);
+                for (int k = 0; k < fields; k++) {
+                    final String mapping = randomBoolean() ? "{\"type\":\"string\"}" : "{\"type\":\"keyword\"}";
+                    FieldMappingMetaData metaData = new FieldMappingMetaData("my field", new BytesArray(mapping));


Small comment, "my field" -> randomAlphaOfLength(8).

…ndices_get_field_mapping

…ead-de-duplication * elastic/master: Use explicit version for build-tools in example plugin integ tests (elastic#37792) Change `rational` to `saturation` in script_score (elastic#37766) Deprecate types in get field mapping API (elastic#37667) Add ability to listen to group of affix settings (elastic#37679) Ensure changes requests return the latest mapping version (elastic#37633) Make Minio Setup more Reliable (elastic#37747)

* elastic/master: (85 commits) Use explicit version for build-tools in example plugin integ tests (elastic#37792) Change `rational` to `saturation` in script_score (elastic#37766) Deprecate types in get field mapping API (elastic#37667) Add ability to listen to group of affix settings (elastic#37679) Ensure changes requests return the latest mapping version (elastic#37633) Make Minio Setup more Reliable (elastic#37747) Liberalize StreamOutput#writeStringList (elastic#37768) Add PersistentTasksClusterService::unassignPersistentTask method (elastic#37576) Tests: disable testRandomGeoCollectionQuery on tiny polygons (elastic#37579) Use ILM for Watcher history deletion (elastic#37443) Make sure PutMappingRequest accepts content types other than JSON. (elastic#37720) Retry ILM steps that fail due to SnapshotInProgressException (elastic#37624) Use disassociate in preference to deassociate (elastic#37704) Delete Redundant RoutingServiceTests (elastic#37750) Always return metadata version if metadata is requested (elastic#37674) [TEST] Mute MlMappingsUpgradeIT testMappingsUpgrade Streamline skip_unavailable handling (elastic#37672) Only bootstrap and elect node in current voting configuration (elastic#37712) Ensure either success or failure path for SearchOperationListener is called (elastic#37467) Target only specific index in update settings test ...

- Add deprecation warning to RestGetFieldMappingAction - Add two new java HRLC classes GetFieldMappingsRequest and GetFieldMappingsResponse. These classes use new typeless forms of a request and response, and differ in that from the server versions. Backport for elastic#37667 Relates to elastic#35190

- Add deprecation warning to RestGetFieldMappingAction - Add two new java HRLC classes GetFieldMappingsRequest and GetFieldMappingsResponse. These classes use new typeless forms of a request and response, and differ in that from the server versions. Backport for #37667 Relates to #35190

mayya-sharipova added v7.0.0 v6.7.0 >deprecation :Search Foundations/Mapping Index mappings, including merging and defining field types labels Jan 21, 2019

mayya-sharipova force-pushed the deprecate_types_indices_get_field_mapping branch from 05aee75 to 9e0801c Compare January 21, 2019 19:34

jtibshirani mentioned this pull request Jan 21, 2019

Implementation tracking for 7.0 types deprecation. #35190

Closed

48 tasks

jtibshirani reviewed Jan 22, 2019

View reviewed changes

jtibshirani requested a review from hub-cap January 22, 2019 02:07

hub-cap approved these changes Jan 22, 2019

View reviewed changes

Address Julie's and Michael's comments

5343850

jtibshirani approved these changes Jan 22, 2019

View reviewed changes

mayya-sharipova added 2 commits January 23, 2019 08:51

Address Julie's comments 2

d33a654

Merge remote-tracking branch 'upstream/master' into deprecate_types_i…

a9fd592

…ndices_get_field_mapping

mayya-sharipova merged commit c8565fe into elastic:master Jan 23, 2019

This was referenced Jan 24, 2019

Deprecate types in get field mapping API (#37667) #37795

Merged

Correct deprec log in RestGetFieldMappingAction #37843

Merged

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Deprecate types in get field mapping API #37667

Deprecate types in get field mapping API #37667

mayya-sharipova commented Jan 21, 2019

elasticmachine commented Jan 21, 2019

jtibshirani left a comment

jtibshirani Jan 22, 2019

jtibshirani Jan 22, 2019

hub-cap Jan 22, 2019

jtibshirani Jan 22, 2019

jtibshirani Jan 22, 2019

hub-cap Jan 22, 2019

mayya-sharipova Jan 22, 2019

hub-cap left a comment

hub-cap Jan 22, 2019

hub-cap Jan 22, 2019

mayya-sharipova commented Jan 22, 2019

jtibshirani left a comment

jtibshirani Jan 22, 2019

jtibshirani Jan 22, 2019 •

edited

Loading


		private IndicesOptions indicesOptions = IndicesOptions.strictExpandOpen();

		public GetFieldMappingsRequest() {}

		}


		/** returns the retrieved field mapping. The return map keys are index and field (as specified in the request). */

Deprecate types in get field mapping API #37667

Deprecate types in get field mapping API #37667

Conversation

mayya-sharipova commented Jan 21, 2019

elasticmachine commented Jan 21, 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

hub-cap left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mayya-sharipova commented Jan 22, 2019

jtibshirani left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jtibshirani Jan 22, 2019 • edited Loading

Choose a reason for hiding this comment

jtibshirani Jan 22, 2019 •

edited

Loading