Remove `index.mapping.single_type=false` from core/tests #25331

s1monw · 2017-06-21T10:18:40Z

This change cleans up core tests to not use index.mapping.single_type=false
but instead where applicable use a single type or markt the index as created
with a pre 6.x version.

Relates to #24961

This change cleans up core tests to not use `index.mapping.single_type=false` but instead where applicable use a single type or markt the index as created with a pre 6.x version. Relates to elastic#24961

jpountz

I think it's a fair way to fix those tests. LGTM.

nik9000 · 2017-06-21T12:55:19Z

I dunno. I expected this process to consist more of removing multiple types in the tests and less of declaring indices to be old. The test names don't make it look like they are testing old index behavior. They mostly look like they intend to test "normal" index behavior.

I think this kicks the can down to 7.0 when someone will have to look at these tests and decide weather to rewrite them with one type or just remove them.

jpountz · 2017-06-21T13:27:48Z

The test names don't make it look like they are testing old index behavior.

They do since they are using parent/child?

s1monw · 2017-06-21T13:59:21Z

I dunno. I expected this process to consist more of removing multiple types in the tests and less of declaring indices to be old. The test names don't make it look like they are testing old index behavior. They mostly look like they intend to test "normal" index behavior.

I did convert quite a few of them, actually all of the ones that are possible and I invested a lot of time. I think you should look closer and tell me the ones that I missed. We are in-fact supporting multiple types for 5.x indices so we should also test the APIs against it. This is why I kept or duplicated a lot of them.

martijnvg

LGTM

nik9000

I left a bunch of comments, mostly asking for comments explaining why we need multiple types and/or asking whether we should have a "single type" variant of tests.

nik9000 · 2017-06-21T14:04:04Z

core/src/test/java/org/elasticsearch/get/GetActionIT.java

                .addMapping("type1", mapping1, XContentType.JSON)
                .addMapping("type2", mapping2, XContentType.JSON)
-                .setSettings("index.refresh_interval", -1, "index.mapping.single_type", false));
+                .setSettings("index.refresh_interval", -1, "index.version.created", Version.V_5_6_0.id)); // multi types in 5.6


If this is testing 5.x compat we should rename the test. It reads like it is testing stored fields with multiple values.

nik9000 · 2017-06-21T14:11:34Z

core/src/test/java/org/elasticsearch/get/GetActionIT.java

                .addMapping("my-type1", "_parent", "type=parent", "field1", "type=keyword,store=true")
                .addAlias(new Alias("alias"))
-                .setSettings("index.refresh_interval", -1, "index.mapping.single_type", false));
+                .setSettings("index.refresh_interval", -1,  "index.version.created", Version.V_5_6_0.id)); // multi types in 5.6


Maybe we're better off testing old style parent/child metadata in its own test. That way we can remove the entire test when we no longer support it. Maybe that'd be as simple as moving routing into another test and renaming the test.

nik9000 · 2017-06-21T14:18:09Z

