Run TransportGetMappingsAction on local node
#122921
Conversation
This action solely needs the cluster state, it can run on any node. Additionally, it needs to be cancellable to avoid doing unnecessary work after a client failure or timeout. Relates elastic#101805
nielsbauman
added
>enhancement
:Data Management/Indices APIs
|
Pinging @elastic/es-data-management (Team:Data Management) |
|
Hi @nielsbauman, I've created a changelog YAML for you. |
| import org.elasticsearch.tasks.CancellableTask; | ||
| import org.elasticsearch.tasks.Task; | ||
| import org.elasticsearch.tasks.TaskId; | ||
|
|
||
| import java.io.IOException; | ||
| import java.util.Map; | ||
|
|
||
| public class GetMappingsRequest extends ClusterInfoRequest<GetMappingsRequest> { | ||
| public class GetMappingsRequest extends LocalClusterStateRequest implements IndicesRequest.Replaceable { |
There was a problem hiding this comment.
Similar to #122885, this also gets rid of the ClusterInfo abstraction.
| } | ||
|
|
||
| @Override | ||
| protected void doMasterOperation( | ||
| protected void localClusterStateOperation( |
There was a problem hiding this comment.
This one is missing a call to ((CancellableTask) task).ensureNotCancelled(); somewhere
There was a problem hiding this comment.
(and perhaps add a test for it)
I already added a test:
I think the reason the test isn't failing is that we already check for the task cancellation right before we execute this method:
In other words, all the
((CancellableTask) task).ensureNotCancelled()s I've added in the other classes aren't super valuable (in most places). They're not hurting anything, but they're not super valuable either.
I'll add it to this method too, for consistency, but let me know what you think about all this.
There was a problem hiding this comment.
I was viewing the ((CancellableTask) task).ensureNotCancelled() call as a way to do a check prior to some (semi-)expensive work, to avoid doing it if it was going to be thrown away anyway. It seems like it would still have some value there, though only a little, as you say.
I'm fine either way, so I'll leave it to you whether to include them or not.
|
@elasticmachine update branch |
|
@elasticmachine update branch |
| "deprecated":{ | ||
| "version":"7.8.0", | ||
| "description":"This parameter is a no-op and field mappings are always retrieved locally." | ||
| } | ||
| "deprecated":true |
There was a problem hiding this comment.
Was there a specific reason to remove the description and version?
There was a problem hiding this comment.
Tbh, the reason is that I didn't add a description and version to any of the actions I converted similarly already because I didn't know they existed. It didn't seem worth it to go back and update all the previous ones to include a description and version, so I went for consistency here. Do you think it is worth to go back and update the previous ones to specify a description and version?
There was a problem hiding this comment.
No that's fine, especially as we're trying to move away from manually editing rest-api-spec. Thanks for the explanation!
This action solely needs the cluster state, it can run on any node. Additionally, it needs to be cancellable to avoid doing unnecessary work after a client failure or timeout.
Relates #101805