-
Notifications
You must be signed in to change notification settings - Fork 25k
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
Removes typed endpoint from search and related APIs #41640
Conversation
Pinging @elastic/es-search |
@elasticmachine run elasticsearch-ci/2 |
1 similar comment
@elasticmachine run elasticsearch-ci/2 |
@elasticmachine run elasticsearch-ci/1 |
@elasticmachine run elasticsearch-ci/2 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just filling in on types-related reviews as @jpountz is out. It is great to see +72 -1,104 lines
!
I left one comment on the code and had a couple thoughts:
- Since we're removing support for the typed endpoints, using types in certain Java HLRC calls will now result in failures. Are we planning to remove types in HLRC requests/ request conversion code in a future PR, or would it make sense to also cover it here to avoid an 'in-between' state?
- There are a few more search-related areas where types have been deprecated, such as searches on the
_type
field and the use of types in 'lookup' queries liketerms
lookup and percolator. If this PR is already getting fairly large, perhaps we could just make sure to track them on the meta-issue.
==== Removal of types | ||
|
||
The `/{index}/{type}/_delete_by_query` and `/{index}/{type}/_update_by_query` REST endpoints have been removed in favour of `/{index}/_delete_by_query` and `/{index}/_update_by_query`. Since indexes can now only contain a single unnamed type these typed endpoints are obselete. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder if the phrasing 'indexes can now only contain a single unnamed type' could be confusing. We now provide 'typeless' index creation APIs, which to users seem like they don't introduce a type at all. Perhaps we could just say 'Since indexes no longer contain types, these typed endpoints are obsolete.'
Also small typo: obselete -> obsolete.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
++ good catch, I think your wording is much better. I'll change it in this and the other PR
@elasticmachine run elasticsearch-ci/2 |
Thanks for the review @jtibshirani
I had a look at doing this but unless I'm mistaken
I've added a check-box for this in #41059. I would like to handle this in a separate change to avoid this PR becoming too big |
@elasticmachine run elasticsearch-ci/1 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm happy to tackle this one straight after this PR is merged to avoid the "in between" state being around for very long
Tackling it in the next PR sounds good to me, thanks! I agree there's not a perfect split into PRs because we share the server and HLRC classes in some places.
[float] | ||
==== Removal of types | ||
|
||
The `/{index}/{type}/_search`, `/{index}/{type}/_msearch`, `/{index}/{type}/_search/template` and `/{index}/{type}/_msearch/template` REST endpoints have been removed in favour of `/{index}/_search`, `/{index}/_msearch`, `/{index}/_search/template` and `/{index}/_msearch/template`, since indexes no longer contain types, these typed endpoints are obsolete.. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Accidental double period here (and below).
@elasticmachine run elasticsearch-ci/docbldesx |
@elasticmachine run elasticsearch-ci/1 |
@elasticmachine run elasticsearch-ci/packaging-sample |
…PIs (#54572) This adds a compatible APIs for typed endpoints removed in #41640 202 failing tests These 2 tests are explicitly fixed by this PR CompatRestIT. test {yaml=mtermvectors/21_deprecated_with_types/Deprecated camel case and _ parameters should fail in Term Vectors query} CompatRestIT. test {yaml=mtermvectors/30_mix_typeless_typeful/mtermvectors without types on an index that has types} I think more yml tests should be added to 7.x to cover these endpoints 17th june 1306tests | 197failures | 16ignored | 10m56.91sduration
No description provided.