core/src/test/java/org/elasticsearch/indices/IndicesOptionsIntegrationIT.java


        for (String index : Arrays.asList("foo", "foobar", "bar", "barbaz")) {
-            assertAcked(prepareCreate(index).setSettings("index.mapping.single_type", false));
+            assertAcked(prepareCreate(index).setSettings("index.version.created", Version.V_5_6_0.id)); // allows for multiple types


Should this test have a multi-type variant and a single type variant?

nik9000 · 2017-06-21T14:19:50Z

core/src/test/java/org/elasticsearch/indices/mapping/SimpleGetFieldMappingsIT.java


        assertAcked(prepareCreate("indexa")
-                .setSettings("index.mapping.single_type", false)
+                .setSettings("index.version.created", Version.V_5_6_0.id)


Does this need a single-type variant?

nik9000 · 2017-06-21T14:21:44Z

core/src/test/java/org/elasticsearch/indices/mapping/SimpleGetMappingsIT.java

    public void testSimpleGetMappings() throws Exception {
        client().admin().indices().prepareCreate("indexa")
-                .setSettings("index.mapping.single_type", false)
+                .setSettings("index.version.created", Version.V_5_6_0.id)


I don't think multiple types are "simple" any more. Should this have a single type and a multi-type variant?

nik9000 · 2017-06-21T15:21:27Z

core/src/test/java/org/elasticsearch/indices/template/SimpleIndexTemplateIT.java


        assertAcked(prepareCreate("test_index")
-                .setSettings("index.mapping.single_type", false)
+                .setSettings("index.version.created", Version.V_5_6_0.id) // allow for multiple version


Is it worth adding a single type variant of this?

nik9000 · 2017-06-21T15:26:42Z

core/src/test/java/org/elasticsearch/search/fetch/subphase/highlight/HighlighterSearchIT.java


-        assertAcked(prepareCreate("test").setSettings(builder.build()).addMapping("type1", type1TermVectorMapping())
-                .addMapping("type2",
+        assertAcked(prepareCreate("test").setSettings(builder.build()).addMapping("type1", type1TermVectorMapping()));


Can you rename these indices? I know I'll get test and test1 mixed up. Even test1 and test2 would be better.

nik9000 · 2017-06-21T15:30:12Z

core/src/test/java/org/elasticsearch/search/fields/SearchFieldsIT.java

    public void testLoadMetadata() throws Exception {
        assertAcked(prepareCreate("test")
-                .setSettings("index.mapping.single_type", false)
+                .setSettings("index.version.created", Version.V_5_6_0.id)


If we have another test for routing I'd rename this one to testLoadParent or something. Then we can nuke it in 7.0 easilly.

nik9000 · 2017-06-21T15:31:18Z

core/src/test/java/org/elasticsearch/search/query/SearchQueryIT.java

+        assertThat(searchResponse.getHits().getHits().length, equalTo(3));
+    }
+
+    public void testBasicQueryByIdMultiType() throws Exception {


nik9000 · 2017-06-21T15:35:54Z

core/src/test/java/org/elasticsearch/update/UpdateIT.java

    public void testContextVariables() throws Exception {
        assertAcked(prepareCreate("test")
-                        .setSettings("index.mapping.single_type", false)
+                        .setSettings("index.version.created", Version.V_5_6_0.id)


Maybe worth leaving a comment that we can remove this setting, the parent, and the second document once we no longer support types.

wont' the test just fail in that case?

s1monw · 2017-06-22T13:08:07Z

@nik9000 I pushed changes

* master: (56 commits) Initialize max unsafe auto ID timestamp on shrink Enable a long translog retention policy by default (elastic#25294) Remove `index.mapping.single_type=false` from core/tests (elastic#25331) test: single type defaults to true since alpha1 and not alpha3 Get short path name for native controllers Live primary-replica resync (no rollback) (elastic#24841) Upgrade to lucene-7.0.0-snapshot-ad2cb77. (elastic#25349) percolator: Deprecate `document_type` parameter. [DOCS] Fixed typo. [rest-api-spec/indices.refresh] Remove old params Remove redundant and broken MD5 checksum from repository-s3 (elastic#25270) Initialize sequence numbers on a shrunken index Port most snapshot/restore static bwc tests to qa:full-cluster-restart (elastic#25296) Javadoc: ThreadPool doesn't reject while shutdown (elastic#23678) test: verify `size_to_upgrade_in_bytes` in assertBusy(...) Docs: Removed duplicated line in mapping docs Add backward compatibility indices for 5.4.2 Update MockTransportService to the age of Transport.Connection (elastic#25320) Add version v5.4.2 after release IndexMetaData: Add internal format index setting (elastic#25292) ...

Remove index.mapping.single_type=false from core/tests

e97651e

This change cleans up core tests to not use `index.mapping.single_type=false` but instead where applicable use a single type or markt the index as created with a pre 6.x version. Relates to elastic#24961

s1monw added >non-issue >test Issues or PRs that are addressing/adding tests v6.0.0 labels Jun 21, 2017

s1monw requested review from jpountz and nik9000 June 21, 2017 10:18

jpountz approved these changes Jun 21, 2017

View reviewed changes

fix IdFieldMapperTests

dff604e

martijnvg approved these changes Jun 21, 2017

View reviewed changes

nik9000 reviewed Jun 21, 2017

View reviewed changes

Apply freedback

b028ea4

nik9000 approved these changes Jun 22, 2017

View reviewed changes

s1monw added 2 commits June 22, 2017 15:37

Merge branch 'master' into remove_multi_types_from_core

4c7e3dc

fix 2 more tests

8cd7289

s1monw merged commit 29e80ee into elastic:master Jun 22, 2017

clintongormley added v6.0.0-beta1 and removed v6.0.0 labels Jul 25, 2017

Remove index.mapping.single_type=false from core/tests #25331

Remove index.mapping.single_type=false from core/tests #25331

Uh oh!

Conversation

s1monw commented Jun 21, 2017

Uh oh!

jpountz left a comment

Choose a reason for hiding this comment

Uh oh!

nik9000 commented Jun 21, 2017

Uh oh!

jpountz commented Jun 21, 2017

Uh oh!

s1monw commented Jun 21, 2017

Uh oh!

martijnvg left a comment

Choose a reason for hiding this comment

Uh oh!

nik9000 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!

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!

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!

s1monw commented Jun 22, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

Remove `index.mapping.single_type=false` from core/tests #25331

Remove `index.mapping.single_type=false` from core/tests #25331