-
Notifications
You must be signed in to change notification settings - Fork 24.9k
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
Remove support for _type in searches #68564
Conversation
Pinging @elastic/es-search (Team:Search) |
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.
LGTM, thanks @jtibshirani!
@@ -84,10 +84,6 @@ | |||
* Returns the mapped field type for the given field name. | |||
*/ | |||
MappedFieldType get(String field) { | |||
if (field.equals(TypeFieldType.NAME)) { |
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 think this means we can remove the 'type' field and constructor parameter as well?
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.
Oops, yep I'll remove it.
@@ -562,22 +562,6 @@ setup: | |||
|
|||
- match: {hits.total: 4} | |||
|
|||
--- |
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.
is this the only yaml test case that will be affected? Just wanted to make sure we have coverage in 7.x to bring this with compatible api
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.
Yes this is the only one, we had very light test coverage for this capability.
@pgomulka and I spoke about this offline, and I want to lay out my thoughts on why removing this in 8.0 is the correct solution, given our backwards-compatibility guarantees. We're adding version compatibility to our REST layer, meaning that if you have a 7.x client you should be able to communicate with an 8.x cluster without getting parse errors. The motive for this is specifically to allow a staggered upgrade of clients and servers. Without this layer, given the changes in the structure of search responses (eg removal of the 'type' field from search hits), it is possible that a 7.x client communicating with an 8.x server would see a response as invalid and throw an exception; similarly, an 8.x client communicating with a 7.x server would also treat the response as invalid; this leaves users attempting a rolling upgrade from 7.x to 8.x with a conundrum - when should they upgrade their clients? The addition of the compatibility layer solves this by allowing you to upgrade your cluster fully while still running queries from a 7.x client, and then upgrade the clients once the cluster upgrade is complete. Running queries or aggregations against a 'type' field is deprecated in 7.x. Whenever you do this, you will get warning headers and log messages. There is nothing inherent in the difference between a 7.x cluster and an 8.x cluster that prevents users from addressing these deprecation warnings before upgrading, and so I would argue that the compatibility layer should specifically not try and deal with this behaviour. You should fix deprecation warnings before doing an upgrade, full stop. This is a different category of problem to the one that the compatibility layer is designed to deal with, where the difference in response structure is something that cannot be dealt with by the user. |
TL;DR: the compatibility layer is there to deal with things that can't issue a deprecation warning; queries against |
at first I thought we should support this with compatible API. From client's point of view the request shape has changed, the 'meta' field type has gone, so his 7.x client with compatible API should still work. I wonder what is @jakelandis and @Mpdreamz view on this? |
Thanks for the review! I'm going to merge this (as we discussed), but feel free to continue the discussion here. |
we discussed this with @jakelandis and we think that this breaking change does not need a compatible api implementation. |
Adds an 8.0 breaking change for PR #68564
Types are no longer allowed in requests in 8.0, so we can remove support for
using the
_type
field within a search request.Relates to #41059.
Closes #68311.