-
Notifications
You must be signed in to change notification settings - Fork 25.1k
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
Allow indices.get_mapping response parsing without types #37492
Changes from 3 commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
This file was deleted.
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -20,6 +20,7 @@ | |
package org.elasticsearch.rest.action.admin.indices; | ||
|
||
import com.carrotsearch.hppc.cursors.ObjectCursor; | ||
|
||
import org.apache.logging.log4j.LogManager; | ||
import org.apache.logging.log4j.Logger; | ||
import org.elasticsearch.action.admin.indices.mapping.get.GetMappingsRequest; | ||
|
@@ -59,6 +60,8 @@ | |
public class RestGetMappingAction extends BaseRestHandler { | ||
private static final Logger logger = LogManager.getLogger(RestGetMappingAction.class); | ||
private static final DeprecationLogger deprecationLogger = new DeprecationLogger(logger); | ||
static final String TYPES_DEPRECATION_MESSAGE = "[types removal] Using `include_type_name` in get mapping requests is deprecated. " | ||
+ "The parameter will be removed in the next major version."; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. In #37484 I opted for a general "include_type_name is deprecated" message as that is our system-wide policy. I'm not sure if it's more useful to warn people they shouldn't be using the param anywhere or that it was detected in use on a particular API as in this case. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think I like that the deprecation explicitely says which API call this warning comes from. You might be able to see the same from the logger, but then again maybe not. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I've been taking the same approach as @cbuescher, so it is really clear what API call is causing the warning. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. One last (small) comment I thought of -- we should probably standardize on using `include_type_name` (with backticks) vs. include_type_name in these messages, for easy searchability. I think I would vote for no backticks, since I don't see them used often in our logging messages? |
||
|
||
public RestGetMappingAction(final Settings settings, final RestController controller) { | ||
super(settings); | ||
|
@@ -90,6 +93,9 @@ public RestChannelConsumer prepareRequest(final RestRequest request, final NodeC | |
throw new IllegalArgumentException("Types cannot be provided in get mapping requests, unless" + | ||
" include_type_name is set to true."); | ||
} | ||
if (request.hasParam(INCLUDE_TYPE_NAME_PARAMETER)) { | ||
deprecationLogger.deprecatedAndMaybeLog("get_mapping_with_types", TYPES_DEPRECATION_MESSAGE); | ||
} | ||
|
||
final GetMappingsRequest getMappingsRequest = new GetMappingsRequest(); | ||
getMappingsRequest.indices(indices).types(types); | ||
|
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 assuming that we'll revisit the interface to
GetMappingsResponse
for the HLRC, as we plan to do forGetIndexResponse
.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 would say so, as I'm still not conviced we if/how we should do this just now, so I'd like to discuss this more focussed in a separate move.