Remove types from internal GetFieldMappings request/response objects by romseygeek · Pull Request #48815 · elastic/elasticsearch

romseygeek · 2019-11-01T16:06:57Z

Type filters and intermediate type levels in mappings responses have already been
removed from the GetFieldMappings REST layer; we can also remove them from the
internal Node client classes.

Relates to #41059

elasticmachine · 2019-11-01T16:06:59Z

Pinging @elastic/es-core-features (:Core/Features/Indices APIs)

jtibshirani

This looks good to me, just some small suggestions.

jtibshirani · 2019-11-01T21:57:49Z

...rc/main/java/org/elasticsearch/action/admin/indices/mapping/get/GetFieldMappingsRequest.java

        indices = in.readStringArray();
-        types = in.readStringArray();
+        if (in.getVersion().before(Version.V_8_0_0)) {
+            in.readStringArray();


We could throw an IllegalArgumentException here (as we discussed doing in other APIs).

jtibshirani · 2019-11-01T22:14:37Z

...c/main/java/org/elasticsearch/action/admin/indices/mapping/get/GetFieldMappingsResponse.java

-            for (int j = 0; j < typesSize; j++) {
-                String type = in.readString();
+            Map<String, FieldMappingMetaData> fieldMapBuilder = new HashMap<>();
+            if (in.getVersion().before(Version.V_8_0_0)) {


Small comment, we could structure this as

if (in.getVersion().before(Version.V_8_0_0)) { int typesSize = in.readVInt(); assert typesSize <= 1; in.readString(); // type }

This would allow for unifying the following code and also for keeping the map pre-sizing (new HashMap<>(fieldSize);).

jtibshirani · 2019-11-01T22:18:03Z

...c/main/java/org/elasticsearch/action/admin/indices/mapping/get/GetFieldMappingsResponse.java

+                out.writeVInt(1);
+                out.writeString(MapperService.SINGLE_MAPPING_NAME);
+                out.writeVInt(indexEntry.getValue().size());
+                for (Map.Entry<String, FieldMappingMetaData> fieldEntry : indexEntry.getValue().entrySet()) {


Same small suggestion here, we could unify the part

out.writeVInt(indexEntry.getValue().size()); for (Map.Entry<String, FieldMappingMetaData> fieldEntry : indexEntry.getValue().entrySet()) { ... }

jtibshirani · 2019-11-01T23:30:22Z

Oops, I just saw some tests are failing related to field mappings. I'll do a quick re-review once those are addressed.

…eldmappings

jtibshirani · 2019-11-11T19:16:38Z

server/src/test/java/org/elasticsearch/index/mapper/FieldFilterMapperPluginTests.java

+        GetFieldMappingsResponse response = client().admin().indices().prepareGetFieldMappings("index1").setFields("non-existent").get();
+        Map<String, Map<String, GetFieldMappingsResponse.FieldMappingMetaData>> mappings = response.mappings();
+        assertEquals(1, mappings.size());
+        Map<String, GetFieldMappingsResponse.FieldMappingMetaData> fieldmapping = mappings.get("index1");


Small comment, fieldmapping -> fieldMapping.

jtibshirani

Sorry for the confusion -- I was over-eager on approving (again) and noticed an API change that is important to discuss.

jtibshirani · 2019-11-11T20:55:31Z

...pi-spec/src/main/resources/rest-api-spec/test/indices.get_field_mapping/20_missing_field.yml

        index: test_index
        fields: not_existent

-  - match: { 'test_index.mappings': {}}


I just realized this doesn't seem right -- why did the response change here? It seems like an API break to start returning a completely empty response.

This threw me as well, but if you look at the history it's actually restoring the behaviour that was there before include_type_name was added - see #37210. There's specific logic in RestGetFieldMappingAction to return an empty response if the index exists but the field doesn't, but it wasn't being tripped beforehand because no types parameter was included in the request. Now that types is gone entirely it has reverted back to the previous behaviour, but I agree it seems a bit odd - maybe we should just remove this extra logic entirely?

I see now, thanks for the context! Looks like I subtly changed the behavior there without an explicit discussion.

I also think we should remove the extra logic. Given that we already used this new response format for a whole major release, I think it'd be confusing to change this format again in 8.0. It's also a breaking change that the user can't opt out of.

I noticed that with the current behavior we only return an empty object when a single field is provided. If the request contains two fields that are both non-existent, then we still return the format { "index": { "mappings": {} }. This is true both of the current PR and the old 6.x logic. This seems quite inconsistent, and suggests to me that it's not so bad to keep the { "index": { "mappings": {} } format everywhere.

++, I'll remove that special case and re-test

I just noticed a potential issue.

…eldmappings

jtibshirani

Sorry for the slow turn-around on the review and that there has been a lot of back-and-forth on the PR.

I took a look at your latest changes, especially the fixes around null field mappings in 7e8a493. It seemed to me like the cleanest approach would be to remove all references to probablySingleFieldRequest, since it is no longer has a use. I think this would also let us remove FieldMappingMetaData.NULL.

However I am unsure about removing these because I can't determine why they were originally introduced in #4822. I've read over the PR and issue but still don't understand their purpose. Do you have an understanding of why there was special logic around probablySingleFieldRequest? I think this would be good to investigate (now or in a follow-up) to make sure that we didn't regress something in the response format through our types removal efforts.

…eldmappings

romseygeek · 2019-11-22T11:30:08Z

However I am unsure about removing these because I am can't determine why they were originally introduced in #4822.

I think this is to do with how the maps were built prior to this refactoring, in that because of the double-nested maps it wasn't obvious when a field was missing vs when a type was missing. Now that we only have two levels it's easy to handle a present index but missing field just by passing around empty maps. I've removed the probablySingleFieldRequest parameter and associated special-case logic, let's see what CI makes of it

jtibshirani

Looks good to me, thanks for your patience on this one :) I'll follow-up with the original author on why the logic around probablySingleFieldRequest was added and will loop back if I find anything that needs addressing.

jtibshirani · 2019-11-27T00:08:37Z

...in/java/org/elasticsearch/action/admin/indices/mapping/get/GetFieldMappingsIndexRequest.java

        fields = in.readStringArray();
        includeDefaults = in.readBoolean();
-        probablySingleFieldRequest = in.readBoolean();
+        if (in.getVersion().before(Version.V_8_0_0)) {


Small comment, we could add a comment here explaining this backwards-compatibility logic.

…eldmappings

Remove types from internal GetFieldMappings request/response objects

59ddbd5

romseygeek added :Data Management/Indices APIs DO NOT USE. Use ":Distributed/Indices APIs" or ":StorageEngine/Templates" instead. >refactoring v8.0.0 labels Nov 1, 2019

romseygeek requested review from jtibshirani and mayya-sharipova November 1, 2019 16:06

romseygeek self-assigned this Nov 1, 2019

jtibshirani approved these changes Nov 1, 2019

View reviewed changes

romseygeek added 2 commits November 11, 2019 10:47

feedback; tests

f660bf3

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

c7ce92a

…eldmappings

romseygeek requested a review from jtibshirani November 11, 2019 12:01

jtibshirani previously approved these changes Nov 11, 2019

View reviewed changes

jtibshirani reviewed Nov 11, 2019

View reviewed changes

romseygeek added 4 commits November 12, 2019 17:16

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

0e910dc

…eldmappings

Remove special-case when no fields are found

cc2af67

better tests for null mappings

7e8a493

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

86c90ad

…eldmappings

romseygeek requested a review from jtibshirani November 14, 2019 10:15

romseygeek added 3 commits November 14, 2019 13:27

cleanup

5791352

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

6b06af6

…eldmappings

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

e25bc12

…eldmappings

jtibshirani reviewed Nov 20, 2019

View reviewed changes

romseygeek added 2 commits November 22, 2019 10:41

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

c6ece6c

…eldmappings

Remove special-case logic for existing index but non-existing field

6a3847e

plugin compilation

50bb694

watcher test

0f137dd

jtibshirani approved these changes Nov 27, 2019

View reviewed changes

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

2f96021

…eldmappings

romseygeek merged commit 35355e2 into elastic:master Nov 27, 2019

romseygeek deleted the types-removal/getfieldmappings branch November 27, 2019 11:41

original-brownbear mentioned this pull request Nov 29, 2019

[CI] Failure in org.elasticsearch.backwards.MixedClusterClientYamlTestSuiteIT #49702

Closed

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

Conversation

romseygeek commented Nov 1, 2019

Uh oh!

elasticmachine commented Nov 1, 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!

jtibshirani commented Nov 1, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jtibshirani left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jtibshirani Nov 11, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

romseygeek commented Nov 22, 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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

jtibshirani left a comment •

edited

Loading

jtibshirani Nov 11, 2019 •

edited

Loading

jtibshirani left a comment •

edited

Loading