Make `_doc` work as an alias of the actual type of an index. #39505

jpountz · 2019-02-28T10:43:14Z

This is similar to the work that has been done on 7.x to make typeless API calls
work on indices that have types, except that this commit doesn't introduce
typeless calls, eg. the REST API spec or REST handlers haven't been updated.

It only affects the get, index, update, delete and bulk APIs. Other APIs that
require types such as explain or termvectors are left unchanged.

This is necesarry to allow for rolling upgrades from 6.7 to 7.x while internal
indices might remain queried during upgrade by nodes that are on either version.

Closes #39469

This is similar to the work that has been done on 7.x to make typeless API calls work on indices that have types, except that this commit doesn't introduce typeless calls, eg. the REST API spec or REST handlers haven't been updated. It only affects the get, index, update, delete and bulk APIs. Other APIs that require types such as explain or termvectors are left unchanged. This is necesarry to allow for rolling upgrades from 6.7 to 7.x while internal indices might remain queried during upgrade by nodes that are on either version. Closes elastic#39469

elasticmachine · 2019-02-28T10:43:16Z

Pinging @elastic/es-distributed

jtibshirani

Thanks @jpountz, I left some comments but it looks very close to me. Except for the point around 'routing' below, all changes we tracked in #37450 seem to be reflected here.

I had two additional questions:

Would it be worth adding light tests that use _doc with the transport client? That is the functionality we're actually adding here (and that ML + others will be depending on), so it might be nice to have dedicated tests.
It could be worth catching up with someone from the distributed team to see if any issues stand out to them, or they expect any differences between 6.7 and 7.0. It's very much up to you, I'm just suggesting this as we don't have as much time to let this 'bake' in CI for 6.7 in order to catch tricky issues.

jtibshirani · 2019-03-01T01:58:38Z

server/src/main/java/org/elasticsearch/cluster/metadata/IndexMetaData.java

+     * being used in the mappings. This allows typeless APIs such as 'index' or 'put mappings'
+     * to work against indices with a custom type name.
+     */
+    public String resolveDocumentType(String type) {


Small comment, but would it make sense to always call this as part of the IndexMetaData#mappingOrDefault method, instead of doing mappingOrDefault(indexMetaData.resolveDocumentType...) at each call site? That matches what happens on 7.x slightly more closely, and this could become package-private for testing.

7.x is a bit simpler since this method no longer takes a type parameter. I considered having it as part of mappingOrDefault but then it felt wrong to not have this logic in mapping as well, while the automatic replacement does not always make sense for call sites of the mapping method such as MapperService#assertMappingVersion.

jtibshirani · 2019-03-01T02:25:44Z

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

        assertEquals("the number of source shards [2] must be a factor of [3]", iae.getMessage());
    }
+
+    public void testResolveDocumentType() throws IOException {


Thanks for these great unit tests!

jtibshirani · 2019-03-01T05:16:04Z

server/src/main/java/org/elasticsearch/index/shard/IndexShard.java

        assert versionType.validateVersionForWrites(version);
+
+        String resolvedType = mapperService.resolveDocumentType(sourceToParse.type());
+        if (resolvedType != sourceToParse.type()) {


Since this is a string comparison, it would be good to switch to resolvedType.equals(sourceToParse.type()) == false.

OK I'l do that.

jtibshirani · 2019-03-01T05:29:13Z

server/src/main/java/org/elasticsearch/index/shard/IndexShard.java

                : "op term [ " + opPrimaryTerm + " ] > shard term [" + getOperationPrimaryTerm() + "]";
        assert versionType.validateVersionForWrites(version);
+
+        String resolvedType = mapperService.resolveDocumentType(sourceToParse.type());


Small comment, but I am wondering why this change is structured slightly differently than it is on master -- it just looks like a refactor to me, and that nothing has changed in terms of functionality. If that's true, then maybe it's worth keeping it in the same structure so the code lines up better between versions (even if it's a bit less elegant)?

jtibshirani · 2019-03-01T05:37:00Z

server/src/main/java/org/elasticsearch/index/shard/IndexShard.java

                : "op term [ " + opPrimaryTerm + " ] > shard term [" + getOperationPrimaryTerm() + "]";
        assert versionType.validateVersionForWrites(version);
+
+        type = mapperService.resolveDocumentType(type);


I'm wondering why we need this type resolution here (which doesn't appear on master either)? To me it looks like resolution is already handled in the individual calls to docMapper and prepareDelete.

Thanks for catching this, it was probably only useful in an earlier version of the change.

jtibshirani · 2019-03-01T05:57:47Z

server/src/test/java/org/elasticsearch/index/shard/IndexShardIT.java

+                .put("index.number_of_replicas", 0).build();
+        createIndex("index", settings, "some_type", "_routing", "required=true");
+
+        expectThrows(RoutingMissingException.class,


I didn't see any changes to 'required routing' for delete, update, or get -- I think we might need to resolve the type in some additional places.

Good catch.

jpountz · 2019-03-01T14:13:03Z

@dnhatn I added you as a reviewer since you dug the bug about duplicate translog entries. If you could take a look - even after merge in case Julie approves changes before you have a chance to review - this would be great.

jtibshirani

This looks good to me with the latest changes. I'm also happy if you want to merge now, and have @dnhatn review post-merge.

jpountz · 2019-03-01T16:33:34Z

Thanks @jtibshirani. @dnhatn I'll merge this now, but I'm still interested in your opinion when you have a chance to look.

dnhatn

LGTM.

jpountz · 2019-03-01T17:41:38Z

Thank you @dnhatn !

jpountz added >feature :Distributed Indexing/CRUD A catch all label for issues around indexing, updating and getting a doc by id. Not search. v6.7.0 labels Feb 28, 2019

jpountz added the blocker label Feb 28, 2019

jtibshirani reviewed Mar 1, 2019

View reviewed changes

jpountz added 2 commits March 1, 2019 12:08

Address @jtibshirani's feedback.

43810ef

Fix test to wait for mappings to be available.

8498b34

jpountz requested a review from dnhatn March 1, 2019 14:12

jtibshirani approved these changes Mar 1, 2019

View reviewed changes

jpountz merged commit f14405a into elastic:6.7 Mar 1, 2019

jpountz deleted the feature/typeless_67 branch March 1, 2019 16:33

dnhatn reviewed Mar 1, 2019

View reviewed changes

jpountz mentioned this pull request Mar 5, 2019

_doc should work as an alias for the actual type on 6.7 #39469

Closed

Make _doc work as an alias of the actual type of an index. #39505

Make _doc work as an alias of the actual type of an index. #39505

Uh oh!

Conversation

jpountz commented Feb 28, 2019

Uh oh!

elasticmachine commented Feb 28, 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!

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!

jpountz commented Mar 1, 2019

Uh oh!

jtibshirani left a comment

Choose a reason for hiding this comment

Uh oh!

jpountz commented Mar 1, 2019

Uh oh!

dnhatn left a comment

Choose a reason for hiding this comment

Uh oh!

jpountz commented Mar 1, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Make `_doc` work as an alias of the actual type of an index. #39505

Make `_doc` work as an alias of the actual type of an index. #39505