Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Solidify the approach to allowing typeless requests on an index with a custom type. #37450

Closed
jtibshirani opened this issue Jan 15, 2019 · 6 comments · Fixed by #40578
Closed
Assignees
Labels
:Distributed/CRUD A catch all label for issues around indexing, updating and getting a doc by id. Not search. >refactoring :Search Foundations/Mapping Index mappings, including merging and defining field types Team:Search Foundations Meta label for the Search Foundations team in Elasticsearch

Comments

@jtibshirani
Copy link
Contributor

jtibshirani commented Jan 15, 2019

In #35790, we introduced support for making typeless API calls against an index with a custom type name. For example, given an existing index with type my_type, we allow new documents to be indexed using the typeless endpoint index/_doc/id. This is functionality is important in supporting the move to typeless APIs, since many users are likely to be working with older indices that were created with a custom type.

The approach in #35790 is fairly fragile, and we would like to step back and consider if there is something cleaner and more solid we could do instead. To help document the current implementation, here is the list of PRs associated with this work:

@jtibshirani jtibshirani added :Search Foundations/Mapping Index mappings, including merging and defining field types :Distributed/CRUD A catch all label for issues around indexing, updating and getting a doc by id. Not search. v7.0.0 >refactoring labels Jan 15, 2019
@elasticmachine
Copy link
Collaborator

Pinging @elastic/es-distributed

@elasticmachine
Copy link
Collaborator

Pinging @elastic/es-search

@jtibshirani
Copy link
Contributor Author

@jpountz I assigned this to you, but please feel free to ping me if we should find another owner.

@jpountz
Copy link
Contributor

jpountz commented Jan 25, 2019

I looked into this over the past two weeks. Unfortunately I haven't been able to come up with an approach that would feel more solid. On the positive side over the months we have made more and more internal APIs typeless, such as moving from MapperService#documentMapper(String type) to MapperService#documentMapper(). Most call sites are now using the latter. This doesn't prevent bugs, but it helps.

I will keep looking into it over the next weeks, but I don't expect big changes, rather small changes in the same spirit as the one mentioned above.

@jtibshirani
Copy link
Contributor Author

jtibshirani commented Jan 31, 2019

Thanks @jpountz, that makes sense to me! I mentioned this earlier, but I am fairly concerned about the amount of complexity we've been introducing into the mapping code related to mixed typed and typeless APIs. I was wondering if there were some smaller-scale refactors we could do to clarify or unify the points where we're resolving _doc to the custom type name. Although we have REST test coverage, it would also be good to have unit tests (I find it easier to get good coverage with unit tests, and they also help in quickly iterating on bugs).

@jasontedor jasontedor added v8.0.0 and removed v7.0.0 labels Feb 6, 2019
@jpountz jpountz removed the v8.0.0 label Mar 28, 2019
jpountz added a commit to jpountz/elasticsearch that referenced this issue Mar 28, 2019
This is currently only tested via REST tests.

Closes elastic#37450
@jpountz
Copy link
Contributor

jpountz commented Mar 28, 2019

I've been thinking a bit more about this one recently. In general I like the current approach that resolves the type as high in the stack as possible a bit better as it makes things easier to reason about. Having it eg. in MapperService#documentMapper would feel a bit more dangerous to me as it has many call sites and not all of them expect _doc should be an alias of the current type of the index.

I'm leaning towards addressing this issue through testing. I've been reviewing places where we had gaps in testing. One clear one is MetaDataMappingService, which I tried to address via #40578.

jpountz added a commit that referenced this issue Apr 2, 2019
jpountz added a commit to jpountz/elasticsearch that referenced this issue Apr 2, 2019
jpountz added a commit that referenced this issue Apr 4, 2019
gurkankaymak pushed a commit to gurkankaymak/elasticsearch that referenced this issue May 27, 2019
@javanna javanna added the Team:Search Foundations Meta label for the Search Foundations team in Elasticsearch label Jul 16, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
:Distributed/CRUD A catch all label for issues around indexing, updating and getting a doc by id. Not search. >refactoring :Search Foundations/Mapping Index mappings, including merging and defining field types Team:Search Foundations Meta label for the Search Foundations team in Elasticsearch
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants