-
Notifications
You must be signed in to change notification settings - Fork 25.3k
Run TransportGetMappingsAction
on local node
#122921
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
Changes from all commits
db98fba
4ee9ce3
b56dcdb
92f0445
0f28789
13e4dab
590055c
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,5 @@ | ||
pr: 122921 | ||
summary: Run `TransportGetMappingsAction` on local node | ||
area: Indices APIs | ||
type: enhancement | ||
issues: [] |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -9,26 +9,44 @@ | |
|
||
package org.elasticsearch.action.admin.indices.mapping.get; | ||
|
||
import org.elasticsearch.TransportVersions; | ||
import org.elasticsearch.action.ActionRequestValidationException; | ||
import org.elasticsearch.action.IndicesRequest; | ||
import org.elasticsearch.action.support.IndicesOptions; | ||
import org.elasticsearch.action.support.master.info.ClusterInfoRequest; | ||
import org.elasticsearch.action.support.local.LocalClusterStateRequest; | ||
import org.elasticsearch.common.Strings; | ||
import org.elasticsearch.common.io.stream.StreamInput; | ||
import org.elasticsearch.core.TimeValue; | ||
import org.elasticsearch.core.UpdateForV10; | ||
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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Similar to #122885, this also gets rid of the |
||
|
||
private String[] indices = Strings.EMPTY_ARRAY; | ||
private IndicesOptions indicesOptions; | ||
|
||
public GetMappingsRequest(TimeValue masterTimeout) { | ||
super(masterTimeout, IndicesOptions.strictExpandOpen()); | ||
super(masterTimeout); | ||
indicesOptions = IndicesOptions.strictExpandOpen(); | ||
} | ||
|
||
/** | ||
* NB prior to 9.0 this was a TransportMasterNodeReadAction so for BwC we must remain able to read these requests until | ||
* we no longer need to support calling this action remotely. | ||
*/ | ||
@UpdateForV10(owner = UpdateForV10.Owner.DATA_MANAGEMENT) | ||
public GetMappingsRequest(StreamInput in) throws IOException { | ||
super(in); | ||
indices = in.readStringArray(); | ||
if (in.getTransportVersion().before(TransportVersions.V_8_0_0)) { | ||
in.readStringArray(); | ||
} | ||
indicesOptions = IndicesOptions.readIndicesOptions(in); | ||
} | ||
|
||
@Override | ||
|
@@ -40,4 +58,30 @@ public ActionRequestValidationException validate() { | |
public Task createTask(long id, String type, String action, TaskId parentTaskId, Map<String, String> headers) { | ||
return new CancellableTask(id, type, action, "", parentTaskId, headers); | ||
} | ||
|
||
@Override | ||
public GetMappingsRequest indices(String... indices) { | ||
this.indices = indices; | ||
return this; | ||
} | ||
|
||
public GetMappingsRequest indicesOptions(IndicesOptions indicesOptions) { | ||
this.indicesOptions = indicesOptions; | ||
return this; | ||
} | ||
|
||
@Override | ||
public String[] indices() { | ||
return indices; | ||
} | ||
|
||
@Override | ||
public IndicesOptions indicesOptions() { | ||
return indicesOptions; | ||
} | ||
|
||
@Override | ||
public boolean includeDataStreams() { | ||
return true; | ||
} | ||
} |
Original file line number | Diff line number | Diff line change | ||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
@@ -13,12 +13,16 @@ | |||||||||||||||
import org.apache.logging.log4j.Logger; | ||||||||||||||||
import org.elasticsearch.action.ActionListener; | ||||||||||||||||
import org.elasticsearch.action.support.ActionFilters; | ||||||||||||||||
import org.elasticsearch.action.support.master.info.TransportClusterInfoAction; | ||||||||||||||||
import org.elasticsearch.cluster.ClusterState; | ||||||||||||||||
import org.elasticsearch.action.support.ChannelActionListener; | ||||||||||||||||
import org.elasticsearch.action.support.local.TransportLocalProjectMetadataAction; | ||||||||||||||||
import org.elasticsearch.cluster.ProjectState; | ||||||||||||||||
import org.elasticsearch.cluster.block.ClusterBlockException; | ||||||||||||||||
import org.elasticsearch.cluster.block.ClusterBlockLevel; | ||||||||||||||||
import org.elasticsearch.cluster.metadata.IndexNameExpressionResolver; | ||||||||||||||||
import org.elasticsearch.cluster.metadata.MappingMetadata; | ||||||||||||||||
import org.elasticsearch.cluster.project.ProjectResolver; | ||||||||||||||||
import org.elasticsearch.cluster.service.ClusterService; | ||||||||||||||||
import org.elasticsearch.core.UpdateForV10; | ||||||||||||||||
import org.elasticsearch.indices.IndicesService; | ||||||||||||||||
import org.elasticsearch.injection.guice.Inject; | ||||||||||||||||
import org.elasticsearch.tasks.CancellableTask; | ||||||||||||||||
|
@@ -28,12 +32,19 @@ | |||||||||||||||
|
||||||||||||||||
import java.util.Map; | ||||||||||||||||
|
||||||||||||||||
public class TransportGetMappingsAction extends TransportClusterInfoAction<GetMappingsRequest, GetMappingsResponse> { | ||||||||||||||||
public class TransportGetMappingsAction extends TransportLocalProjectMetadataAction<GetMappingsRequest, GetMappingsResponse> { | ||||||||||||||||
|
||||||||||||||||
private static final Logger logger = LogManager.getLogger(TransportGetMappingsAction.class); | ||||||||||||||||
|
||||||||||||||||
private final IndicesService indicesService; | ||||||||||||||||
private final IndexNameExpressionResolver indexNameExpressionResolver; | ||||||||||||||||
|
||||||||||||||||
/** | ||||||||||||||||
* NB prior to 9.0 this was a TransportMasterNodeReadAction so for BwC it must be registered with the TransportService until | ||||||||||||||||
* we no longer need to support calling this action remotely. | ||||||||||||||||
*/ | ||||||||||||||||
@UpdateForV10(owner = UpdateForV10.Owner.DATA_MANAGEMENT) | ||||||||||||||||
@SuppressWarnings("this-escape") | ||||||||||||||||
@Inject | ||||||||||||||||
public TransportGetMappingsAction( | ||||||||||||||||
TransportService transportService, | ||||||||||||||||
|
@@ -46,28 +57,46 @@ public TransportGetMappingsAction( | |||||||||||||||
) { | ||||||||||||||||
super( | ||||||||||||||||
GetMappingsAction.NAME, | ||||||||||||||||
transportService, | ||||||||||||||||
clusterService, | ||||||||||||||||
threadPool, | ||||||||||||||||
actionFilters, | ||||||||||||||||
GetMappingsRequest::new, | ||||||||||||||||
indexNameExpressionResolver, | ||||||||||||||||
GetMappingsResponse::new, | ||||||||||||||||
transportService.getTaskManager(), | ||||||||||||||||
clusterService, | ||||||||||||||||
threadPool.executor(ThreadPool.Names.MANAGEMENT), | ||||||||||||||||
projectResolver | ||||||||||||||||
); | ||||||||||||||||
this.indicesService = indicesService; | ||||||||||||||||
this.indexNameExpressionResolver = indexNameExpressionResolver; | ||||||||||||||||
|
||||||||||||||||
transportService.registerRequestHandler( | ||||||||||||||||
actionName, | ||||||||||||||||
executor, | ||||||||||||||||
false, | ||||||||||||||||
true, | ||||||||||||||||
GetMappingsRequest::new, | ||||||||||||||||
(request, channel, task) -> executeDirect(task, request, new ChannelActionListener<>(channel)) | ||||||||||||||||
); | ||||||||||||||||
} | ||||||||||||||||
|
||||||||||||||||
@Override | ||||||||||||||||
protected ClusterBlockException checkBlock(GetMappingsRequest request, ProjectState state) { | ||||||||||||||||
return state.blocks() | ||||||||||||||||
.indicesBlockedException( | ||||||||||||||||
state.projectId(), | ||||||||||||||||
ClusterBlockLevel.METADATA_READ, | ||||||||||||||||
indexNameExpressionResolver.concreteIndexNames(state.metadata(), request) | ||||||||||||||||
); | ||||||||||||||||
} | ||||||||||||||||
|
||||||||||||||||
@Override | ||||||||||||||||
protected void doMasterOperation( | ||||||||||||||||
protected void localClusterStateOperation( | ||||||||||||||||
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. This one is missing a call to 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 already added a test: Lines 112 to 115 in 4ee9ce3
I think the reason the test isn't failing is that we already check for the task cancellation right before we execute this method: Lines 86 to 88 in afff39e
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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I was viewing the I'm fine either way, so I'll leave it to you whether to include them or not. |
||||||||||||||||
Task task, | ||||||||||||||||
final GetMappingsRequest request, | ||||||||||||||||
String[] concreteIndices, | ||||||||||||||||
final ClusterState state, | ||||||||||||||||
final ProjectState state, | ||||||||||||||||
final ActionListener<GetMappingsResponse> listener | ||||||||||||||||
) { | ||||||||||||||||
logger.trace("serving getMapping request based on version {}", state.version()); | ||||||||||||||||
final Map<String, MappingMetadata> mappings = projectResolver.getProjectMetadata(state) | ||||||||||||||||
((CancellableTask) task).ensureNotCancelled(); | ||||||||||||||||
logger.trace("serving getMapping request based on version {}", state.cluster().version()); | ||||||||||||||||
String[] concreteIndices = indexNameExpressionResolver.concreteIndexNames(state.metadata(), request); | ||||||||||||||||
final Map<String, MappingMetadata> mappings = state.metadata() | ||||||||||||||||
.findMappings(concreteIndices, indicesService.getFieldFilter(), () -> checkCancellation(task)); | ||||||||||||||||
listener.onResponse(new GetMappingsResponse(mappings)); | ||||||||||||||||
} | ||||||||||||||||
|
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.
Was there a specific reason to remove the description and version?
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No that's fine, especially as we're trying to move away from manually editing rest-api-spec. Thanks for the explanation!