Remove type filter from GetMappings API #47364

romseygeek · 2019-10-01T13:58:48Z

This commit removes the types filter from the GetMappings API, which is no longer
useful seeing as we can only have a single mapping type per index. It also changes
the structure of GetMappingsResponse and GetIndexResponse to remove the extra
nesting of mappings below the no-longer-relevant type name, and removes the types
methods from the equivalent request classes.

Relates to #41059

elasticmachine · 2019-10-01T13:58:49Z

Pinging @elastic/es-search

colings86 · 2019-10-01T14:17:08Z

server/src/test/java/org/elasticsearch/cluster/metadata/MetaDataIndexStateServiceTests.java

@@ -333,7 +333,7 @@ public void testAddIndexClosedBlocksReusesBlocks() {
    }

    public void testValidateShardLimit() {
-        int nodesInCluster = randomIntBetween(2,100);
+        int nodesInCluster = randomIntBetween(2,90);


This change seems unrelated to types removal?

Fixed by a separate merge in from master

…ppings

…/getmappings

…ppings

jtibshirani

This looks good to me overall, I left a few minor comments.

I was a bit confused at first about what this PR did -- we're removing deprecated HLRC methods and also refactoring transport client classes at the same time. For future APIs perhaps we could split these into two separate tasks, or just clarify in the PR description? It could also be helpful to mention 'transport API' in the PR title, otherwise it's easy to assume it refers to the REST API.

jtibshirani · 2019-10-12T01:24:35Z

server/src/main/java/org/elasticsearch/action/admin/indices/get/GetIndexResponse.java

+                assert MapperService.SINGLE_MAPPING_NAME.equals(type) : "Expected type [_doc] but got " + type;
+                mappingsMapBuilder.put(key, new MappingMetaData(in));
+            }
+            else {


Small comment, I think we always put else on the same line as the previous closing brace: } else {. This applies to a few places in the PR.

jtibshirani · 2019-10-12T01:49:41Z

server/src/main/java/org/elasticsearch/action/support/master/info/ClusterInfoRequest.java

@@ -42,15 +42,19 @@ public ClusterInfoRequest() {
    public ClusterInfoRequest(StreamInput in) throws IOException {
        super(in);
        indices = in.readStringArray();
-        types = in.readStringArray();
+        if (in.getVersion().before(Version.V_8_0_0)) {
+            in.readStringArray();


Small comment, we could assert the types are empty.

I think these can still be non-empty in a mixed cluster with a user-request containing types routed through a 7x node

I don't think we support the case where a user is making typed requests in a mixed 7.x and 8.x cluster?

True, but then I think we need to throw an exception rather than assert? Or is the idea that assertions are only there in our test infrastructure, and in production it would be ignored. In which case maybe we should throw an exception here rather than use an assertion?

Right, it seems like we should be throwing exceptions here (and in other requests) instead of asserts. This would allow us to 'fail fast' when a user starts a rolling upgrade to 8.x but hasn't fully switched to typeless calls.

I think we use asserts in other requests like GraphExploreRequest (I missed that in previous reviews!), perhaps we could update to using exceptions everywhere in a future PR?

jtibshirani · 2019-10-15T22:46:59Z

...er/src/main/java/org/elasticsearch/action/admin/indices/mapping/get/GetMappingsResponse.java

+                    indexMapBuilder.put(index, new MappingMetaData(in));
+                }
+                else {
+                    indexMapBuilder.put(index, null);


It seems best to avoid having null values in maps. It makes the result of Map#get less clear --null result could indicate that the key isn't present, or it could be present but have a null value. Also, many map construction methods (like Map.of) will reject null values. Perhaps we could use Optional here, or introduce a notion of empty MappingMetaData?

I agree, null is nasty here. I've pushed a change to create a new empty mappings placeholder.

jtibshirani · 2019-10-15T23:06:30Z

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

    }

-    private Predicate<String> randomFieldsExcludeFilter() {


It could make sense to keep this as a precaution, to ensure the mappings are always well-formed.

HLRC's AbstractResponseTestCase doesn't seem to support this, unfortunately

…ppings

romseygeek · 2019-10-16T09:06:34Z

I was a bit confused at first about what this PR did -- we're removing deprecated HLRC methods and also refactoring transport client classes at the same time. For future APIs perhaps we could split these into two separate tasks, or just clarify in the PR description? It could also be helpful to mention 'transport API' in the PR title, otherwise it's easy to assume it refers to the REST API.

Yes, sorry about that. I think I'll open a separate PR to just remove all the type-referring deprecated methods in HLRC to prevent confusion in the future

romseygeek · 2019-10-16T16:04:18Z

@elasticmachine run elasticsearch-ci/2
@elasticmachine run elasticsearch-ci/packaging-sample-matrix

jtibshirani

I think I'll open a separate PR to just remove all the type-referring deprecated methods in HLRC to prevent confusion in the future

Seems like a nice idea!

jtibshirani · 2019-10-16T21:05:34Z

x-pack/plugin/enrich/src/main/java/org/elasticsearch/xpack/enrich/EnrichPolicyRunner.java

-        if (indexMapping.keys().size() == 0) {
+        ImmutableOpenMap<String, MappingMetaData> mappings = getIndexResponse.mappings();
+        MappingMetaData indexMapping = mappings.get(sourceIndexName);
+        if (indexMapping == null || indexMapping == MappingMetaData.EMPTY_MAPPINGS) {


I'm wondering why we need to check for null here?

Because the index itself might not exist for some reason, in which case Map.get(key) will return null

Although looking more closely, the GetIndexRequest used above here will error out if the index doesn't exist, so we can remove the null check. Good catch!

jtibshirani · 2019-10-16T21:08:20Z

server/src/main/java/org/elasticsearch/action/admin/indices/get/GetIndexResponse.java

+                    String type = in.readString();
+                    assert MapperService.SINGLE_MAPPING_NAME.equals(type) : "Expected [_doc] but got [" + type + "]";
+                    mappingsMapBuilder.put(index, new MappingMetaData(in));
+                }


If numMappings == 0, I think we would want to put (index, MappingMetaData.EMPTY_MAPPINGS). That way we maintain the invariant that the map contains a key for every requested index.

jtibshirani · 2019-10-16T21:09:15Z

server/src/main/java/org/elasticsearch/action/admin/indices/get/GetIndexResponse.java

-                mappingEntry.value.writeTo(out);
+            if (out.getVersion().before(Version.V_8_0_0)) {
+                out.writeVInt(indexEntry.value == MappingMetaData.EMPTY_MAPPINGS ? 0 : 1);
+                if (indexEntry.value != null) {


Should this be indexEntry.value != MappingMetaData.EMPTY_MAPPINGS?

Missed this one... will fix, thanks!

romseygeek · 2019-10-17T08:54:11Z

@elasticmachine update branch

romseygeek · 2019-10-20T12:53:46Z

@elasticmachine update branch

…ppings

based on pgomulka:compat/delete_update #58246 CompatRestIT. test {yaml=indices.get_field_mapping/30_missing_type/Raise 404 when type doesn't exist} because of that change (custom type to _doc) a test that was expecting 404 for a type that did not exist, passes now as the logic is trying to fetch for _doc (any type that exist on that index will be good). CompatRestIT. test {yaml=indices.get_mapping/11_basic_with_types/Get /{index}/_mapping with empty mappings} this is because I can't tell if the mapping contained a type or not when fetching a mapping. CompatRestIT. test {yaml=indices.get_mapping/20_missing_type/Existent and non-existent type returns 404 and the existing type} CompatRestIT. test {yaml=indices.get_mapping/20_missing_type/Existent and non-existent types returns 404 and the existing type} CompatRestIT. test {yaml=indices.get_mapping/20_missing_type/No type matching pattern returns 404} CompatRestIT. test {yaml=indices.get_mapping/20_missing_type/Non-existent type returns 404} failing - (not returning 404) - because type is always found (defaulted to _doc) CompatRestIT. test {yaml=indices.get_mapping/20_missing_type/Type missing when no types exist} CompatRestIT. test {yaml=indices.get_mapping/61_empty_with_types/Check empty mapping when getting all mappings via /_mapping} CompatRestIT. test {yaml=indices.get_template/11_basic_with_types/Get template with no mappings} CompatRestIT. test {yaml=indices.get_template/11_basic_with_types/Get template} CompatRestIT. test {yaml=indices.put_mapping/20_mix_typeless_typeful/PUT mapping with _doc on an index that has types} CompatRestIT. test {yaml=indices.put_mapping/20_mix_typeless_typeful/PUT mapping with typeless API on an index that has types} #47364 #41676

This was done in master via elastic#47364, but that change was never backported to 7.x. This change only includes the changes to GetIndexResponseTests from that pr. This should also fix the failure in elastic#66653, since no fields are shuffled in the server side response. Closes elastic#66653

This was done in master via #47364, but that change was never backported to 7.x. This change only includes the changes to GetIndexResponseTests from that pr. This should also fix the failure in #66653, since no fields are shuffled in the server side response. Closes #66653

Remove types from GetMappings request and response

de8411a

romseygeek added :Search Foundations/Mapping Index mappings, including merging and defining field types >breaking-java v8.0.0 labels Oct 1, 2019

romseygeek requested review from colings86 and jtibshirani October 1, 2019 13:58

romseygeek self-assigned this Oct 1, 2019

colings86 reviewed Oct 1, 2019

View reviewed changes

romseygeek added 12 commits October 2, 2019 09:32

test failures

e8e8ca9

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

97f5d3a

…ppings

null check

04341c8

null checks - need to work out what to do here

4a053c3

Deal correctly with null mappings

b584dbe

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

de90da4

…ppings

imports

8edbc27

Fix ml mapping merger

51382b9

tests

5bfc3ba

GetMappingsResponse and GetIndexResponse don't return types in mappings

292c045

Merge branch 'types-removal/get-mappings-response' into types-removal…

b911f14

…/getmappings

serialization

53f54e3

romseygeek requested a review from colings86 October 14, 2019 08:03

romseygeek added 3 commits October 14, 2019 09:26

Use proper response tests

777c8eb

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

6227bcb

…ppings

precommit, serialization

3ef6360

jtibshirani reviewed Oct 15, 2019

View reviewed changes

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

b392317

…ppings

romseygeek added 2 commits October 16, 2019 10:39

feedback

b03506c

compilation

e8bdb16

romseygeek added 2 commits October 16, 2019 11:41

still need to deal with nulls in certain cases

c8b64b3

test failures

645de20

jtibshirani reviewed Oct 16, 2019

View reviewed changes

feedback

0e5323d

Merge branch 'master' into types-removal/getmappings

80374dc

jtibshirani approved these changes Oct 18, 2019

View reviewed changes

elasticmachine and others added 3 commits October 20, 2019 05:53

Merge branch 'master' into types-removal/getmappings

0e9a3dd

BWC

9f93b0e

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

8a90d94

…ppings

romseygeek merged commit afeee4b into elastic:master Oct 21, 2019

romseygeek deleted the types-removal/getmappings branch October 21, 2019 09:10

pgomulka mentioned this pull request Jul 8, 2020

Mappings get put and get mappings #58426

Merged

martijnvg mentioned this pull request Jan 4, 2021

Change HLRC GetIndexResponseTests to AbstractResponseTestCase #66934

Merged

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove type filter from GetMappings API #47364

Remove type filter from GetMappings API #47364

romseygeek commented Oct 1, 2019

elasticmachine commented Oct 1, 2019

colings86 Oct 1, 2019

romseygeek Oct 2, 2019

jtibshirani left a comment •

edited

Loading

jtibshirani Oct 12, 2019

jtibshirani Oct 12, 2019

romseygeek Oct 16, 2019

jtibshirani Oct 16, 2019

romseygeek Oct 17, 2019

jtibshirani Oct 18, 2019

jtibshirani Oct 15, 2019 •

edited

Loading

romseygeek Oct 16, 2019

jtibshirani Oct 15, 2019

romseygeek Oct 16, 2019

jtibshirani Oct 16, 2019

romseygeek commented Oct 16, 2019

romseygeek commented Oct 16, 2019

jtibshirani left a comment

jtibshirani Oct 16, 2019

romseygeek Oct 17, 2019

romseygeek Oct 17, 2019

jtibshirani Oct 16, 2019

romseygeek Oct 17, 2019

jtibshirani Oct 16, 2019

romseygeek Oct 17, 2019

romseygeek commented Oct 17, 2019

romseygeek commented Oct 20, 2019

Remove type filter from GetMappings API #47364

Remove type filter from GetMappings API #47364

Conversation

romseygeek commented Oct 1, 2019

elasticmachine commented Oct 1, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jtibshirani left a comment • edited Loading

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

jtibshirani Oct 15, 2019 • edited Loading

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

romseygeek commented Oct 16, 2019

romseygeek commented Oct 16, 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

romseygeek commented Oct 17, 2019

romseygeek commented Oct 20, 2019

jtibshirani left a comment •

edited

Loading

jtibshirani Oct 15, 2019 •

edited

Loading