From 18c5be04cd689f47711a4f7675ad00a245a196e9 Mon Sep 17 00:00:00 2001 From: Bharathwaj G Date: Thu, 1 Sep 2022 17:10:39 +0530 Subject: [PATCH 01/12] Changes for list all and pit segments Signed-off-by: Bharathwaj G --- .../opensearch/client/RequestConverters.java | 4 + .../client/RestHighLevelClient.java | 35 ++++ .../client/{PitIT.java => Pit1IT.java} | 48 +++-- .../org/opensearch/action/ActionModule.java | 4 + .../action/search/GetAllPitNodesResponse.java | 47 ++++- .../opensearch/action/search/ListPitInfo.java | 41 +++-- .../action/cat/RestPitSegmentsAction.java | 165 ++++++++++++++++++ .../action/search/RestGetAllPitsAction.java | 84 +++++++++ 8 files changed, 407 insertions(+), 21 deletions(-) rename client/rest-high-level/src/test/java/org/opensearch/client/{PitIT.java => Pit1IT.java} (65%) create mode 100644 server/src/main/java/org/opensearch/rest/action/cat/RestPitSegmentsAction.java create mode 100644 server/src/main/java/org/opensearch/rest/action/search/RestGetAllPitsAction.java diff --git a/client/rest-high-level/src/main/java/org/opensearch/client/RequestConverters.java b/client/rest-high-level/src/main/java/org/opensearch/client/RequestConverters.java index eedc27d1d2ea7..41391ed6f1c07 100644 --- a/client/rest-high-level/src/main/java/org/opensearch/client/RequestConverters.java +++ b/client/rest-high-level/src/main/java/org/opensearch/client/RequestConverters.java @@ -498,6 +498,10 @@ static Request deleteAllPits() { return new Request(HttpDelete.METHOD_NAME, "/_search/point_in_time/_all"); } + static Request getAllPits() { + return new Request(HttpGet.METHOD_NAME, "/_search/point_in_time/all"); + } + static Request multiSearch(MultiSearchRequest multiSearchRequest) throws IOException { Request request = new Request(HttpPost.METHOD_NAME, "/_msearch"); diff --git a/client/rest-high-level/src/main/java/org/opensearch/client/RestHighLevelClient.java b/client/rest-high-level/src/main/java/org/opensearch/client/RestHighLevelClient.java index 0c73c65f6175f..b054c4f46365f 100644 --- a/client/rest-high-level/src/main/java/org/opensearch/client/RestHighLevelClient.java +++ b/client/rest-high-level/src/main/java/org/opensearch/client/RestHighLevelClient.java @@ -63,6 +63,7 @@ import org.opensearch.action.search.CreatePitResponse; import org.opensearch.action.search.DeletePitRequest; import org.opensearch.action.search.DeletePitResponse; +import org.opensearch.action.search.GetAllPitNodesResponse; import org.opensearch.action.search.MultiSearchRequest; import org.opensearch.action.search.MultiSearchResponse; import org.opensearch.action.search.SearchRequest; @@ -1368,6 +1369,40 @@ public final Cancellable deleteAllPitsAsync(RequestOptions options, ActionListen ); } + /** + * Get all point in time searches using list all PITs API + * + * @param options the request options (e.g. headers), use {@link RequestOptions#DEFAULT} if nothing needs to be customized + * @return the response + */ + public final GetAllPitNodesResponse getAllPits(RequestOptions options) throws IOException { + return performRequestAndParseEntity( + new MainRequest(), + (request) -> RequestConverters.getAllPits(), + options, + GetAllPitNodesResponse::fromXContent, + emptySet() + ); + } + + /** + * Asynchronously get all point in time searches using list all PITs API + * + * @param options the request options (e.g. headers), use {@link RequestOptions#DEFAULT} if nothing needs to be customized + * @param listener the listener to be notified upon request completion + * @return the response + */ + public final Cancellable getAllPitsAsync(RequestOptions options, ActionListener listener) { + return performRequestAsyncAndParseEntity( + new MainRequest(), + (request) -> RequestConverters.getAllPits(), + options, + GetAllPitNodesResponse::fromXContent, + listener, + emptySet() + ); + } + /** * Clears one or more scroll ids using the Clear Scroll API. * diff --git a/client/rest-high-level/src/test/java/org/opensearch/client/PitIT.java b/client/rest-high-level/src/test/java/org/opensearch/client/Pit1IT.java similarity index 65% rename from client/rest-high-level/src/test/java/org/opensearch/client/PitIT.java rename to client/rest-high-level/src/test/java/org/opensearch/client/Pit1IT.java index 395ec6e46a7b3..240696a6cd65b 100644 --- a/client/rest-high-level/src/test/java/org/opensearch/client/PitIT.java +++ b/client/rest-high-level/src/test/java/org/opensearch/client/Pit1IT.java @@ -18,17 +18,19 @@ import org.opensearch.action.search.DeletePitInfo; import org.opensearch.action.search.DeletePitRequest; import org.opensearch.action.search.DeletePitResponse; +import org.opensearch.action.search.GetAllPitNodesResponse; import org.opensearch.common.unit.TimeValue; import java.io.IOException; import java.util.ArrayList; import java.util.List; import java.util.concurrent.TimeUnit; +import java.util.stream.Collectors; /** * Tests point in time API with rest high level client */ -public class PitIT extends OpenSearchRestHighLevelClientTestCase { +public class Pit1IT extends OpenSearchRestHighLevelClientTestCase { @Before public void indexDocuments() throws IOException { @@ -52,21 +54,24 @@ public void indexDocuments() throws IOException { public void testCreateAndDeletePit() throws IOException { CreatePitRequest pitRequest = new CreatePitRequest(new TimeValue(1, TimeUnit.DAYS), true, "index"); - CreatePitResponse pitResponse = execute(pitRequest, highLevelClient()::createPit, highLevelClient()::createPitAsync); - assertTrue(pitResponse.getId() != null); - assertEquals(1, pitResponse.getTotalShards()); - assertEquals(1, pitResponse.getSuccessfulShards()); - assertEquals(0, pitResponse.getFailedShards()); - assertEquals(0, pitResponse.getSkippedShards()); + CreatePitResponse createPitResponse = execute(pitRequest, highLevelClient()::createPit, highLevelClient()::createPitAsync); + assertTrue(createPitResponse.getId() != null); + assertEquals(1, createPitResponse.getTotalShards()); + assertEquals(1, createPitResponse.getSuccessfulShards()); + assertEquals(0, createPitResponse.getFailedShards()); + assertEquals(0, createPitResponse.getSkippedShards()); + GetAllPitNodesResponse getAllPitResponse = highLevelClient().getAllPits(RequestOptions.DEFAULT); + List pits = getAllPitResponse.getPitInfos().stream().map(r -> r.getPitId()).collect(Collectors.toList()); + assertTrue(pits.contains(createPitResponse.getId())); List pitIds = new ArrayList<>(); - pitIds.add(pitResponse.getId()); + pitIds.add(createPitResponse.getId()); DeletePitRequest deletePitRequest = new DeletePitRequest(pitIds); DeletePitResponse deletePitResponse = execute(deletePitRequest, highLevelClient()::deletePit, highLevelClient()::deletePitAsync); assertTrue(deletePitResponse.getDeletePitResults().get(0).isSuccessful()); - assertTrue(deletePitResponse.getDeletePitResults().get(0).getPitId().equals(pitResponse.getId())); + assertTrue(deletePitResponse.getDeletePitResults().get(0).getPitId().equals(createPitResponse.getId())); } - public void testDeleteAllPits() throws IOException { + public void testDeleteAllAndListAllPits() throws IOException { CreatePitRequest pitRequest = new CreatePitRequest(new TimeValue(1, TimeUnit.DAYS), true, "index"); CreatePitResponse pitResponse = execute(pitRequest, highLevelClient()::createPit, highLevelClient()::createPitAsync); CreatePitResponse pitResponse1 = execute(pitRequest, highLevelClient()::createPit, highLevelClient()::createPitAsync); @@ -80,6 +85,11 @@ public void testDeleteAllPits() throws IOException { pitResponse1 = execute(pitRequest, highLevelClient()::createPit, highLevelClient()::createPitAsync); assertTrue(pitResponse.getId() != null); assertTrue(pitResponse1.getId() != null); + GetAllPitNodesResponse getAllPitResponse = highLevelClient().getAllPits(RequestOptions.DEFAULT); + + List pits = getAllPitResponse.getPitInfos().stream().map(r -> r.getPitId()).collect(Collectors.toList()); + assertTrue(pits.contains(pitResponse.getId())); + assertTrue(pits.contains(pitResponse1.getId())); ActionListener deletePitListener = new ActionListener<>() { @Override public void onResponse(DeletePitResponse response) { @@ -95,8 +105,26 @@ public void onFailure(Exception e) { } } }; + final CreatePitResponse pitResponse3 = execute(pitRequest, highLevelClient()::createPit, highLevelClient()::createPitAsync); + + ActionListener getPitsListener = new ActionListener() { + @Override + public void onResponse(GetAllPitNodesResponse response) { + List pits = response.getPitInfos().stream().map(r -> r.getPitId()).collect(Collectors.toList()); + assertTrue(pits.contains(pitResponse3.getId())); + } + + @Override + public void onFailure(Exception e) { + if (!(e instanceof OpenSearchStatusException)) { + throw new AssertionError("List all PITs failed", e); + } + } + }; + highLevelClient().getAllPitsAsync(RequestOptions.DEFAULT, getPitsListener); highLevelClient().deleteAllPitsAsync(RequestOptions.DEFAULT, deletePitListener); // validate no pits case + //highLevelClient().getAllPitsAsync(RequestOptions.DEFAULT, getPitsListener); highLevelClient().deleteAllPitsAsync(RequestOptions.DEFAULT, deletePitListener); } } diff --git a/server/src/main/java/org/opensearch/action/ActionModule.java b/server/src/main/java/org/opensearch/action/ActionModule.java index 74be544123d9f..7dbc222d42072 100644 --- a/server/src/main/java/org/opensearch/action/ActionModule.java +++ b/server/src/main/java/org/opensearch/action/ActionModule.java @@ -385,6 +385,7 @@ import org.opensearch.rest.action.cat.RestClusterManagerAction; import org.opensearch.rest.action.cat.RestNodeAttrsAction; import org.opensearch.rest.action.cat.RestNodesAction; +import org.opensearch.rest.action.cat.RestPitSegmentsAction; import org.opensearch.rest.action.cat.RestPluginsAction; import org.opensearch.rest.action.cat.RestRepositoriesAction; import org.opensearch.rest.action.cat.RestSegmentsAction; @@ -413,6 +414,7 @@ import org.opensearch.rest.action.search.RestCreatePitAction; import org.opensearch.rest.action.search.RestDeletePitAction; import org.opensearch.rest.action.search.RestExplainAction; +import org.opensearch.rest.action.search.RestGetAllPitsAction; import org.opensearch.rest.action.search.RestMultiSearchAction; import org.opensearch.rest.action.search.RestSearchAction; import org.opensearch.rest.action.search.RestSearchScrollAction; @@ -858,6 +860,8 @@ public void initRestHandlers(Supplier nodesInCluster) { // Point in time API registerHandler.accept(new RestCreatePitAction()); registerHandler.accept(new RestDeletePitAction()); + registerHandler.accept(new RestGetAllPitsAction()); + registerHandler.accept(new RestPitSegmentsAction()); for (ActionPlugin plugin : actionPlugins) { for (RestHandler handler : plugin.getRestHandlers( diff --git a/server/src/main/java/org/opensearch/action/search/GetAllPitNodesResponse.java b/server/src/main/java/org/opensearch/action/search/GetAllPitNodesResponse.java index 091447798cf5f..be5421f019c8b 100644 --- a/server/src/main/java/org/opensearch/action/search/GetAllPitNodesResponse.java +++ b/server/src/main/java/org/opensearch/action/search/GetAllPitNodesResponse.java @@ -11,10 +11,13 @@ import org.opensearch.action.FailedNodeException; import org.opensearch.action.support.nodes.BaseNodesResponse; import org.opensearch.cluster.ClusterName; +import org.opensearch.common.ParseField; import org.opensearch.common.io.stream.StreamInput; import org.opensearch.common.io.stream.StreamOutput; +import org.opensearch.common.xcontent.ConstructingObjectParser; import org.opensearch.common.xcontent.ToXContentObject; import org.opensearch.common.xcontent.XContentBuilder; +import org.opensearch.common.xcontent.XContentParser; import java.io.IOException; import java.util.ArrayList; @@ -24,6 +27,8 @@ import java.util.Set; import java.util.stream.Collectors; +import static org.opensearch.common.xcontent.ConstructingObjectParser.constructorArg; + /** * This class transforms active PIT objects from all nodes to unique PIT objects */ @@ -60,14 +65,30 @@ public GetAllPitNodesResponse(List listPitInfos, GetAllPitNodesResp pitInfos.addAll(listPitInfos); } + public GetAllPitNodesResponse( + List listPitInfos, + ClusterName clusterName, + List getAllPitNodeResponse, + List failures + ) { + super(clusterName, getAllPitNodeResponse, failures); + pitInfos.addAll(listPitInfos); + } + @Override public XContentBuilder toXContent(XContentBuilder builder, Params params) throws IOException { builder.startObject(); - builder.startArray("pitInfos"); + builder.startArray("pits"); for (ListPitInfo pit : pitInfos) { pit.toXContent(builder, params); } builder.endArray(); + if (!failures().isEmpty()) { + builder.startArray("failures"); + for (FailedNodeException e : failures()) { + e.toXContent(builder, params); + } + } builder.endObject(); return builder; } @@ -85,4 +106,28 @@ public void writeNodesTo(StreamOutput out, List nodes) th public List getPitInfos() { return Collections.unmodifiableList(new ArrayList<>(pitInfos)); } + + private static final ConstructingObjectParser PARSER = new ConstructingObjectParser<>( + "get_all_pits_response", + true, + (Object[] parsedObjects) -> { + @SuppressWarnings("unchecked") + List listPitInfos = (List) parsedObjects[0]; + List failures = null; + if (parsedObjects.length > 1) { + failures = (List) parsedObjects[1]; + } + if (failures == null) { + failures = new ArrayList<>(); + } + return new GetAllPitNodesResponse(listPitInfos, new ClusterName(""), new ArrayList<>(), failures); + } + ); + static { + PARSER.declareObjectArray(constructorArg(), ListPitInfo.PARSER, new ParseField("pits")); + } + + public static GetAllPitNodesResponse fromXContent(XContentParser parser) throws IOException { + return PARSER.parse(parser, null); + } } diff --git a/server/src/main/java/org/opensearch/action/search/ListPitInfo.java b/server/src/main/java/org/opensearch/action/search/ListPitInfo.java index 4499e7d6e8ef5..25377b8d466d8 100644 --- a/server/src/main/java/org/opensearch/action/search/ListPitInfo.java +++ b/server/src/main/java/org/opensearch/action/search/ListPitInfo.java @@ -8,14 +8,18 @@ package org.opensearch.action.search; +import org.opensearch.common.ParseField; import org.opensearch.common.io.stream.StreamInput; import org.opensearch.common.io.stream.StreamOutput; import org.opensearch.common.io.stream.Writeable; +import org.opensearch.common.xcontent.ConstructingObjectParser; import org.opensearch.common.xcontent.ToXContentFragment; import org.opensearch.common.xcontent.XContentBuilder; import java.io.IOException; +import static org.opensearch.common.xcontent.ConstructingObjectParser.constructorArg; + /** * This holds information about pit reader context such as pit id and creation time */ @@ -36,16 +40,6 @@ public ListPitInfo(StreamInput in) throws IOException { this.keepAlive = in.readLong(); } - @Override - public XContentBuilder toXContent(XContentBuilder builder, Params params) throws IOException { - builder.startObject(); - builder.field("pitId", pitId); - builder.field("creationTime", creationTime); - builder.field("keepAlive", keepAlive); - builder.endObject(); - return builder; - } - public String getPitId() { return pitId; } @@ -60,4 +54,31 @@ public void writeTo(StreamOutput out) throws IOException { out.writeLong(creationTime); out.writeLong(keepAlive); } + + static final ConstructingObjectParser PARSER = new ConstructingObjectParser<>( + "list_pit_info", + true, + args -> new ListPitInfo((String) args[0], (long) args[1], (long) args[2]) + ); + + static { + PARSER.declareString(constructorArg(), new ParseField("pit_id")); + PARSER.declareLong(constructorArg(), new ParseField("creation_time")); + PARSER.declareLong(constructorArg(), new ParseField("keep_alive")); + } + + private static final ParseField CREATION_TIME = new ParseField("creation_time"); + private static final ParseField PIT_ID = new ParseField("pit_id"); + private static final ParseField KEEP_ALIVE = new ParseField("keep_alive"); + + @Override + public XContentBuilder toXContent(XContentBuilder builder, Params params) throws IOException { + builder.startObject(); + builder.field(PIT_ID.getPreferredName(), pitId); + builder.field(CREATION_TIME.getPreferredName(), creationTime); + builder.field(KEEP_ALIVE.getPreferredName(), keepAlive); + builder.endObject(); + return builder; + } + } diff --git a/server/src/main/java/org/opensearch/rest/action/cat/RestPitSegmentsAction.java b/server/src/main/java/org/opensearch/rest/action/cat/RestPitSegmentsAction.java new file mode 100644 index 0000000000000..b9aaaf77b5cba --- /dev/null +++ b/server/src/main/java/org/opensearch/rest/action/cat/RestPitSegmentsAction.java @@ -0,0 +1,165 @@ +/* + * SPDX-License-Identifier: Apache-2.0 + * + * The OpenSearch Contributors require contributions made to + * this file be licensed under the Apache-2.0 license or a + * compatible open source license. + */ + +package org.opensearch.rest.action.cat; + +import org.opensearch.action.admin.cluster.state.ClusterStateRequest; +import org.opensearch.action.admin.cluster.state.ClusterStateResponse; +import org.opensearch.action.admin.indices.segments.IndexSegments; +import org.opensearch.action.admin.indices.segments.IndexShardSegments; +import org.opensearch.action.admin.indices.segments.IndicesSegmentResponse; +import org.opensearch.action.admin.indices.segments.PitSegmentsAction; +import org.opensearch.action.admin.indices.segments.PitSegmentsRequest; +import org.opensearch.action.admin.indices.segments.ShardSegments; +import org.opensearch.client.node.NodeClient; +import org.opensearch.cluster.node.DiscoveryNodes; +import org.opensearch.common.Strings; +import org.opensearch.common.Table; +import org.opensearch.index.engine.Segment; +import org.opensearch.rest.BaseRestHandler; +import org.opensearch.rest.RestHandler; +import org.opensearch.rest.RestRequest; +import org.opensearch.rest.RestResponse; +import org.opensearch.rest.action.RestActionListener; +import org.opensearch.rest.action.RestResponseListener; + +import java.util.ArrayList; +import java.util.Arrays; +import java.util.List; +import java.util.Map; + +import static java.util.Arrays.asList; +import static java.util.Collections.unmodifiableList; +import static org.opensearch.rest.RestRequest.Method.GET; + +/** + * Rest action for pit segments + */ +public class RestPitSegmentsAction extends AbstractCatAction { + @Override + public List routes() { + return unmodifiableList(asList(new Route(GET, "/_cat/pit_segments"), new Route(GET, "/_cat/pit_segments/{pit_id}"))); + } + + @Override + public String getName() { + return "cat_pit_segments_action"; + } + + @Override + public boolean allowSystemIndexAccessByDefault() { + return true; + } + + @Override + protected BaseRestHandler.RestChannelConsumer doCatRequest(final RestRequest request, final NodeClient client) { + final String[] pitIds = Strings.splitStringByCommaToArray(request.param("pit_id")); + final ClusterStateRequest clusterStateRequest = new ClusterStateRequest(); + clusterStateRequest.local(false); + clusterStateRequest.masterNodeTimeout(request.paramAsTime("master_timeout", clusterStateRequest.masterNodeTimeout())); + clusterStateRequest.clear().nodes(true).routingTable(true).indices("*"); + + return channel -> client.admin().cluster().state(clusterStateRequest, new RestActionListener<>(channel) { + @Override + public void processResponse(final ClusterStateResponse clusterStateResponse) { + final PitSegmentsRequest pitSegmentsRequest = new PitSegmentsRequest(); + pitSegmentsRequest.clearAndSetPitIds(new ArrayList<>(Arrays.asList(pitIds))); + client.execute(PitSegmentsAction.INSTANCE, pitSegmentsRequest, new RestResponseListener<>(channel) { + @Override + public RestResponse buildResponse(final IndicesSegmentResponse indicesSegmentResponse) throws Exception { + final Map indicesSegments = indicesSegmentResponse.getIndices(); + Table tab = buildTable(request, clusterStateResponse, indicesSegments); + return RestTable.buildResponse(tab, channel); + } + }); + } + }); + } + + @Override + protected void documentation(StringBuilder sb) { + sb.append("/_cat/pit_segments\n"); + sb.append("/_cat/pit_segments/{pit_id}\n"); + } + + @Override + protected Table getTableWithHeader(RestRequest request) { + Table table = new Table(); + table.startHeaders(); + table.addCell("index", "default:true;alias:i,idx;desc:index name"); + table.addCell("shard", "default:true;alias:s,sh;desc:shard name"); + table.addCell("prirep", "alias:p,pr,primaryOrReplica;default:true;desc:primary or replica"); + table.addCell("ip", "default:true;desc:ip of node where it lives"); + table.addCell("id", "default:false;desc:unique id of node where it lives"); + table.addCell("segment", "default:true;alias:seg;desc:segment name"); + table.addCell("generation", "default:true;alias:g,gen;text-align:right;desc:segment generation"); + table.addCell("docs.count", "default:true;alias:dc,docsCount;text-align:right;desc:number of docs in segment"); + table.addCell("docs.deleted", "default:true;alias:dd,docsDeleted;text-align:right;desc:number of deleted docs in segment"); + table.addCell("size", "default:true;alias:si;text-align:right;desc:segment size in bytes"); + table.addCell("size.memory", "default:true;alias:sm,sizeMemory;text-align:right;desc:segment memory in bytes"); + table.addCell("committed", "default:true;alias:ic,isCommitted;desc:is segment committed"); + table.addCell("searchable", "default:true;alias:is,isSearchable;desc:is segment searched"); + table.addCell("version", "default:true;alias:v,ver;desc:version"); + table.addCell("compound", "default:true;alias:ico,isCompound;desc:is segment compound"); + table.endHeaders(); + return table; + } + + private Table buildTable(final RestRequest request, ClusterStateResponse state, Map indicesSegments) { + Table table = getTableWithHeader(request); + + DiscoveryNodes nodes = state.getState().nodes(); + table.startRow(); + table.addCell("index", "default:true;alias:i,idx;desc:index name"); + table.addCell("shard", "default:true;alias:s,sh;desc:shard name"); + table.addCell("prirep", "alias:p,pr,primaryOrReplica;default:true;desc:primary or replica"); + table.addCell("ip", "default:true;desc:ip of node where it lives"); + table.addCell("id", "default:false;desc:unique id of node where it lives"); + table.addCell("segment", "default:true;alias:seg;desc:segment name"); + table.addCell("generation", "default:true;alias:g,gen;text-align:right;desc:segment generation"); + table.addCell("docs.count", "default:true;alias:dc,docsCount;text-align:right;desc:number of docs in segment"); + table.addCell("docs.deleted", "default:true;alias:dd,docsDeleted;text-align:right;desc:number of deleted docs in segment"); + table.addCell("size", "default:true;alias:si;text-align:right;desc:segment size in bytes"); + table.addCell("size.memory", "default:true;alias:sm,sizeMemory;text-align:right;desc:segment memory in bytes"); + table.addCell("committed", "default:true;alias:ic,isCommitted;desc:is segment committed"); + table.addCell("searchable", "default:true;alias:is,isSearchable;desc:is segment searched"); + table.addCell("version", "default:true;alias:v,ver;desc:version"); + table.addCell("compound", "default:true;alias:ico,isCompound;desc:is segment compound"); + table.endRow(); + for (IndexSegments indexSegments : indicesSegments.values()) { + Map shards = indexSegments.getShards(); + for (IndexShardSegments indexShardSegments : shards.values()) { + ShardSegments[] shardSegments = indexShardSegments.getShards(); + for (ShardSegments shardSegment : shardSegments) { + List segments = shardSegment.getSegments(); + for (Segment segment : segments) { + table.startRow(); + table.addCell(shardSegment.getShardRouting().getIndexName()); + table.addCell(shardSegment.getShardRouting().getId()); + table.addCell(shardSegment.getShardRouting().primary() ? "p" : "r"); + table.addCell(nodes.get(shardSegment.getShardRouting().currentNodeId()).getHostAddress()); + table.addCell(shardSegment.getShardRouting().currentNodeId()); + table.addCell(segment.getName()); + table.addCell(segment.getGeneration()); + table.addCell(segment.getNumDocs()); + table.addCell(segment.getDeletedDocs()); + table.addCell(segment.getSize()); + table.addCell(0L); + table.addCell(segment.isCommitted()); + table.addCell(segment.isSearch()); + table.addCell(segment.getVersion()); + table.addCell(segment.isCompound()); + table.endRow(); + + } + } + } + } + return table; + } +} diff --git a/server/src/main/java/org/opensearch/rest/action/search/RestGetAllPitsAction.java b/server/src/main/java/org/opensearch/rest/action/search/RestGetAllPitsAction.java new file mode 100644 index 0000000000000..3ef3a20ffc9a7 --- /dev/null +++ b/server/src/main/java/org/opensearch/rest/action/search/RestGetAllPitsAction.java @@ -0,0 +1,84 @@ + +/* + * SPDX-License-Identifier: Apache-2.0 + * + * The OpenSearch Contributors require contributions made to + * this file be licensed under the Apache-2.0 license or a + * compatible open source license. + */ + +package org.opensearch.rest.action.search; + +import org.opensearch.action.admin.cluster.state.ClusterStateRequest; +import org.opensearch.action.search.GetAllPitNodesRequest; +import org.opensearch.action.search.GetAllPitNodesResponse; +import org.opensearch.action.search.GetAllPitsAction; +import org.opensearch.client.node.NodeClient; +import org.opensearch.common.xcontent.XContentBuilder; +import org.opensearch.rest.BaseRestHandler; +import org.opensearch.rest.BytesRestResponse; +import org.opensearch.rest.RestRequest; +import org.opensearch.rest.RestResponse; +import org.opensearch.rest.RestStatus; +import org.opensearch.rest.action.RestResponseListener; + +import java.io.IOException; +import java.util.Collections; +import java.util.List; + +import static java.util.Collections.unmodifiableList; +import static org.opensearch.rest.RestRequest.Method.GET; + +/** + * Rest action for retrieving all active PIT IDs across all nodes + */ +public class RestGetAllPitsAction extends BaseRestHandler { + @Override + public String getName() { + return "get_all_pit_action"; + } + + @Override + protected RestChannelConsumer prepareRequest(RestRequest request, NodeClient client) throws IOException { + final ClusterStateRequest clusterStateRequest = new ClusterStateRequest(); + clusterStateRequest.local(false); + boolean includeAll = request.paramAsBoolean("include_all", false); + clusterStateRequest.clusterManagerNodeTimeout( + request.paramAsTime("cluster_manager_timeout", clusterStateRequest.clusterManagerNodeTimeout()) + ); + clusterStateRequest.clear().nodes(true).routingTable(true).indices("*"); + GetAllPitNodesRequest getAllPITNodesRequest = new GetAllPitNodesRequest(); + return channel -> client.execute( + GetAllPitsAction.INSTANCE, + getAllPITNodesRequest, + new RestResponseListener(channel) { + @Override + public RestResponse buildResponse(final GetAllPitNodesResponse getAllPITNodesResponse) throws Exception { + try (XContentBuilder builder = channel.newBuilder()) { + builder.startObject(); + if (getAllPITNodesResponse.hasFailures()) { + builder.startArray("failures"); + for (int idx = 0; idx < getAllPITNodesResponse.failures().size(); idx++) { + builder.field( + getAllPITNodesResponse.failures().get(idx).nodeId(), + getAllPITNodesResponse.failures().get(idx).getDetailedMessage() + ); + } + builder.endArray(); + } + builder.field("pits", getAllPITNodesResponse.getPitInfos()); + builder.endObject(); + if (getAllPITNodesResponse.getPitInfos().isEmpty()) return new BytesRestResponse(RestStatus.NOT_FOUND, builder); + return new BytesRestResponse(RestStatus.OK, builder); + } + } + } + ); + } + + @Override + public List routes() { + return unmodifiableList(Collections.singletonList(new Route(GET, "/_search/point_in_time/all"))); + } + +} From 8c97d73ab35b889070338adce2ceccea61e17238 Mon Sep 17 00:00:00 2001 From: Bharathwaj G Date: Fri, 2 Sep 2022 11:21:13 +0530 Subject: [PATCH 02/12] Adding rest tests Signed-off-by: Bharathwaj G --- CHANGELOG.md | 1 + .../opensearch/client/RequestConverters.java | 2 +- .../client/{Pit1IT.java => PitIT.java} | 4 +- .../rest-api-spec/api/get_all_pits.json | 19 +++++++ .../rest-api-spec/test/pit/10_basic.yml | 12 +++++ .../action/search/GetAllPitNodesResponse.java | 2 +- .../java/org/opensearch/client/Client.java | 4 ++ .../client/support/AbstractClient.java | 8 +++ .../action/cat/RestPitSegmentsAction.java | 15 +++--- .../action/search/RestGetAllPitsAction.java | 53 ++++++++----------- .../search/pit/RestGetAllPitsActionTests.java | 45 ++++++++++++++++ 11 files changed, 123 insertions(+), 42 deletions(-) rename client/rest-high-level/src/test/java/org/opensearch/client/{Pit1IT.java => PitIT.java} (97%) create mode 100644 rest-api-spec/src/main/resources/rest-api-spec/api/get_all_pits.json create mode 100644 server/src/test/java/org/opensearch/search/pit/RestGetAllPitsActionTests.java diff --git a/CHANGELOG.md b/CHANGELOG.md index f89e7eba0698c..f517092c02d04 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -5,6 +5,7 @@ Inspired from [Keep a Changelog](https://keepachangelog.com/en/1.0.0/) ### Added - Github workflow for changelog verification ([#4085](https://github.com/opensearch-project/OpenSearch/pull/4085)) - Point in time rest layer changes for create and delete PIT API ([#4064](https://github.com/opensearch-project/OpenSearch/pull/4064)) +- Point in time rest layer changes for list PIT and PIT segments API ([#4388](https://github.com/opensearch-project/OpenSearch/pull/4388)) - Added @dreamer-89 as an Opensearch maintainer ([#4342](https://github.com/opensearch-project/OpenSearch/pull/4342)) - Added release notes for 1.3.5 ([#4343](https://github.com/opensearch-project/OpenSearch/pull/4343)) - Added release notes for 2.2.1 ([#4344](https://github.com/opensearch-project/OpenSearch/pull/4344)) diff --git a/client/rest-high-level/src/main/java/org/opensearch/client/RequestConverters.java b/client/rest-high-level/src/main/java/org/opensearch/client/RequestConverters.java index 41391ed6f1c07..91c339cc92c1b 100644 --- a/client/rest-high-level/src/main/java/org/opensearch/client/RequestConverters.java +++ b/client/rest-high-level/src/main/java/org/opensearch/client/RequestConverters.java @@ -499,7 +499,7 @@ static Request deleteAllPits() { } static Request getAllPits() { - return new Request(HttpGet.METHOD_NAME, "/_search/point_in_time/all"); + return new Request(HttpGet.METHOD_NAME, "/_search/point_in_time/_all"); } static Request multiSearch(MultiSearchRequest multiSearchRequest) throws IOException { diff --git a/client/rest-high-level/src/test/java/org/opensearch/client/Pit1IT.java b/client/rest-high-level/src/test/java/org/opensearch/client/PitIT.java similarity index 97% rename from client/rest-high-level/src/test/java/org/opensearch/client/Pit1IT.java rename to client/rest-high-level/src/test/java/org/opensearch/client/PitIT.java index 240696a6cd65b..69bec31d632e9 100644 --- a/client/rest-high-level/src/test/java/org/opensearch/client/Pit1IT.java +++ b/client/rest-high-level/src/test/java/org/opensearch/client/PitIT.java @@ -30,7 +30,7 @@ /** * Tests point in time API with rest high level client */ -public class Pit1IT extends OpenSearchRestHighLevelClientTestCase { +public class PitIT extends OpenSearchRestHighLevelClientTestCase { @Before public void indexDocuments() throws IOException { @@ -124,7 +124,7 @@ public void onFailure(Exception e) { highLevelClient().getAllPitsAsync(RequestOptions.DEFAULT, getPitsListener); highLevelClient().deleteAllPitsAsync(RequestOptions.DEFAULT, deletePitListener); // validate no pits case - //highLevelClient().getAllPitsAsync(RequestOptions.DEFAULT, getPitsListener); + highLevelClient().getAllPitsAsync(RequestOptions.DEFAULT, getPitsListener); highLevelClient().deleteAllPitsAsync(RequestOptions.DEFAULT, deletePitListener); } } diff --git a/rest-api-spec/src/main/resources/rest-api-spec/api/get_all_pits.json b/rest-api-spec/src/main/resources/rest-api-spec/api/get_all_pits.json new file mode 100644 index 0000000000000..544a8cb11b002 --- /dev/null +++ b/rest-api-spec/src/main/resources/rest-api-spec/api/get_all_pits.json @@ -0,0 +1,19 @@ +{ + "get_all_pits":{ + "documentation":{ + "url":"https://opensearch.org/docs/latest/opensearch/rest-api/point_in_time/", + "description":"Lists all active point in time searches." + }, + "stability":"stable", + "url":{ + "paths":[ + { + "path":"/_search/point_in_time/_all", + "methods":[ + "GET" + ] + } + ] + } + } +} diff --git a/rest-api-spec/src/main/resources/rest-api-spec/test/pit/10_basic.yml b/rest-api-spec/src/main/resources/rest-api-spec/test/pit/10_basic.yml index 2023bcc8f5c87..cd0c5b9126a9d 100644 --- a/rest-api-spec/src/main/resources/rest-api-spec/test/pit/10_basic.yml +++ b/rest-api-spec/src/main/resources/rest-api-spec/test/pit/10_basic.yml @@ -79,6 +79,12 @@ - match: {hits.total: 3 } - length: {hits.hits: 1 } + - do: + get_all_pits: {} + + - match: {pits.0.pit_id: $pit_id} + - match: {pits.0.keep_alive: 82800000 } + - do: delete_pit: body: @@ -119,6 +125,12 @@ - set: {pit_id: pit_id} - match: { _shards.failed: 0} + - do: + get_all_pits: {} + + - match: {pits.0.pit_id: $pit_id} + - match: {pits.0.keep_alive: 82800000 } + - do: delete_all_pits: {} diff --git a/server/src/main/java/org/opensearch/action/search/GetAllPitNodesResponse.java b/server/src/main/java/org/opensearch/action/search/GetAllPitNodesResponse.java index be5421f019c8b..9543c5ae01297 100644 --- a/server/src/main/java/org/opensearch/action/search/GetAllPitNodesResponse.java +++ b/server/src/main/java/org/opensearch/action/search/GetAllPitNodesResponse.java @@ -65,7 +65,7 @@ public GetAllPitNodesResponse(List listPitInfos, GetAllPitNodesResp pitInfos.addAll(listPitInfos); } - public GetAllPitNodesResponse( + public GetAllPitNodesResponse( List listPitInfos, ClusterName clusterName, List getAllPitNodeResponse, diff --git a/server/src/main/java/org/opensearch/client/Client.java b/server/src/main/java/org/opensearch/client/Client.java index 94043d5c3c89f..d996cbfbe6e89 100644 --- a/server/src/main/java/org/opensearch/client/Client.java +++ b/server/src/main/java/org/opensearch/client/Client.java @@ -64,6 +64,8 @@ import org.opensearch.action.search.CreatePitResponse; import org.opensearch.action.search.DeletePitRequest; import org.opensearch.action.search.DeletePitResponse; +import org.opensearch.action.search.GetAllPitNodesRequest; +import org.opensearch.action.search.GetAllPitNodesResponse; import org.opensearch.action.search.MultiSearchRequest; import org.opensearch.action.search.MultiSearchRequestBuilder; import org.opensearch.action.search.MultiSearchResponse; @@ -341,6 +343,8 @@ public interface Client extends OpenSearchClient, Releasable { */ void deletePits(DeletePitRequest deletePITRequest, ActionListener listener); + void getAllPits(GetAllPitNodesRequest getAllPitNodesRequest, ActionListener listener); + /** * Get information of segments of one or more PITs */ diff --git a/server/src/main/java/org/opensearch/client/support/AbstractClient.java b/server/src/main/java/org/opensearch/client/support/AbstractClient.java index bc80a2ba92bf8..b20d5432d17b7 100644 --- a/server/src/main/java/org/opensearch/client/support/AbstractClient.java +++ b/server/src/main/java/org/opensearch/client/support/AbstractClient.java @@ -335,6 +335,9 @@ import org.opensearch.action.search.DeletePitAction; import org.opensearch.action.search.DeletePitRequest; import org.opensearch.action.search.DeletePitResponse; +import org.opensearch.action.search.GetAllPitNodesRequest; +import org.opensearch.action.search.GetAllPitNodesResponse; +import org.opensearch.action.search.GetAllPitsAction; import org.opensearch.action.search.MultiSearchAction; import org.opensearch.action.search.MultiSearchRequest; import org.opensearch.action.search.MultiSearchRequestBuilder; @@ -595,6 +598,11 @@ public void deletePits(final DeletePitRequest deletePITRequest, final ActionList execute(DeletePitAction.INSTANCE, deletePITRequest, listener); } + @Override + public void getAllPits(final GetAllPitNodesRequest getAllPitNodesRequest, final ActionListener listener) { + execute(GetAllPitsAction.INSTANCE, getAllPitNodesRequest, listener); + } + @Override public void pitSegments(final PitSegmentsRequest request, final ActionListener listener) { execute(PitSegmentsAction.INSTANCE, request, listener); diff --git a/server/src/main/java/org/opensearch/rest/action/cat/RestPitSegmentsAction.java b/server/src/main/java/org/opensearch/rest/action/cat/RestPitSegmentsAction.java index b9aaaf77b5cba..49c1357e2436c 100644 --- a/server/src/main/java/org/opensearch/rest/action/cat/RestPitSegmentsAction.java +++ b/server/src/main/java/org/opensearch/rest/action/cat/RestPitSegmentsAction.java @@ -20,6 +20,7 @@ import org.opensearch.cluster.node.DiscoveryNodes; import org.opensearch.common.Strings; import org.opensearch.common.Table; +import org.opensearch.common.logging.DeprecationLogger; import org.opensearch.index.engine.Segment; import org.opensearch.rest.BaseRestHandler; import org.opensearch.rest.RestHandler; @@ -28,8 +29,6 @@ import org.opensearch.rest.action.RestActionListener; import org.opensearch.rest.action.RestResponseListener; -import java.util.ArrayList; -import java.util.Arrays; import java.util.List; import java.util.Map; @@ -41,6 +40,9 @@ * Rest action for pit segments */ public class RestPitSegmentsAction extends AbstractCatAction { + + private static final DeprecationLogger deprecationLogger = DeprecationLogger.getLogger(RestPitSegmentsAction.class); + @Override public List routes() { return unmodifiableList(asList(new Route(GET, "/_cat/pit_segments"), new Route(GET, "/_cat/pit_segments/{pit_id}"))); @@ -61,14 +63,15 @@ protected BaseRestHandler.RestChannelConsumer doCatRequest(final RestRequest req final String[] pitIds = Strings.splitStringByCommaToArray(request.param("pit_id")); final ClusterStateRequest clusterStateRequest = new ClusterStateRequest(); clusterStateRequest.local(false); - clusterStateRequest.masterNodeTimeout(request.paramAsTime("master_timeout", clusterStateRequest.masterNodeTimeout())); + clusterStateRequest.clusterManagerNodeTimeout( + request.paramAsTime("cluster_manager_timeout", clusterStateRequest.clusterManagerNodeTimeout()) + ); + parseDeprecatedMasterTimeoutParameter(clusterStateRequest, request, deprecationLogger, getName()); clusterStateRequest.clear().nodes(true).routingTable(true).indices("*"); - return channel -> client.admin().cluster().state(clusterStateRequest, new RestActionListener<>(channel) { @Override public void processResponse(final ClusterStateResponse clusterStateResponse) { - final PitSegmentsRequest pitSegmentsRequest = new PitSegmentsRequest(); - pitSegmentsRequest.clearAndSetPitIds(new ArrayList<>(Arrays.asList(pitIds))); + final PitSegmentsRequest pitSegmentsRequest = new PitSegmentsRequest(pitIds); client.execute(PitSegmentsAction.INSTANCE, pitSegmentsRequest, new RestResponseListener<>(channel) { @Override public RestResponse buildResponse(final IndicesSegmentResponse indicesSegmentResponse) throws Exception { diff --git a/server/src/main/java/org/opensearch/rest/action/search/RestGetAllPitsAction.java b/server/src/main/java/org/opensearch/rest/action/search/RestGetAllPitsAction.java index 3ef3a20ffc9a7..687ecddc1021d 100644 --- a/server/src/main/java/org/opensearch/rest/action/search/RestGetAllPitsAction.java +++ b/server/src/main/java/org/opensearch/rest/action/search/RestGetAllPitsAction.java @@ -9,10 +9,8 @@ package org.opensearch.rest.action.search; -import org.opensearch.action.admin.cluster.state.ClusterStateRequest; import org.opensearch.action.search.GetAllPitNodesRequest; import org.opensearch.action.search.GetAllPitNodesResponse; -import org.opensearch.action.search.GetAllPitsAction; import org.opensearch.client.node.NodeClient; import org.opensearch.common.xcontent.XContentBuilder; import org.opensearch.rest.BaseRestHandler; @@ -40,45 +38,36 @@ public String getName() { @Override protected RestChannelConsumer prepareRequest(RestRequest request, NodeClient client) throws IOException { - final ClusterStateRequest clusterStateRequest = new ClusterStateRequest(); - clusterStateRequest.local(false); - boolean includeAll = request.paramAsBoolean("include_all", false); - clusterStateRequest.clusterManagerNodeTimeout( - request.paramAsTime("cluster_manager_timeout", clusterStateRequest.clusterManagerNodeTimeout()) - ); - clusterStateRequest.clear().nodes(true).routingTable(true).indices("*"); GetAllPitNodesRequest getAllPITNodesRequest = new GetAllPitNodesRequest(); - return channel -> client.execute( - GetAllPitsAction.INSTANCE, - getAllPITNodesRequest, - new RestResponseListener(channel) { - @Override - public RestResponse buildResponse(final GetAllPitNodesResponse getAllPITNodesResponse) throws Exception { - try (XContentBuilder builder = channel.newBuilder()) { - builder.startObject(); - if (getAllPITNodesResponse.hasFailures()) { - builder.startArray("failures"); - for (int idx = 0; idx < getAllPITNodesResponse.failures().size(); idx++) { - builder.field( - getAllPITNodesResponse.failures().get(idx).nodeId(), - getAllPITNodesResponse.failures().get(idx).getDetailedMessage() - ); - } - builder.endArray(); + return channel -> client.getAllPits(getAllPITNodesRequest, new RestResponseListener(channel) { + @Override + public RestResponse buildResponse(final GetAllPitNodesResponse getAllPITNodesResponse) throws Exception { + try (XContentBuilder builder = channel.newBuilder()) { + builder.startObject(); + if (getAllPITNodesResponse.hasFailures()) { + builder.startArray("failures"); + for (int idx = 0; idx < getAllPITNodesResponse.failures().size(); idx++) { + builder.field( + getAllPITNodesResponse.failures().get(idx).nodeId(), + getAllPITNodesResponse.failures().get(idx).getDetailedMessage() + ); } - builder.field("pits", getAllPITNodesResponse.getPitInfos()); - builder.endObject(); - if (getAllPITNodesResponse.getPitInfos().isEmpty()) return new BytesRestResponse(RestStatus.NOT_FOUND, builder); - return new BytesRestResponse(RestStatus.OK, builder); + builder.endArray(); } + builder.field("pits", getAllPITNodesResponse.getPitInfos()); + builder.endObject(); + if (getAllPITNodesResponse.getPitInfos().isEmpty()) { + return new BytesRestResponse(RestStatus.NOT_FOUND, builder); + } + return new BytesRestResponse(RestStatus.OK, builder); } } - ); + }); } @Override public List routes() { - return unmodifiableList(Collections.singletonList(new Route(GET, "/_search/point_in_time/all"))); + return unmodifiableList(Collections.singletonList(new Route(GET, "/_search/point_in_time/_all"))); } } diff --git a/server/src/test/java/org/opensearch/search/pit/RestGetAllPitsActionTests.java b/server/src/test/java/org/opensearch/search/pit/RestGetAllPitsActionTests.java new file mode 100644 index 0000000000000..9d6c5af1f2715 --- /dev/null +++ b/server/src/test/java/org/opensearch/search/pit/RestGetAllPitsActionTests.java @@ -0,0 +1,45 @@ +/* + * SPDX-License-Identifier: Apache-2.0 + * + * The OpenSearch Contributors require contributions made to + * this file be licensed under the Apache-2.0 license or a + * compatible open source license. + */ + +package org.opensearch.search.pit; + +import org.apache.lucene.util.SetOnce; +import org.opensearch.action.ActionListener; +import org.opensearch.action.search.GetAllPitNodesRequest; +import org.opensearch.action.search.GetAllPitNodesResponse; +import org.opensearch.client.node.NodeClient; +import org.opensearch.rest.RestRequest; +import org.opensearch.rest.action.search.RestGetAllPitsAction; +import org.opensearch.test.OpenSearchTestCase; +import org.opensearch.test.client.NoOpNodeClient; +import org.opensearch.test.rest.FakeRestChannel; +import org.opensearch.test.rest.FakeRestRequest; + +import static org.hamcrest.Matchers.equalTo; + +/** + * Tests for rest get all PITs action + */ +public class RestGetAllPitsActionTests extends OpenSearchTestCase { + + public void testGetAllPits() throws Exception { + SetOnce pitCalled = new SetOnce<>(); + try (NodeClient nodeClient = new NoOpNodeClient(this.getTestName()) { + @Override + public void getAllPits(GetAllPitNodesRequest request, ActionListener listener) { + pitCalled.set(true); + } + }) { + RestGetAllPitsAction action = new RestGetAllPitsAction(); + RestRequest request = new FakeRestRequest.Builder(xContentRegistry()).withPath("/_all").build(); + FakeRestChannel channel = new FakeRestChannel(request, false, 0); + action.handleRequest(request, channel, nodeClient); + assertThat(pitCalled.get(), equalTo(true)); + } + } +} From 5167a2c78dff07719677ea4ce0fcb78dedd443b6 Mon Sep 17 00:00:00 2001 From: Bharathwaj G Date: Fri, 2 Sep 2022 13:18:30 +0530 Subject: [PATCH 03/12] Fixes for security Signed-off-by: Bharathwaj G --- .../indices/segments/PitSegmentsRequest.java | 34 +++++++++++++++++++ .../segments/TransportPitSegmentsAction.java | 6 +++- .../action/cat/RestPitSegmentsAction.java | 24 ++++++++++--- 3 files changed, 58 insertions(+), 6 deletions(-) diff --git a/server/src/main/java/org/opensearch/action/admin/indices/segments/PitSegmentsRequest.java b/server/src/main/java/org/opensearch/action/admin/indices/segments/PitSegmentsRequest.java index 84f5e5ad6a1e8..c26ce48a210b4 100644 --- a/server/src/main/java/org/opensearch/action/admin/indices/segments/PitSegmentsRequest.java +++ b/server/src/main/java/org/opensearch/action/admin/indices/segments/PitSegmentsRequest.java @@ -13,6 +13,7 @@ import org.opensearch.common.Strings; import org.opensearch.common.io.stream.StreamInput; import org.opensearch.common.io.stream.StreamOutput; +import org.opensearch.common.xcontent.XContentParser; import java.io.IOException; import java.util.ArrayList; @@ -84,4 +85,37 @@ public ActionRequestValidationException validate() { } return validationException; } + + public void fromXContent(XContentParser parser) throws IOException { + pitIds.clear(); + if (parser.nextToken() != XContentParser.Token.START_OBJECT) { + throw new IllegalArgumentException("Malformed content, must start with an object"); + } else { + XContentParser.Token token; + String currentFieldName = null; + while ((token = parser.nextToken()) != XContentParser.Token.END_OBJECT) { + if (token == XContentParser.Token.FIELD_NAME) { + currentFieldName = parser.currentName(); + } else if ("pit_id".equals(currentFieldName)) { + if (token == XContentParser.Token.START_ARRAY) { + while ((token = parser.nextToken()) != XContentParser.Token.END_ARRAY) { + if (token.isValue() == false) { + throw new IllegalArgumentException("pit_id array element should only contain pit_id"); + } + pitIds.add(parser.text()); + } + } else { + if (token.isValue() == false) { + throw new IllegalArgumentException("pit_id element should only contain pit_id"); + } + pitIds.add(parser.text()); + } + } else { + throw new IllegalArgumentException( + "Unknown parameter [" + currentFieldName + "] in request body or parameter is of the wrong type[" + token + "] " + ); + } + } + } + } } diff --git a/server/src/main/java/org/opensearch/action/admin/indices/segments/TransportPitSegmentsAction.java b/server/src/main/java/org/opensearch/action/admin/indices/segments/TransportPitSegmentsAction.java index 9d4ece74a7270..6a4b4b6b4019a 100644 --- a/server/src/main/java/org/opensearch/action/admin/indices/segments/TransportPitSegmentsAction.java +++ b/server/src/main/java/org/opensearch/action/admin/indices/segments/TransportPitSegmentsAction.java @@ -95,7 +95,11 @@ public TransportPitSegmentsAction( @Override protected void doExecute(Task task, PitSegmentsRequest request, ActionListener listener) { List pitIds = request.getPitIds(); - if (pitIds.size() == 1 && "_all".equals(pitIds.get(0))) { + // when security plugin intercepts the request, if PITs are not present in the cluster the PIT IDs in request will be empty + // and in this case return empty response + if (pitIds.isEmpty()) { + listener.onResponse(new IndicesSegmentResponse(new ShardSegments[] {}, 0, 0, 0, new ArrayList<>())); + } else if (pitIds.size() == 1 && "_all".equals(pitIds.get(0))) { pitService.getAllPits(ActionListener.wrap(response -> { request.clearAndSetPitIds(response.getPitInfos().stream().map(ListPitInfo::getPitId).collect(Collectors.toList())); super.doExecute(task, request, listener); diff --git a/server/src/main/java/org/opensearch/rest/action/cat/RestPitSegmentsAction.java b/server/src/main/java/org/opensearch/rest/action/cat/RestPitSegmentsAction.java index 49c1357e2436c..7075ea13d4f27 100644 --- a/server/src/main/java/org/opensearch/rest/action/cat/RestPitSegmentsAction.java +++ b/server/src/main/java/org/opensearch/rest/action/cat/RestPitSegmentsAction.java @@ -18,7 +18,6 @@ import org.opensearch.action.admin.indices.segments.ShardSegments; import org.opensearch.client.node.NodeClient; import org.opensearch.cluster.node.DiscoveryNodes; -import org.opensearch.common.Strings; import org.opensearch.common.Table; import org.opensearch.common.logging.DeprecationLogger; import org.opensearch.index.engine.Segment; @@ -29,6 +28,7 @@ import org.opensearch.rest.action.RestActionListener; import org.opensearch.rest.action.RestResponseListener; +import java.io.IOException; import java.util.List; import java.util.Map; @@ -45,7 +45,7 @@ public class RestPitSegmentsAction extends AbstractCatAction { @Override public List routes() { - return unmodifiableList(asList(new Route(GET, "/_cat/pit_segments"), new Route(GET, "/_cat/pit_segments/{pit_id}"))); + return unmodifiableList(asList(new Route(GET, "/_cat/pit_segments/_all"), new Route(GET, "/_cat/pit_segments"))); } @Override @@ -60,7 +60,6 @@ public boolean allowSystemIndexAccessByDefault() { @Override protected BaseRestHandler.RestChannelConsumer doCatRequest(final RestRequest request, final NodeClient client) { - final String[] pitIds = Strings.splitStringByCommaToArray(request.param("pit_id")); final ClusterStateRequest clusterStateRequest = new ClusterStateRequest(); clusterStateRequest.local(false); clusterStateRequest.clusterManagerNodeTimeout( @@ -70,8 +69,23 @@ protected BaseRestHandler.RestChannelConsumer doCatRequest(final RestRequest req clusterStateRequest.clear().nodes(true).routingTable(true).indices("*"); return channel -> client.admin().cluster().state(clusterStateRequest, new RestActionListener<>(channel) { @Override - public void processResponse(final ClusterStateResponse clusterStateResponse) { - final PitSegmentsRequest pitSegmentsRequest = new PitSegmentsRequest(pitIds); + public void processResponse(final ClusterStateResponse clusterStateResponse) throws IOException { + String allPitIdsQualifier = "_all"; + final PitSegmentsRequest pitSegmentsRequest; + if (request.path().contains(allPitIdsQualifier)) { + pitSegmentsRequest = new PitSegmentsRequest(allPitIdsQualifier); + } else { + pitSegmentsRequest = new PitSegmentsRequest(); + request.withContentOrSourceParamParserOrNull((xContentParser -> { + if (xContentParser != null) { + try { + pitSegmentsRequest.fromXContent(xContentParser); + } catch (IOException e) { + throw new IllegalArgumentException("Failed to parse request body", e); + } + } + })); + } client.execute(PitSegmentsAction.INSTANCE, pitSegmentsRequest, new RestResponseListener<>(channel) { @Override public RestResponse buildResponse(final IndicesSegmentResponse indicesSegmentResponse) throws Exception { From 13b5f4cbbdcbc7bc82ca2bf1a176b8cdf5cf589c Mon Sep 17 00:00:00 2001 From: Bharathwaj G Date: Fri, 2 Sep 2022 20:40:31 +0530 Subject: [PATCH 04/12] Fixing build and addressing comment Signed-off-by: Bharathwaj G --- .../client/RestHighLevelClient.java | 22 +++++++++---------- .../client/RestHighLevelClientTests.java | 1 + .../action/search/GetAllPitNodesResponse.java | 6 ++--- 3 files changed, 15 insertions(+), 14 deletions(-) diff --git a/client/rest-high-level/src/main/java/org/opensearch/client/RestHighLevelClient.java b/client/rest-high-level/src/main/java/org/opensearch/client/RestHighLevelClient.java index b054c4f46365f..0a5880b778942 100644 --- a/client/rest-high-level/src/main/java/org/opensearch/client/RestHighLevelClient.java +++ b/client/rest-high-level/src/main/java/org/opensearch/client/RestHighLevelClient.java @@ -1377,11 +1377,11 @@ public final Cancellable deleteAllPitsAsync(RequestOptions options, ActionListen */ public final GetAllPitNodesResponse getAllPits(RequestOptions options) throws IOException { return performRequestAndParseEntity( - new MainRequest(), - (request) -> RequestConverters.getAllPits(), - options, - GetAllPitNodesResponse::fromXContent, - emptySet() + new MainRequest(), + (request) -> RequestConverters.getAllPits(), + options, + GetAllPitNodesResponse::fromXContent, + emptySet() ); } @@ -1394,12 +1394,12 @@ public final GetAllPitNodesResponse getAllPits(RequestOptions options) throws IO */ public final Cancellable getAllPitsAsync(RequestOptions options, ActionListener listener) { return performRequestAsyncAndParseEntity( - new MainRequest(), - (request) -> RequestConverters.getAllPits(), - options, - GetAllPitNodesResponse::fromXContent, - listener, - emptySet() + new MainRequest(), + (request) -> RequestConverters.getAllPits(), + options, + GetAllPitNodesResponse::fromXContent, + listener, + emptySet() ); } diff --git a/client/rest-high-level/src/test/java/org/opensearch/client/RestHighLevelClientTests.java b/client/rest-high-level/src/test/java/org/opensearch/client/RestHighLevelClientTests.java index cdd63743f2644..ad8da7244eae0 100644 --- a/client/rest-high-level/src/test/java/org/opensearch/client/RestHighLevelClientTests.java +++ b/client/rest-high-level/src/test/java/org/opensearch/client/RestHighLevelClientTests.java @@ -135,6 +135,7 @@ public class RestHighLevelClientTests extends OpenSearchTestCase { "ping", "info", "delete_all_pits", + "get_all_pits", // security "security.get_ssl_certificates", "security.authenticate", diff --git a/server/src/main/java/org/opensearch/action/search/GetAllPitNodesResponse.java b/server/src/main/java/org/opensearch/action/search/GetAllPitNodesResponse.java index 9543c5ae01297..e293ff3309e9a 100644 --- a/server/src/main/java/org/opensearch/action/search/GetAllPitNodesResponse.java +++ b/server/src/main/java/org/opensearch/action/search/GetAllPitNodesResponse.java @@ -45,13 +45,13 @@ public GetAllPitNodesResponse(StreamInput in) throws IOException { public GetAllPitNodesResponse( ClusterName clusterName, - List getAllPitNodeResponse, + List getAllPitNodeResponseList, List failures ) { - super(clusterName, getAllPitNodeResponse, failures); + super(clusterName, getAllPitNodeResponseList, failures); Set uniquePitIds = new HashSet<>(); pitInfos.addAll( - getAllPitNodeResponse.stream() + getAllPitNodeResponseList.stream() .flatMap(p -> p.getPitInfos().stream().filter(t -> uniquePitIds.add(t.getPitId()))) .collect(Collectors.toList()) ); From 660b3d563fdc67062e3009ab5bc27a6ce4172244 Mon Sep 17 00:00:00 2001 From: Bharathwaj G Date: Mon, 5 Sep 2022 13:02:51 +0530 Subject: [PATCH 05/12] Addressing comment Signed-off-by: Bharathwaj G --- .../org/opensearch/action/search/ListPitInfo.java | 11 +++++------ .../rest/action/search/RestGetAllPitsAction.java | 4 +++- 2 files changed, 8 insertions(+), 7 deletions(-) diff --git a/server/src/main/java/org/opensearch/action/search/ListPitInfo.java b/server/src/main/java/org/opensearch/action/search/ListPitInfo.java index 25377b8d466d8..92a730b2b0e69 100644 --- a/server/src/main/java/org/opensearch/action/search/ListPitInfo.java +++ b/server/src/main/java/org/opensearch/action/search/ListPitInfo.java @@ -61,15 +61,14 @@ public void writeTo(StreamOutput out) throws IOException { args -> new ListPitInfo((String) args[0], (long) args[1], (long) args[2]) ); - static { - PARSER.declareString(constructorArg(), new ParseField("pit_id")); - PARSER.declareLong(constructorArg(), new ParseField("creation_time")); - PARSER.declareLong(constructorArg(), new ParseField("keep_alive")); - } - private static final ParseField CREATION_TIME = new ParseField("creation_time"); private static final ParseField PIT_ID = new ParseField("pit_id"); private static final ParseField KEEP_ALIVE = new ParseField("keep_alive"); + static { + PARSER.declareString(constructorArg(), new ParseField(PIT_ID.getPreferredName())); + PARSER.declareLong(constructorArg(), new ParseField(CREATION_TIME.getPreferredName())); + PARSER.declareLong(constructorArg(), new ParseField(KEEP_ALIVE.getPreferredName())); + } @Override public XContentBuilder toXContent(XContentBuilder builder, Params params) throws IOException { diff --git a/server/src/main/java/org/opensearch/rest/action/search/RestGetAllPitsAction.java b/server/src/main/java/org/opensearch/rest/action/search/RestGetAllPitsAction.java index 687ecddc1021d..a69c3b0bdba19 100644 --- a/server/src/main/java/org/opensearch/rest/action/search/RestGetAllPitsAction.java +++ b/server/src/main/java/org/opensearch/rest/action/search/RestGetAllPitsAction.java @@ -56,7 +56,9 @@ public RestResponse buildResponse(final GetAllPitNodesResponse getAllPITNodesRes } builder.field("pits", getAllPITNodesResponse.getPitInfos()); builder.endObject(); - if (getAllPITNodesResponse.getPitInfos().isEmpty()) { + if (getAllPITNodesResponse.hasFailures()) { + return new BytesRestResponse(RestStatus.INTERNAL_SERVER_ERROR, builder); + } else if (getAllPITNodesResponse.getPitInfos().isEmpty()) { return new BytesRestResponse(RestStatus.NOT_FOUND, builder); } return new BytesRestResponse(RestStatus.OK, builder); From bf4c52d3953e509924df3881bbb0cec106cdb937 Mon Sep 17 00:00:00 2001 From: Bharathwaj G Date: Mon, 5 Sep 2022 15:56:30 +0530 Subject: [PATCH 06/12] Fixes for cross cluster Signed-off-by: Bharathwaj G --- .../opensearch/action/search/CreatePitController.java | 11 ++++++++++- .../java/org/opensearch/action/search/PitService.java | 8 ++++++-- 2 files changed, 16 insertions(+), 3 deletions(-) diff --git a/server/src/main/java/org/opensearch/action/search/CreatePitController.java b/server/src/main/java/org/opensearch/action/search/CreatePitController.java index f64dd3d7efae6..3d7a92ec1ed06 100644 --- a/server/src/main/java/org/opensearch/action/search/CreatePitController.java +++ b/server/src/main/java/org/opensearch/action/search/CreatePitController.java @@ -98,6 +98,11 @@ public void executeCreatePit( task.getParentTaskId(), Collections.emptyMap() ); + /** + * This is needed for cross cluster functionality to work with PITs and current ccsMinimizeRoundTrips is + * not supported for point in time + */ + searchRequest.setCcsMinimizeRoundtrips(false); /** * Phase 1 of create PIT */ @@ -193,6 +198,9 @@ void executeUpdatePitId( ); for (Map.Entry entry : contextId.shards().entrySet()) { DiscoveryNode node = nodelookup.apply(entry.getValue().getClusterAlias(), entry.getValue().getNode()); + if (node == null) { + node = this.clusterService.state().getNodes().get(entry.getValue().getNode()); + } try { final Transport.Connection connection = searchTransportService.getConnection(entry.getValue().getClusterAlias(), node); searchTransportService.updatePitContext( @@ -206,11 +214,12 @@ void executeUpdatePitId( groupedActionListener ); } catch (Exception e) { + String nodeName = node.getName(); logger.error( () -> new ParameterizedMessage( "Create pit update phase failed for PIT ID [{}] on node [{}]", searchResponse.pointInTimeId(), - node + nodeName ), e ); diff --git a/server/src/main/java/org/opensearch/action/search/PitService.java b/server/src/main/java/org/opensearch/action/search/PitService.java index ff068397ad94e..17a00916401fe 100644 --- a/server/src/main/java/org/opensearch/action/search/PitService.java +++ b/server/src/main/java/org/opensearch/action/search/PitService.java @@ -93,7 +93,10 @@ public void deletePitContexts( for (Map.Entry> entry : nodeToContextsMap.entrySet()) { String clusterAlias = entry.getValue().get(0).getSearchContextIdForNode().getClusterAlias(); - final DiscoveryNode node = nodeLookup.apply(clusterAlias, entry.getValue().get(0).getSearchContextIdForNode().getNode()); + DiscoveryNode node = nodeLookup.apply(clusterAlias, entry.getValue().get(0).getSearchContextIdForNode().getNode()); + if (node == null) { + node = this.clusterService.state().getNodes().get(entry.getValue().get(0).getSearchContextIdForNode().getNode()); + } if (node == null) { logger.error( () -> new ParameterizedMessage("node [{}] not found", entry.getValue().get(0).getSearchContextIdForNode().getNode()) @@ -108,7 +111,8 @@ public void deletePitContexts( final Transport.Connection connection = searchTransportService.getConnection(clusterAlias, node); searchTransportService.sendFreePITContexts(connection, entry.getValue(), groupedListener); } catch (Exception e) { - logger.error(() -> new ParameterizedMessage("Delete PITs failed on node [{}]", node.getName()), e); + String nodeName = node.getName(); + logger.error(() -> new ParameterizedMessage("Delete PITs failed on node [{}]", nodeName), e); List deletePitInfos = new ArrayList<>(); for (PitSearchContextIdForNode pitSearchContextIdForNode : entry.getValue()) { deletePitInfos.add(new DeletePitInfo(false, pitSearchContextIdForNode.getPitId())); From ba70d3e87a9e66aa001553de69c619533c39bf85 Mon Sep 17 00:00:00 2001 From: Bharathwaj G Date: Tue, 6 Sep 2022 16:10:59 +0530 Subject: [PATCH 07/12] Security model changes Signed-off-by: Bharathwaj G --- .../java/org/opensearch/client/PitIT.java | 4 +- .../org/opensearch/action/ActionModule.java | 6 +- .../segments/TransportPitSegmentsAction.java | 7 - .../opensearch/action/search/PitService.java | 9 +- .../search/TransportDeletePitAction.java | 29 -- .../search/TransportGetAllPitsAction.java | 2 - .../TransportNodesGetAllPitsAction.java | 21 +- .../java/org/opensearch/client/Client.java | 4 - .../opensearch/client/ClusterAdminClient.java | 4 + .../client/support/AbstractClient.java | 12 +- .../action/cat/RestPitSegmentsAction.java | 105 +++--- .../action/search/RestDeletePitAction.java | 61 +++- .../action/search/RestGetAllPitsAction.java | 40 ++- .../search/TransportDeletePitActionTests.java | 314 ------------------ .../search/pit/RestDeletePitActionTests.java | 70 +++- .../search/pit/RestGetAllPitsActionTests.java | 47 ++- 16 files changed, 282 insertions(+), 453 deletions(-) diff --git a/client/rest-high-level/src/test/java/org/opensearch/client/PitIT.java b/client/rest-high-level/src/test/java/org/opensearch/client/PitIT.java index 69bec31d632e9..7f81bf393d619 100644 --- a/client/rest-high-level/src/test/java/org/opensearch/client/PitIT.java +++ b/client/rest-high-level/src/test/java/org/opensearch/client/PitIT.java @@ -124,7 +124,9 @@ public void onFailure(Exception e) { highLevelClient().getAllPitsAsync(RequestOptions.DEFAULT, getPitsListener); highLevelClient().deleteAllPitsAsync(RequestOptions.DEFAULT, deletePitListener); // validate no pits case - highLevelClient().getAllPitsAsync(RequestOptions.DEFAULT, getPitsListener); + getAllPitResponse = highLevelClient().getAllPits(RequestOptions.DEFAULT); + assertTrue(getAllPitResponse.getPitInfos().size() == 0); + // highLevelClient().getAllPitsAsync(RequestOptions.DEFAULT, getPitsListener); highLevelClient().deleteAllPitsAsync(RequestOptions.DEFAULT, deletePitListener); } } diff --git a/server/src/main/java/org/opensearch/action/ActionModule.java b/server/src/main/java/org/opensearch/action/ActionModule.java index 7dbc222d42072..554b006a76b61 100644 --- a/server/src/main/java/org/opensearch/action/ActionModule.java +++ b/server/src/main/java/org/opensearch/action/ActionModule.java @@ -859,9 +859,9 @@ public void initRestHandlers(Supplier nodesInCluster) { // Point in time API registerHandler.accept(new RestCreatePitAction()); - registerHandler.accept(new RestDeletePitAction()); - registerHandler.accept(new RestGetAllPitsAction()); - registerHandler.accept(new RestPitSegmentsAction()); + registerHandler.accept(new RestDeletePitAction(nodesInCluster)); + registerHandler.accept(new RestGetAllPitsAction(nodesInCluster)); + registerHandler.accept(new RestPitSegmentsAction(nodesInCluster)); for (ActionPlugin plugin : actionPlugins) { for (RestHandler handler : plugin.getRestHandlers( diff --git a/server/src/main/java/org/opensearch/action/admin/indices/segments/TransportPitSegmentsAction.java b/server/src/main/java/org/opensearch/action/admin/indices/segments/TransportPitSegmentsAction.java index 6a4b4b6b4019a..b1a43aa73cd97 100644 --- a/server/src/main/java/org/opensearch/action/admin/indices/segments/TransportPitSegmentsAction.java +++ b/server/src/main/java/org/opensearch/action/admin/indices/segments/TransportPitSegmentsAction.java @@ -8,7 +8,6 @@ package org.opensearch.action.admin.indices.segments; import org.opensearch.action.ActionListener; -import org.opensearch.action.search.ListPitInfo; import org.opensearch.action.search.PitService; import org.opensearch.action.search.SearchContextId; import org.opensearch.action.search.SearchContextIdForNode; @@ -46,7 +45,6 @@ import java.util.Collections; import java.util.List; import java.util.Map; -import java.util.stream.Collectors; import static org.opensearch.action.search.SearchContextId.decode; @@ -99,11 +97,6 @@ protected void doExecute(Task task, PitSegmentsRequest request, ActionListener())); - } else if (pitIds.size() == 1 && "_all".equals(pitIds.get(0))) { - pitService.getAllPits(ActionListener.wrap(response -> { - request.clearAndSetPitIds(response.getPitInfos().stream().map(ListPitInfo::getPitId).collect(Collectors.toList())); - super.doExecute(task, request, listener); - }, listener::onFailure)); } else { super.doExecute(task, request, listener); } diff --git a/server/src/main/java/org/opensearch/action/search/PitService.java b/server/src/main/java/org/opensearch/action/search/PitService.java index 17a00916401fe..984424e00276e 100644 --- a/server/src/main/java/org/opensearch/action/search/PitService.java +++ b/server/src/main/java/org/opensearch/action/search/PitService.java @@ -170,17 +170,20 @@ public Map getIndicesForPits(List pitIds) { /** * Get all active point in time contexts */ - public void getAllPits(ActionListener getAllPitsListener) { + public void getAllPits(GetAllPitNodesResponse getAllPitNodesResponse, ActionListener getAllPitsListener) { final List nodes = new ArrayList<>(); for (ObjectCursor cursor : clusterService.state().nodes().getDataNodes().values()) { DiscoveryNode node = cursor.value; nodes.add(node); } + logger.debug("Number of active PTIs in cluster: " + getAllPitNodesResponse.getPitInfos().size()); DiscoveryNode[] disNodesArr = nodes.toArray(new DiscoveryNode[nodes.size()]); + GetAllPitNodesRequest getAllPitNodesRequest = new GetAllPitNodesRequest(disNodesArr); + getAllPitNodesRequest.setGetAllPitNodesResponse(getAllPitNodesResponse); transportService.sendRequest( transportService.getLocalNode(), - NodesGetAllPitsAction.NAME, - new GetAllPitNodesRequest(disNodesArr), + GetAllPitsAction.NAME, + getAllPitNodesRequest, new TransportResponseHandler() { @Override diff --git a/server/src/main/java/org/opensearch/action/search/TransportDeletePitAction.java b/server/src/main/java/org/opensearch/action/search/TransportDeletePitAction.java index 19abe2361290d..f5ae7d79529c8 100644 --- a/server/src/main/java/org/opensearch/action/search/TransportDeletePitAction.java +++ b/server/src/main/java/org/opensearch/action/search/TransportDeletePitAction.java @@ -21,16 +21,12 @@ import java.util.HashMap; import java.util.List; import java.util.Map; -import java.util.stream.Collectors; /** * Transport action for deleting point in time searches - supports deleting list and all point in time searches */ public class TransportDeletePitAction extends HandledTransportAction { private final NamedWriteableRegistry namedWriteableRegistry; - private TransportSearchAction transportSearchAction; - private final ClusterService clusterService; - private final SearchTransportService searchTransportService; private final PitService pitService; @Inject @@ -45,9 +41,6 @@ public TransportDeletePitAction( ) { super(DeletePitAction.NAME, transportService, actionFilters, DeletePitRequest::new); this.namedWriteableRegistry = namedWriteableRegistry; - this.transportSearchAction = transportSearchAction; - this.clusterService = clusterService; - this.searchTransportService = searchTransportService; this.pitService = pitService; } @@ -61,8 +54,6 @@ protected void doExecute(Task task, DeletePitRequest request, ActionListener())); - } else if (pitIds.size() == 1 && "_all".equals(pitIds.get(0))) { - deleteAllPits(listener); } else { deletePits(listener, request); } @@ -84,24 +75,4 @@ private void deletePits(ActionListener listener, DeletePitReq } pitService.deletePitContexts(nodeToContextsMap, listener); } - - /** - * Delete all active PIT reader contexts leveraging list all PITs - * - * For Cross cluster PITs : - * - mixed cluster PITs ( PIT comprising local and remote ) will be fully deleted. Since there will atleast be - * one reader context with PIT ID present in local cluster, 'Get all PITs' will retrieve the PIT ID with which - * we can completely delete the PIT contexts in both local and remote cluster. - * - fully remote PITs will not be deleted as 'Get all PITs' operates on local cluster only and no PIT info can - * be retrieved when it's fully remote. - */ - private void deleteAllPits(ActionListener listener) { - // Get all PITs and execute delete operation for the PITs. - pitService.getAllPits(ActionListener.wrap(getAllPitNodesResponse -> { - DeletePitRequest deletePitRequest = new DeletePitRequest( - getAllPitNodesResponse.getPitInfos().stream().map(r -> r.getPitId()).collect(Collectors.toList()) - ); - deletePits(listener, deletePitRequest); - }, listener::onFailure)); - } } diff --git a/server/src/main/java/org/opensearch/action/search/TransportGetAllPitsAction.java b/server/src/main/java/org/opensearch/action/search/TransportGetAllPitsAction.java index c8529c5b02bd4..893763f6a3e01 100644 --- a/server/src/main/java/org/opensearch/action/search/TransportGetAllPitsAction.java +++ b/server/src/main/java/org/opensearch/action/search/TransportGetAllPitsAction.java @@ -31,8 +31,6 @@ protected void doExecute(Task task, GetAllPitNodesRequest request, ActionListene // If security plugin intercepts the request, it'll replace all PIT IDs with permitted PIT IDs if (request.getGetAllPitNodesResponse() != null) { listener.onResponse(request.getGetAllPitNodesResponse()); - } else { - pitService.getAllPits(listener); } } } diff --git a/server/src/main/java/org/opensearch/action/search/TransportNodesGetAllPitsAction.java b/server/src/main/java/org/opensearch/action/search/TransportNodesGetAllPitsAction.java index 520830cd293f0..7eee946dd79ea 100644 --- a/server/src/main/java/org/opensearch/action/search/TransportNodesGetAllPitsAction.java +++ b/server/src/main/java/org/opensearch/action/search/TransportNodesGetAllPitsAction.java @@ -8,6 +8,7 @@ package org.opensearch.action.search; +import org.opensearch.action.ActionListener; import org.opensearch.action.FailedNodeException; import org.opensearch.action.support.ActionFilters; import org.opensearch.action.support.nodes.TransportNodesAction; @@ -15,6 +16,7 @@ import org.opensearch.common.inject.Inject; import org.opensearch.common.io.stream.StreamInput; import org.opensearch.search.SearchService; +import org.opensearch.tasks.Task; import org.opensearch.threadpool.ThreadPool; import org.opensearch.transport.TransportService; @@ -31,13 +33,16 @@ public class TransportNodesGetAllPitsAction extends TransportNodesAction< GetAllPitNodeResponse> { private final SearchService searchService; + private final PitService pitService; + @Inject public TransportNodesGetAllPitsAction( ThreadPool threadPool, ClusterService clusterService, TransportService transportService, ActionFilters actionFilters, - SearchService searchService + SearchService searchService, + PitService pitService ) { super( NodesGetAllPitsAction.NAME, @@ -51,15 +56,16 @@ public TransportNodesGetAllPitsAction( GetAllPitNodeResponse.class ); this.searchService = searchService; + this.pitService = pitService; } @Override protected GetAllPitNodesResponse newResponse( GetAllPitNodesRequest request, - List getAllPitNodeRespons, + List getAllPitNodeResponses, List failures ) { - return new GetAllPitNodesResponse(clusterService.getClusterName(), getAllPitNodeRespons, failures); + return new GetAllPitNodesResponse(clusterService.getClusterName(), getAllPitNodeResponses, failures); } @Override @@ -83,4 +89,13 @@ protected GetAllPitNodeResponse nodeOperation(GetAllPitNodeRequest request) { ); return nodeResponse; } + + @Override + protected void doExecute(Task task, GetAllPitNodesRequest request, ActionListener listener) { + super.doExecute( + task, + request, + ActionListener.wrap(response -> { pitService.getAllPits(response, listener); }, e -> { listener.onFailure(e); }) + ); + } } diff --git a/server/src/main/java/org/opensearch/client/Client.java b/server/src/main/java/org/opensearch/client/Client.java index d996cbfbe6e89..94043d5c3c89f 100644 --- a/server/src/main/java/org/opensearch/client/Client.java +++ b/server/src/main/java/org/opensearch/client/Client.java @@ -64,8 +64,6 @@ import org.opensearch.action.search.CreatePitResponse; import org.opensearch.action.search.DeletePitRequest; import org.opensearch.action.search.DeletePitResponse; -import org.opensearch.action.search.GetAllPitNodesRequest; -import org.opensearch.action.search.GetAllPitNodesResponse; import org.opensearch.action.search.MultiSearchRequest; import org.opensearch.action.search.MultiSearchRequestBuilder; import org.opensearch.action.search.MultiSearchResponse; @@ -343,8 +341,6 @@ public interface Client extends OpenSearchClient, Releasable { */ void deletePits(DeletePitRequest deletePITRequest, ActionListener listener); - void getAllPits(GetAllPitNodesRequest getAllPitNodesRequest, ActionListener listener); - /** * Get information of segments of one or more PITs */ diff --git a/server/src/main/java/org/opensearch/client/ClusterAdminClient.java b/server/src/main/java/org/opensearch/client/ClusterAdminClient.java index 7a7b98bf724f6..a1d618d5b7d18 100644 --- a/server/src/main/java/org/opensearch/client/ClusterAdminClient.java +++ b/server/src/main/java/org/opensearch/client/ClusterAdminClient.java @@ -132,6 +132,8 @@ import org.opensearch.action.ingest.SimulatePipelineRequest; import org.opensearch.action.ingest.SimulatePipelineRequestBuilder; import org.opensearch.action.ingest.SimulatePipelineResponse; +import org.opensearch.action.search.GetAllPitNodesRequest; +import org.opensearch.action.search.GetAllPitNodesResponse; import org.opensearch.action.support.master.AcknowledgedResponse; import org.opensearch.common.bytes.BytesReference; import org.opensearch.common.xcontent.XContentType; @@ -340,6 +342,8 @@ public interface ClusterAdminClient extends OpenSearchClient { */ NodesHotThreadsRequestBuilder prepareNodesHotThreads(String... nodesIds); + void listAllPits(GetAllPitNodesRequest request, ActionListener listener); + /** * List tasks * diff --git a/server/src/main/java/org/opensearch/client/support/AbstractClient.java b/server/src/main/java/org/opensearch/client/support/AbstractClient.java index b20d5432d17b7..c4149a9840450 100644 --- a/server/src/main/java/org/opensearch/client/support/AbstractClient.java +++ b/server/src/main/java/org/opensearch/client/support/AbstractClient.java @@ -337,11 +337,11 @@ import org.opensearch.action.search.DeletePitResponse; import org.opensearch.action.search.GetAllPitNodesRequest; import org.opensearch.action.search.GetAllPitNodesResponse; -import org.opensearch.action.search.GetAllPitsAction; import org.opensearch.action.search.MultiSearchAction; import org.opensearch.action.search.MultiSearchRequest; import org.opensearch.action.search.MultiSearchRequestBuilder; import org.opensearch.action.search.MultiSearchResponse; +import org.opensearch.action.search.NodesGetAllPitsAction; import org.opensearch.action.search.SearchAction; import org.opensearch.action.search.SearchRequest; import org.opensearch.action.search.SearchRequestBuilder; @@ -598,11 +598,6 @@ public void deletePits(final DeletePitRequest deletePITRequest, final ActionList execute(DeletePitAction.INSTANCE, deletePITRequest, listener); } - @Override - public void getAllPits(final GetAllPitNodesRequest getAllPitNodesRequest, final ActionListener listener) { - execute(GetAllPitsAction.INSTANCE, getAllPitNodesRequest, listener); - } - @Override public void pitSegments(final PitSegmentsRequest request, final ActionListener listener) { execute(PitSegmentsAction.INSTANCE, request, listener); @@ -897,6 +892,11 @@ public NodesHotThreadsRequestBuilder prepareNodesHotThreads(String... nodesIds) return new NodesHotThreadsRequestBuilder(this, NodesHotThreadsAction.INSTANCE).setNodesIds(nodesIds); } + @Override + public void listAllPits(GetAllPitNodesRequest request, ActionListener listener) { + execute(NodesGetAllPitsAction.INSTANCE, request, listener); + } + @Override public ActionFuture listTasks(final ListTasksRequest request) { return execute(ListTasksAction.INSTANCE, request); diff --git a/server/src/main/java/org/opensearch/rest/action/cat/RestPitSegmentsAction.java b/server/src/main/java/org/opensearch/rest/action/cat/RestPitSegmentsAction.java index 7075ea13d4f27..8e5599a14d776 100644 --- a/server/src/main/java/org/opensearch/rest/action/cat/RestPitSegmentsAction.java +++ b/server/src/main/java/org/opensearch/rest/action/cat/RestPitSegmentsAction.java @@ -8,15 +8,16 @@ package org.opensearch.rest.action.cat; -import org.opensearch.action.admin.cluster.state.ClusterStateRequest; -import org.opensearch.action.admin.cluster.state.ClusterStateResponse; import org.opensearch.action.admin.indices.segments.IndexSegments; import org.opensearch.action.admin.indices.segments.IndexShardSegments; import org.opensearch.action.admin.indices.segments.IndicesSegmentResponse; import org.opensearch.action.admin.indices.segments.PitSegmentsAction; import org.opensearch.action.admin.indices.segments.PitSegmentsRequest; import org.opensearch.action.admin.indices.segments.ShardSegments; +import org.opensearch.action.search.GetAllPitNodesRequest; +import org.opensearch.action.search.GetAllPitNodesResponse; import org.opensearch.client.node.NodeClient; +import org.opensearch.cluster.node.DiscoveryNode; import org.opensearch.cluster.node.DiscoveryNodes; import org.opensearch.common.Table; import org.opensearch.common.logging.DeprecationLogger; @@ -29,8 +30,12 @@ import org.opensearch.rest.action.RestResponseListener; import java.io.IOException; +import java.util.ArrayList; +import java.util.HashMap; import java.util.List; import java.util.Map; +import java.util.function.Supplier; +import java.util.stream.Collectors; import static java.util.Arrays.asList; import static java.util.Collections.unmodifiableList; @@ -43,6 +48,13 @@ public class RestPitSegmentsAction extends AbstractCatAction { private static final DeprecationLogger deprecationLogger = DeprecationLogger.getLogger(RestPitSegmentsAction.class); + private final Supplier nodesInCluster; + + public RestPitSegmentsAction(Supplier nodesInCluster) { + super(); + this.nodesInCluster = nodesInCluster; + } + @Override public List routes() { return unmodifiableList(asList(new Route(GET, "/_cat/pit_segments/_all"), new Route(GET, "/_cat/pit_segments"))); @@ -60,42 +72,61 @@ public boolean allowSystemIndexAccessByDefault() { @Override protected BaseRestHandler.RestChannelConsumer doCatRequest(final RestRequest request, final NodeClient client) { - final ClusterStateRequest clusterStateRequest = new ClusterStateRequest(); - clusterStateRequest.local(false); - clusterStateRequest.clusterManagerNodeTimeout( - request.paramAsTime("cluster_manager_timeout", clusterStateRequest.clusterManagerNodeTimeout()) - ); - parseDeprecatedMasterTimeoutParameter(clusterStateRequest, request, deprecationLogger, getName()); - clusterStateRequest.clear().nodes(true).routingTable(true).indices("*"); - return channel -> client.admin().cluster().state(clusterStateRequest, new RestActionListener<>(channel) { - @Override - public void processResponse(final ClusterStateResponse clusterStateResponse) throws IOException { - String allPitIdsQualifier = "_all"; - final PitSegmentsRequest pitSegmentsRequest; - if (request.path().contains(allPitIdsQualifier)) { - pitSegmentsRequest = new PitSegmentsRequest(allPitIdsQualifier); - } else { - pitSegmentsRequest = new PitSegmentsRequest(); - request.withContentOrSourceParamParserOrNull((xContentParser -> { - if (xContentParser != null) { - try { - pitSegmentsRequest.fromXContent(xContentParser); - } catch (IOException e) { - throw new IllegalArgumentException("Failed to parse request body", e); - } - } - })); - } - client.execute(PitSegmentsAction.INSTANCE, pitSegmentsRequest, new RestResponseListener<>(channel) { + String allPitIdsQualifier = "_all"; + final PitSegmentsRequest pitSegmentsRequest; + if (request.path().contains(allPitIdsQualifier)) { + pitSegmentsRequest = new PitSegmentsRequest(allPitIdsQualifier); + } else { + pitSegmentsRequest = new PitSegmentsRequest(); + try { + request.withContentOrSourceParamParserOrNull((xContentParser -> { + if (xContentParser != null) { + pitSegmentsRequest.fromXContent(xContentParser); + } + })); + } catch (IOException e) { + throw new IllegalArgumentException("Failed to parse request body", e); + } + } + if (request.path().contains(allPitIdsQualifier)) { + final List nodes = new ArrayList<>(); + for (DiscoveryNode node : nodesInCluster.get()) { + nodes.add(node); + } + DiscoveryNode[] disNodesArr = nodes.toArray(new DiscoveryNode[nodes.size()]); + GetAllPitNodesRequest getAllPitNodesRequest = new GetAllPitNodesRequest(disNodesArr); + return channel -> client.admin() + .cluster() + .listAllPits(getAllPitNodesRequest, new RestActionListener(channel) { @Override - public RestResponse buildResponse(final IndicesSegmentResponse indicesSegmentResponse) throws Exception { - final Map indicesSegments = indicesSegmentResponse.getIndices(); - Table tab = buildTable(request, clusterStateResponse, indicesSegments); - return RestTable.buildResponse(tab, channel); + protected void processResponse(GetAllPitNodesResponse getAllPitNodesResponse) throws Exception { + if (getAllPitNodesResponse.getPitInfos().size() == 0) { + final Map indicesSegments = new HashMap<>(); + Table tab = buildTable(request, indicesSegments); + channel.sendResponse(RestTable.buildResponse(tab, channel)); + return; + } + pitSegmentsRequest.clearAndSetPitIds(getAllPitNodesResponse.getPitInfos().stream().map(r -> r.getPitId()).collect(Collectors.toList())); + client.execute(PitSegmentsAction.INSTANCE, pitSegmentsRequest, new RestResponseListener<>(channel) { + @Override + public RestResponse buildResponse(final IndicesSegmentResponse indicesSegmentResponse) throws Exception { + final Map indicesSegments = indicesSegmentResponse.getIndices(); + Table tab = buildTable(request, indicesSegments); + return RestTable.buildResponse(tab, channel); + } + }); } }); - } - }); + } else { + return channel -> client.execute(PitSegmentsAction.INSTANCE, pitSegmentsRequest, new RestResponseListener<>(channel) { + @Override + public RestResponse buildResponse(final IndicesSegmentResponse indicesSegmentResponse) throws Exception { + final Map indicesSegments = indicesSegmentResponse.getIndices(); + Table tab = buildTable(request, indicesSegments); + return RestTable.buildResponse(tab, channel); + } + }); + } } @Override @@ -127,10 +158,10 @@ protected Table getTableWithHeader(RestRequest request) { return table; } - private Table buildTable(final RestRequest request, ClusterStateResponse state, Map indicesSegments) { + private Table buildTable(final RestRequest request, Map indicesSegments) { Table table = getTableWithHeader(request); - DiscoveryNodes nodes = state.getState().nodes(); + DiscoveryNodes nodes = this.nodesInCluster.get(); table.startRow(); table.addCell("index", "default:true;alias:i,idx;desc:index name"); table.addCell("shard", "default:true;alias:s,sh;desc:shard name"); diff --git a/server/src/main/java/org/opensearch/rest/action/search/RestDeletePitAction.java b/server/src/main/java/org/opensearch/rest/action/search/RestDeletePitAction.java index 452e66f8f5018..d94b381b1707b 100644 --- a/server/src/main/java/org/opensearch/rest/action/search/RestDeletePitAction.java +++ b/server/src/main/java/org/opensearch/rest/action/search/RestDeletePitAction.java @@ -8,15 +8,24 @@ package org.opensearch.rest.action.search; +import org.opensearch.action.ActionListener; import org.opensearch.action.search.DeletePitRequest; import org.opensearch.action.search.DeletePitResponse; +import org.opensearch.action.search.GetAllPitNodesRequest; +import org.opensearch.action.search.GetAllPitNodesResponse; import org.opensearch.client.node.NodeClient; +import org.opensearch.cluster.node.DiscoveryNode; +import org.opensearch.cluster.node.DiscoveryNodes; import org.opensearch.rest.BaseRestHandler; import org.opensearch.rest.RestRequest; +import org.opensearch.rest.action.RestActionListener; import org.opensearch.rest.action.RestStatusToXContentListener; import java.io.IOException; +import java.util.ArrayList; import java.util.List; +import java.util.function.Supplier; +import java.util.stream.Collectors; import static java.util.Arrays.asList; import static java.util.Collections.unmodifiableList; @@ -27,6 +36,13 @@ */ public class RestDeletePitAction extends BaseRestHandler { + private final Supplier nodesInCluster; + + public RestDeletePitAction(Supplier nodesInCluster) { + super(); + this.nodesInCluster = nodesInCluster; + } + @Override public String getName() { return "delete_pit_action"; @@ -50,7 +66,50 @@ public RestChannelConsumer prepareRequest(RestRequest request, NodeClient client } })); } - return channel -> client.deletePits(deletePITRequest, new RestStatusToXContentListener(channel)); + /** + * Delete all active PIT reader contexts leveraging list all PITs + * + * For Cross cluster PITs : + * - mixed cluster PITs ( PIT comprising local and remote ) will be fully deleted. Since there will atleast be + * one reader context with PIT ID present in local cluster, 'Get all PITs' will retrieve the PIT ID with which + * we can completely delete the PIT contexts in both local and remote cluster. + * - fully remote PITs will not be deleted as 'Get all PITs' operates on local cluster only and no PIT info can + * be retrieved when it's fully remote. + */ + if (request.path().contains(allPitIdsQualifier)) { + final List nodes = new ArrayList<>(); + for (DiscoveryNode node : nodesInCluster.get()) { + nodes.add(node); + } + DiscoveryNode[] disNodesArr = nodes.toArray(new DiscoveryNode[nodes.size()]); + GetAllPitNodesRequest getAllPitNodesRequest = new GetAllPitNodesRequest(disNodesArr); + return deleteAllPits(client, getAllPitNodesRequest, deletePITRequest); + } else { + return channel -> client.deletePits(deletePITRequest, new RestStatusToXContentListener(channel)); + } + } + + private RestChannelConsumer deleteAllPits( + NodeClient client, + GetAllPitNodesRequest getAllPitNodesRequest, + DeletePitRequest deletePitRequest + ) { + return channel -> client.admin().cluster().listAllPits(getAllPitNodesRequest, new RestActionListener<>(channel) { + @Override + public void processResponse(final GetAllPitNodesResponse getAllPitNodesResponse) throws IOException { + // return empty response when no PITs are retrieved + if (getAllPitNodesResponse.getPitInfos().size() == 0) { + DeletePitResponse response = new DeletePitResponse(new ArrayList<>()); + ActionListener listener = new RestStatusToXContentListener(channel); + listener.onResponse(response); + return; + } + deletePitRequest.clearAndSetPitIds( + getAllPitNodesResponse.getPitInfos().stream().map(r -> r.getPitId()).collect(Collectors.toList()) + ); + client.deletePits(deletePitRequest, new RestStatusToXContentListener(channel)); + } + }); } @Override diff --git a/server/src/main/java/org/opensearch/rest/action/search/RestGetAllPitsAction.java b/server/src/main/java/org/opensearch/rest/action/search/RestGetAllPitsAction.java index a69c3b0bdba19..178c467dfec3f 100644 --- a/server/src/main/java/org/opensearch/rest/action/search/RestGetAllPitsAction.java +++ b/server/src/main/java/org/opensearch/rest/action/search/RestGetAllPitsAction.java @@ -12,17 +12,21 @@ import org.opensearch.action.search.GetAllPitNodesRequest; import org.opensearch.action.search.GetAllPitNodesResponse; import org.opensearch.client.node.NodeClient; +import org.opensearch.cluster.node.DiscoveryNode; +import org.opensearch.cluster.node.DiscoveryNodes; import org.opensearch.common.xcontent.XContentBuilder; import org.opensearch.rest.BaseRestHandler; import org.opensearch.rest.BytesRestResponse; import org.opensearch.rest.RestRequest; import org.opensearch.rest.RestResponse; import org.opensearch.rest.RestStatus; -import org.opensearch.rest.action.RestResponseListener; +import org.opensearch.rest.action.RestBuilderListener; import java.io.IOException; +import java.util.ArrayList; import java.util.Collections; import java.util.List; +import java.util.function.Supplier; import static java.util.Collections.unmodifiableList; import static org.opensearch.rest.RestRequest.Method.GET; @@ -31,6 +35,14 @@ * Rest action for retrieving all active PIT IDs across all nodes */ public class RestGetAllPitsAction extends BaseRestHandler { + + private final Supplier nodesInCluster; + + public RestGetAllPitsAction(Supplier nodesInCluster) { + super(); + this.nodesInCluster = nodesInCluster; + } + @Override public String getName() { return "get_all_pit_action"; @@ -38,33 +50,39 @@ public String getName() { @Override protected RestChannelConsumer prepareRequest(RestRequest request, NodeClient client) throws IOException { - GetAllPitNodesRequest getAllPITNodesRequest = new GetAllPitNodesRequest(); - return channel -> client.getAllPits(getAllPITNodesRequest, new RestResponseListener(channel) { - @Override - public RestResponse buildResponse(final GetAllPitNodesResponse getAllPITNodesResponse) throws Exception { - try (XContentBuilder builder = channel.newBuilder()) { + final List nodes = new ArrayList<>(); + for (DiscoveryNode node : nodesInCluster.get()) { + nodes.add(node); + } + DiscoveryNode[] disNodesArr = nodes.toArray(new DiscoveryNode[nodes.size()]); + GetAllPitNodesRequest getAllPitNodesRequest = new GetAllPitNodesRequest(disNodesArr); + return channel -> client.admin() + .cluster() + .listAllPits(getAllPitNodesRequest, new RestBuilderListener(channel) { + @Override + public RestResponse buildResponse(final GetAllPitNodesResponse getAllPITNodesResponse, XContentBuilder builder) + throws Exception { builder.startObject(); if (getAllPITNodesResponse.hasFailures()) { builder.startArray("failures"); for (int idx = 0; idx < getAllPITNodesResponse.failures().size(); idx++) { + builder.startObject(); builder.field( getAllPITNodesResponse.failures().get(idx).nodeId(), getAllPITNodesResponse.failures().get(idx).getDetailedMessage() ); + builder.endObject(); } builder.endArray(); } builder.field("pits", getAllPITNodesResponse.getPitInfos()); builder.endObject(); - if (getAllPITNodesResponse.hasFailures()) { + if (getAllPITNodesResponse.hasFailures() && getAllPITNodesResponse.getPitInfos().isEmpty()) { return new BytesRestResponse(RestStatus.INTERNAL_SERVER_ERROR, builder); - } else if (getAllPITNodesResponse.getPitInfos().isEmpty()) { - return new BytesRestResponse(RestStatus.NOT_FOUND, builder); } return new BytesRestResponse(RestStatus.OK, builder); } - } - }); + }); } @Override diff --git a/server/src/test/java/org/opensearch/action/search/TransportDeletePitActionTests.java b/server/src/test/java/org/opensearch/action/search/TransportDeletePitActionTests.java index bdc0440a89f69..2bf044d350679 100644 --- a/server/src/test/java/org/opensearch/action/search/TransportDeletePitActionTests.java +++ b/server/src/test/java/org/opensearch/action/search/TransportDeletePitActionTests.java @@ -14,7 +14,6 @@ import org.opensearch.action.support.ActionFilters; import org.opensearch.action.support.PlainActionFuture; import org.opensearch.client.node.NodeClient; -import org.opensearch.cluster.ClusterName; import org.opensearch.cluster.ClusterState; import org.opensearch.cluster.metadata.Metadata; import org.opensearch.cluster.node.DiscoveryNode; @@ -189,84 +188,6 @@ public Transport.Connection getConnection(String clusterAlias, DiscoveryNode nod } } - public void testDeleteAllPITSuccess() throws InterruptedException, ExecutionException { - List deleteNodesInvoked = new CopyOnWriteArrayList<>(); - ActionFilters actionFilters = mock(ActionFilters.class); - when(actionFilters.filters()).thenReturn(new ActionFilter[0]); - List knownNodes = new CopyOnWriteArrayList<>(); - try ( - MockTransportService cluster1Transport = startTransport("cluster_1_node", knownNodes, Version.CURRENT); - MockTransportService cluster2Transport = startTransport("cluster_2_node", knownNodes, Version.CURRENT) - ) { - knownNodes.add(cluster1Transport.getLocalDiscoNode()); - knownNodes.add(cluster2Transport.getLocalDiscoNode()); - Collections.shuffle(knownNodes, random()); - - try ( - MockTransportService transportService = MockTransportService.createNewService( - Settings.EMPTY, - Version.CURRENT, - threadPool, - null - ) - ) { - transportService.start(); - transportService.acceptIncomingRequests(); - SearchTransportService searchTransportService = new SearchTransportService(transportService, null) { - public void sendFreePITContexts( - Transport.Connection connection, - List contextIds, - final ActionListener listener - ) { - deleteNodesInvoked.add(connection.getNode()); - DeletePitInfo deletePitInfo = new DeletePitInfo(true, "pitId"); - List deletePitInfos = new ArrayList<>(); - deletePitInfos.add(deletePitInfo); - Thread t = new Thread(() -> listener.onResponse(new DeletePitResponse(deletePitInfos))); - t.start(); - } - - @Override - public Transport.Connection getConnection(String clusterAlias, DiscoveryNode node) { - return new SearchAsyncActionTests.MockConnection(node); - } - }; - PitService pitService = new PitService(clusterServiceMock, searchTransportService, transportService, client) { - @Override - public void getAllPits(ActionListener getAllPitsListener) { - ListPitInfo listPitInfo = new ListPitInfo(getPitId(), 0, 0); - List list = new ArrayList<>(); - list.add(listPitInfo); - GetAllPitNodeResponse getAllPitNodeResponse = new GetAllPitNodeResponse( - cluster1Transport.getLocalDiscoNode(), - list - ); - List nodeList = new ArrayList(); - nodeList.add(getAllPitNodeResponse); - getAllPitsListener.onResponse(new GetAllPitNodesResponse(new ClusterName("cn"), nodeList, new ArrayList())); - } - }; - TransportDeletePitAction action = new TransportDeletePitAction( - transportService, - actionFilters, - namedWriteableRegistry, - transportSearchAction, - clusterServiceMock, - searchTransportService, - pitService - ); - DeletePitRequest deletePITRequest = new DeletePitRequest("_all"); - PlainActionFuture future = newFuture(); - action.execute(task, deletePITRequest, future); - DeletePitResponse dr = future.get(); - assertTrue(dr.getDeletePitResults().get(0).getPitId().equals("pitId")); - assertTrue(dr.getDeletePitResults().get(0).isSuccessful()); - assertEquals(3, deleteNodesInvoked.size()); - - } - } - } - public void testDeletePitWhenNodeIsDown() throws InterruptedException, ExecutionException { List deleteNodesInvoked = new CopyOnWriteArrayList<>(); ActionFilters actionFilters = mock(ActionFilters.class); @@ -461,239 +382,4 @@ public Transport.Connection getConnection(String clusterAlias, DiscoveryNode nod } } - public void testDeleteAllPitWhenNodeIsDown() { - List deleteNodesInvoked = new CopyOnWriteArrayList<>(); - ActionFilters actionFilters = mock(ActionFilters.class); - when(actionFilters.filters()).thenReturn(new ActionFilter[0]); - - List knownNodes = new CopyOnWriteArrayList<>(); - try ( - MockTransportService cluster1Transport = startTransport("cluster_1_node", knownNodes, Version.CURRENT); - MockTransportService cluster2Transport = startTransport("cluster_2_node", knownNodes, Version.CURRENT) - ) { - knownNodes.add(cluster1Transport.getLocalDiscoNode()); - knownNodes.add(cluster2Transport.getLocalDiscoNode()); - Collections.shuffle(knownNodes, random()); - - try ( - MockTransportService transportService = MockTransportService.createNewService( - Settings.EMPTY, - Version.CURRENT, - threadPool, - null - ) - ) { - transportService.start(); - transportService.acceptIncomingRequests(); - SearchTransportService searchTransportService = new SearchTransportService(transportService, null) { - @Override - public void sendFreePITContexts( - Transport.Connection connection, - List contextIds, - final ActionListener listener - ) { - deleteNodesInvoked.add(connection.getNode()); - if (connection.getNode().getId() == "node_3") { - Thread t = new Thread(() -> listener.onFailure(new Exception("node 3 down"))); - t.start(); - } else { - Thread t = new Thread(() -> listener.onResponse(new DeletePitResponse(new ArrayList<>()))); - t.start(); - } - } - - @Override - public Transport.Connection getConnection(String clusterAlias, DiscoveryNode node) { - return new SearchAsyncActionTests.MockConnection(node); - } - }; - PitService pitService = new PitService(clusterServiceMock, searchTransportService, transportService, client) { - @Override - public void getAllPits(ActionListener getAllPitsListener) { - ListPitInfo listPitInfo = new ListPitInfo(getPitId(), 0, 0); - List list = new ArrayList<>(); - list.add(listPitInfo); - GetAllPitNodeResponse getAllPitNodeResponse = new GetAllPitNodeResponse( - cluster1Transport.getLocalDiscoNode(), - list - ); - List nodeList = new ArrayList(); - nodeList.add(getAllPitNodeResponse); - getAllPitsListener.onResponse(new GetAllPitNodesResponse(new ClusterName("cn"), nodeList, new ArrayList())); - } - }; - TransportDeletePitAction action = new TransportDeletePitAction( - transportService, - actionFilters, - namedWriteableRegistry, - transportSearchAction, - clusterServiceMock, - searchTransportService, - pitService - ); - DeletePitRequest deletePITRequest = new DeletePitRequest("_all"); - PlainActionFuture future = newFuture(); - action.execute(task, deletePITRequest, future); - Exception e = assertThrows(ExecutionException.class, () -> future.get()); - assertThat(e.getMessage(), containsString("node 3 down")); - assertEquals(3, deleteNodesInvoked.size()); - } - } - } - - public void testDeleteAllPitWhenAllNodesAreDown() { - List deleteNodesInvoked = new CopyOnWriteArrayList<>(); - ActionFilters actionFilters = mock(ActionFilters.class); - when(actionFilters.filters()).thenReturn(new ActionFilter[0]); - - List knownNodes = new CopyOnWriteArrayList<>(); - try ( - MockTransportService cluster1Transport = startTransport("cluster_1_node", knownNodes, Version.CURRENT); - MockTransportService cluster2Transport = startTransport("cluster_2_node", knownNodes, Version.CURRENT) - ) { - knownNodes.add(cluster1Transport.getLocalDiscoNode()); - knownNodes.add(cluster2Transport.getLocalDiscoNode()); - Collections.shuffle(knownNodes, random()); - - try ( - MockTransportService transportService = MockTransportService.createNewService( - Settings.EMPTY, - Version.CURRENT, - threadPool, - null - ) - ) { - transportService.start(); - transportService.acceptIncomingRequests(); - SearchTransportService searchTransportService = new SearchTransportService(transportService, null) { - - @Override - public void sendFreePITContexts( - Transport.Connection connection, - List contextIds, - final ActionListener listener - ) { - deleteNodesInvoked.add(connection.getNode()); - Thread t = new Thread(() -> listener.onFailure(new Exception("node down"))); - t.start(); - } - - @Override - public Transport.Connection getConnection(String clusterAlias, DiscoveryNode node) { - return new SearchAsyncActionTests.MockConnection(node); - } - }; - PitService pitService = new PitService(clusterServiceMock, searchTransportService, transportService, client) { - @Override - public void getAllPits(ActionListener getAllPitsListener) { - ListPitInfo listPitInfo = new ListPitInfo(getPitId(), 0, 0); - List list = new ArrayList<>(); - list.add(listPitInfo); - GetAllPitNodeResponse getAllPitNodeResponse = new GetAllPitNodeResponse( - cluster1Transport.getLocalDiscoNode(), - list - ); - List nodeList = new ArrayList(); - nodeList.add(getAllPitNodeResponse); - getAllPitsListener.onResponse(new GetAllPitNodesResponse(new ClusterName("cn"), nodeList, new ArrayList())); - } - }; - TransportDeletePitAction action = new TransportDeletePitAction( - transportService, - actionFilters, - namedWriteableRegistry, - transportSearchAction, - clusterServiceMock, - searchTransportService, - pitService - ); - DeletePitRequest deletePITRequest = new DeletePitRequest("_all"); - PlainActionFuture future = newFuture(); - action.execute(task, deletePITRequest, future); - Exception e = assertThrows(ExecutionException.class, () -> future.get()); - assertThat(e.getMessage(), containsString("node down")); - assertEquals(3, deleteNodesInvoked.size()); - } - } - } - - public void testDeleteAllPitFailure() { - List deleteNodesInvoked = new CopyOnWriteArrayList<>(); - ActionFilters actionFilters = mock(ActionFilters.class); - when(actionFilters.filters()).thenReturn(new ActionFilter[0]); - - List knownNodes = new CopyOnWriteArrayList<>(); - try ( - MockTransportService cluster1Transport = startTransport("cluster_1_node", knownNodes, Version.CURRENT); - MockTransportService cluster2Transport = startTransport("cluster_2_node", knownNodes, Version.CURRENT) - ) { - knownNodes.add(cluster1Transport.getLocalDiscoNode()); - knownNodes.add(cluster2Transport.getLocalDiscoNode()); - Collections.shuffle(knownNodes, random()); - - try ( - MockTransportService transportService = MockTransportService.createNewService( - Settings.EMPTY, - Version.CURRENT, - threadPool, - null - ) - ) { - transportService.start(); - transportService.acceptIncomingRequests(); - SearchTransportService searchTransportService = new SearchTransportService(transportService, null) { - - public void sendFreePITContexts( - Transport.Connection connection, - List contextId, - final ActionListener listener - ) { - deleteNodesInvoked.add(connection.getNode()); - if (connection.getNode().getId() == "node_3") { - Thread t = new Thread(() -> listener.onFailure(new Exception("node 3 is down"))); - t.start(); - } else { - Thread t = new Thread(() -> listener.onResponse(new DeletePitResponse(new ArrayList<>()))); - t.start(); - } - } - - @Override - public Transport.Connection getConnection(String clusterAlias, DiscoveryNode node) { - return new SearchAsyncActionTests.MockConnection(node); - } - }; - PitService pitService = new PitService(clusterServiceMock, searchTransportService, transportService, client) { - @Override - public void getAllPits(ActionListener getAllPitsListener) { - ListPitInfo listPitInfo = new ListPitInfo(getPitId(), 0, 0); - List list = new ArrayList<>(); - list.add(listPitInfo); - GetAllPitNodeResponse getAllPitNodeResponse = new GetAllPitNodeResponse( - cluster1Transport.getLocalDiscoNode(), - list - ); - List nodeList = new ArrayList(); - nodeList.add(getAllPitNodeResponse); - getAllPitsListener.onResponse(new GetAllPitNodesResponse(new ClusterName("cn"), nodeList, new ArrayList())); - } - }; - TransportDeletePitAction action = new TransportDeletePitAction( - transportService, - actionFilters, - namedWriteableRegistry, - transportSearchAction, - clusterServiceMock, - searchTransportService, - pitService - ); - DeletePitRequest deletePITRequest = new DeletePitRequest("_all"); - PlainActionFuture future = newFuture(); - action.execute(task, deletePITRequest, future); - Exception e = assertThrows(ExecutionException.class, () -> future.get()); - assertThat(e.getMessage(), containsString("java.lang.Exception: node 3 is down")); - assertEquals(3, deleteNodesInvoked.size()); - } - } - } } diff --git a/server/src/test/java/org/opensearch/search/pit/RestDeletePitActionTests.java b/server/src/test/java/org/opensearch/search/pit/RestDeletePitActionTests.java index 0bfa16aafe1e3..14ecbf3495582 100644 --- a/server/src/test/java/org/opensearch/search/pit/RestDeletePitActionTests.java +++ b/server/src/test/java/org/opensearch/search/pit/RestDeletePitActionTests.java @@ -9,11 +9,15 @@ package org.opensearch.search.pit; import org.apache.lucene.util.SetOnce; +import org.opensearch.Version; import org.opensearch.action.ActionListener; import org.opensearch.action.search.DeletePitRequest; import org.opensearch.action.search.DeletePitResponse; import org.opensearch.client.node.NodeClient; +import org.opensearch.cluster.node.DiscoveryNode; +import org.opensearch.cluster.node.DiscoveryNodes; import org.opensearch.common.bytes.BytesArray; +import org.opensearch.common.transport.TransportAddress; import org.opensearch.common.xcontent.XContentType; import org.opensearch.rest.RestRequest; import org.opensearch.rest.action.search.RestDeletePitAction; @@ -23,6 +27,7 @@ import org.opensearch.test.rest.FakeRestRequest; import java.util.Collections; +import java.util.function.Supplier; import static org.hamcrest.Matchers.equalTo; import static org.hamcrest.Matchers.hasSize; @@ -32,7 +37,18 @@ */ public class RestDeletePitActionTests extends OpenSearchTestCase { public void testParseDeletePitRequestWithInvalidJsonThrowsException() throws Exception { - RestDeletePitAction action = new RestDeletePitAction(); + RestDeletePitAction action = new RestDeletePitAction(new Supplier() { + @Override + public DiscoveryNodes get() { + DiscoveryNode node0 = new DiscoveryNode( + "node0", + new TransportAddress(TransportAddress.META_ADDRESS, 9000), + Version.CURRENT + ); + DiscoveryNodes nodes = new DiscoveryNodes.Builder().add(node0).clusterManagerNodeId(node0.getId()).build(); + return nodes; + } + }); RestRequest request = new FakeRestRequest.Builder(xContentRegistry()).withContent( new BytesArray("{invalid_json}"), XContentType.JSON @@ -51,7 +67,18 @@ public void deletePits(DeletePitRequest request, ActionListener() { + @Override + public DiscoveryNodes get() { + DiscoveryNode node0 = new DiscoveryNode( + "node0", + new TransportAddress(TransportAddress.META_ADDRESS, 9000), + Version.CURRENT + ); + DiscoveryNodes nodes = new DiscoveryNodes.Builder().add(node0).clusterManagerNodeId(node0.getId()).build(); + return nodes; + } + }); RestRequest request = new FakeRestRequest.Builder(xContentRegistry()).withContent( new BytesArray("{\"pit_id\": [\"BODY\"]}"), XContentType.JSON @@ -73,7 +100,18 @@ public void deletePits(DeletePitRequest request, ActionListener() { + @Override + public DiscoveryNodes get() { + DiscoveryNode node0 = new DiscoveryNode( + "node0", + new TransportAddress(TransportAddress.META_ADDRESS, 9000), + Version.CURRENT + ); + DiscoveryNodes nodes = new DiscoveryNodes.Builder().add(node0).clusterManagerNodeId(node0.getId()).build(); + return nodes; + } + }); RestRequest request = new FakeRestRequest.Builder(xContentRegistry()).withPath("/_all").build(); FakeRestChannel channel = new FakeRestChannel(request, false, 0); action.handleRequest(request, channel, nodeClient); @@ -92,7 +130,18 @@ public void deletePits(DeletePitRequest request, ActionListener() { + @Override + public DiscoveryNodes get() { + DiscoveryNode node0 = new DiscoveryNode( + "node0", + new TransportAddress(TransportAddress.META_ADDRESS, 9000), + Version.CURRENT + ); + DiscoveryNodes nodes = new DiscoveryNodes.Builder().add(node0).clusterManagerNodeId(node0.getId()).build(); + return nodes; + } + }); RestRequest request = new FakeRestRequest.Builder(xContentRegistry()).withContent( new BytesArray("{\"pit_id\": [\"BODY\"]}"), XContentType.JSON @@ -118,7 +167,18 @@ public void deletePits(DeletePitRequest request, ActionListener() { + @Override + public DiscoveryNodes get() { + DiscoveryNode node0 = new DiscoveryNode( + "node0", + new TransportAddress(TransportAddress.META_ADDRESS, 9000), + Version.CURRENT + ); + DiscoveryNodes nodes = new DiscoveryNodes.Builder().add(node0).clusterManagerNodeId(node0.getId()).build(); + return nodes; + } + }); RestRequest request = new FakeRestRequest.Builder(xContentRegistry()).withParams( Collections.singletonMap("pit_id", "QUERY_STRING,QUERY_STRING_1") ).build(); diff --git a/server/src/test/java/org/opensearch/search/pit/RestGetAllPitsActionTests.java b/server/src/test/java/org/opensearch/search/pit/RestGetAllPitsActionTests.java index 9d6c5af1f2715..ab21a6c0b9327 100644 --- a/server/src/test/java/org/opensearch/search/pit/RestGetAllPitsActionTests.java +++ b/server/src/test/java/org/opensearch/search/pit/RestGetAllPitsActionTests.java @@ -8,38 +8,31 @@ package org.opensearch.search.pit; -import org.apache.lucene.util.SetOnce; -import org.opensearch.action.ActionListener; -import org.opensearch.action.search.GetAllPitNodesRequest; -import org.opensearch.action.search.GetAllPitNodesResponse; -import org.opensearch.client.node.NodeClient; -import org.opensearch.rest.RestRequest; -import org.opensearch.rest.action.search.RestGetAllPitsAction; import org.opensearch.test.OpenSearchTestCase; -import org.opensearch.test.client.NoOpNodeClient; -import org.opensearch.test.rest.FakeRestChannel; -import org.opensearch.test.rest.FakeRestRequest; - -import static org.hamcrest.Matchers.equalTo; /** * Tests for rest get all PITs action */ public class RestGetAllPitsActionTests extends OpenSearchTestCase { - public void testGetAllPits() throws Exception { - SetOnce pitCalled = new SetOnce<>(); - try (NodeClient nodeClient = new NoOpNodeClient(this.getTestName()) { - @Override - public void getAllPits(GetAllPitNodesRequest request, ActionListener listener) { - pitCalled.set(true); - } - }) { - RestGetAllPitsAction action = new RestGetAllPitsAction(); - RestRequest request = new FakeRestRequest.Builder(xContentRegistry()).withPath("/_all").build(); - FakeRestChannel channel = new FakeRestChannel(request, false, 0); - action.handleRequest(request, channel, nodeClient); - assertThat(pitCalled.get(), equalTo(true)); - } - } + // public void testGetAllPits() throws Exception { + // SetOnce pitCalled = new SetOnce<>(); + // try (NodeClient nodeClient = new NoOpNodeClient(this.getTestName()) { + //// @Override + //// public void getAllPits(GetAllPitNodesRequest request, ActionListener listener) { + //// pitCalled.set(true); + //// } + // }) { + // RestGetAllPitsAction action = new RestGetAllPitsAction(new Supplier() { + // @Override + // public DiscoveryNodes get() { + // return null; + // } + // }); + // RestRequest request = new FakeRestRequest.Builder(xContentRegistry()).withPath("/_all").build(); + // FakeRestChannel channel = new FakeRestChannel(request, false, 0); + // action.handleRequest(request, channel, nodeClient); + // assertThat(pitCalled.get(), equalTo(true)); + // } + // } } From cc67e750b76efcc742bb332c2b737dc51b1f203f Mon Sep 17 00:00:00 2001 From: Bharathwaj G Date: Tue, 6 Sep 2022 18:59:21 +0530 Subject: [PATCH 08/12] Security model changes Signed-off-by: Bharathwaj G --- .../java/org/opensearch/client/PitIT.java | 1 - .../opensearch/action/search/PitService.java | 2 +- .../TransportNodesGetAllPitsAction.java | 4 ++ .../action/cat/RestPitSegmentsAction.java | 4 +- .../action/search/RestDeletePitAction.java | 20 +++---- .../action/search/PitTestsUtil.java | 17 ++++-- .../search/CreatePitSingleNodeTests.java | 29 ++++----- .../search/DeletePitMultiNodeTests.java | 59 ------------------- .../opensearch/search/PitMultiNodeTests.java | 12 ++-- .../search/pit/RestGetAllPitsActionTests.java | 38 ------------ 10 files changed, 47 insertions(+), 139 deletions(-) delete mode 100644 server/src/test/java/org/opensearch/search/pit/RestGetAllPitsActionTests.java diff --git a/client/rest-high-level/src/test/java/org/opensearch/client/PitIT.java b/client/rest-high-level/src/test/java/org/opensearch/client/PitIT.java index 7f81bf393d619..cbb4db10cd519 100644 --- a/client/rest-high-level/src/test/java/org/opensearch/client/PitIT.java +++ b/client/rest-high-level/src/test/java/org/opensearch/client/PitIT.java @@ -126,7 +126,6 @@ public void onFailure(Exception e) { // validate no pits case getAllPitResponse = highLevelClient().getAllPits(RequestOptions.DEFAULT); assertTrue(getAllPitResponse.getPitInfos().size() == 0); - // highLevelClient().getAllPitsAsync(RequestOptions.DEFAULT, getPitsListener); highLevelClient().deleteAllPitsAsync(RequestOptions.DEFAULT, deletePitListener); } } diff --git a/server/src/main/java/org/opensearch/action/search/PitService.java b/server/src/main/java/org/opensearch/action/search/PitService.java index 984424e00276e..596f8525a0293 100644 --- a/server/src/main/java/org/opensearch/action/search/PitService.java +++ b/server/src/main/java/org/opensearch/action/search/PitService.java @@ -176,7 +176,7 @@ public void getAllPits(GetAllPitNodesResponse getAllPitNodesResponse, ActionList DiscoveryNode node = cursor.value; nodes.add(node); } - logger.debug("Number of active PTIs in cluster: " + getAllPitNodesResponse.getPitInfos().size()); + logger.debug("Number of active PITs in cluster: " + getAllPitNodesResponse.getPitInfos().size()); DiscoveryNode[] disNodesArr = nodes.toArray(new DiscoveryNode[nodes.size()]); GetAllPitNodesRequest getAllPitNodesRequest = new GetAllPitNodesRequest(disNodesArr); getAllPitNodesRequest.setGetAllPitNodesResponse(getAllPitNodesResponse); diff --git a/server/src/main/java/org/opensearch/action/search/TransportNodesGetAllPitsAction.java b/server/src/main/java/org/opensearch/action/search/TransportNodesGetAllPitsAction.java index 7eee946dd79ea..39f7325f62409 100644 --- a/server/src/main/java/org/opensearch/action/search/TransportNodesGetAllPitsAction.java +++ b/server/src/main/java/org/opensearch/action/search/TransportNodesGetAllPitsAction.java @@ -90,6 +90,10 @@ protected GetAllPitNodeResponse nodeOperation(GetAllPitNodeRequest request) { return nodeResponse; } + /** + * Call list all pits of 'indices:data:read' type so that PITs get filtered in security plugin based + * user indices permission + */ @Override protected void doExecute(Task task, GetAllPitNodesRequest request, ActionListener listener) { super.doExecute( diff --git a/server/src/main/java/org/opensearch/rest/action/cat/RestPitSegmentsAction.java b/server/src/main/java/org/opensearch/rest/action/cat/RestPitSegmentsAction.java index 8e5599a14d776..c5432f9157b4a 100644 --- a/server/src/main/java/org/opensearch/rest/action/cat/RestPitSegmentsAction.java +++ b/server/src/main/java/org/opensearch/rest/action/cat/RestPitSegmentsAction.java @@ -106,7 +106,9 @@ protected void processResponse(GetAllPitNodesResponse getAllPitNodesResponse) th channel.sendResponse(RestTable.buildResponse(tab, channel)); return; } - pitSegmentsRequest.clearAndSetPitIds(getAllPitNodesResponse.getPitInfos().stream().map(r -> r.getPitId()).collect(Collectors.toList())); + pitSegmentsRequest.clearAndSetPitIds( + getAllPitNodesResponse.getPitInfos().stream().map(r -> r.getPitId()).collect(Collectors.toList()) + ); client.execute(PitSegmentsAction.INSTANCE, pitSegmentsRequest, new RestResponseListener<>(channel) { @Override public RestResponse buildResponse(final IndicesSegmentResponse indicesSegmentResponse) throws Exception { diff --git a/server/src/main/java/org/opensearch/rest/action/search/RestDeletePitAction.java b/server/src/main/java/org/opensearch/rest/action/search/RestDeletePitAction.java index d94b381b1707b..30fcc7a88338f 100644 --- a/server/src/main/java/org/opensearch/rest/action/search/RestDeletePitAction.java +++ b/server/src/main/java/org/opensearch/rest/action/search/RestDeletePitAction.java @@ -66,16 +66,6 @@ public RestChannelConsumer prepareRequest(RestRequest request, NodeClient client } })); } - /** - * Delete all active PIT reader contexts leveraging list all PITs - * - * For Cross cluster PITs : - * - mixed cluster PITs ( PIT comprising local and remote ) will be fully deleted. Since there will atleast be - * one reader context with PIT ID present in local cluster, 'Get all PITs' will retrieve the PIT ID with which - * we can completely delete the PIT contexts in both local and remote cluster. - * - fully remote PITs will not be deleted as 'Get all PITs' operates on local cluster only and no PIT info can - * be retrieved when it's fully remote. - */ if (request.path().contains(allPitIdsQualifier)) { final List nodes = new ArrayList<>(); for (DiscoveryNode node : nodesInCluster.get()) { @@ -89,6 +79,16 @@ public RestChannelConsumer prepareRequest(RestRequest request, NodeClient client } } + /** + * Delete all active PIT reader contexts leveraging list all PITs + * + * For Cross cluster PITs : + * - mixed cluster PITs ( PIT comprising local and remote ) will be fully deleted. Since there will atleast be + * one reader context with PIT ID present in local cluster, 'Get all PITs' will retrieve the PIT ID with which + * we can completely delete the PIT contexts in both local and remote cluster. + * - fully remote PITs will not be deleted as 'Get all PITs' operates on local cluster only and no PIT info can + * be retrieved when it's fully remote. + */ private RestChannelConsumer deleteAllPits( NodeClient client, GetAllPitNodesRequest getAllPitNodesRequest, diff --git a/server/src/test/java/org/opensearch/action/search/PitTestsUtil.java b/server/src/test/java/org/opensearch/action/search/PitTestsUtil.java index 60a31c62dc32d..f632db0b69748 100644 --- a/server/src/test/java/org/opensearch/action/search/PitTestsUtil.java +++ b/server/src/test/java/org/opensearch/action/search/PitTestsUtil.java @@ -30,6 +30,7 @@ import org.opensearch.search.internal.AliasFilter; import org.opensearch.search.internal.ShardSearchContextId; +import java.util.ArrayList; import java.util.HashMap; import java.util.LinkedList; import java.util.List; @@ -110,7 +111,7 @@ public static void assertUsingGetAllPits(Client client, String id, long creation DiscoveryNode[] disNodesArr = new DiscoveryNode[nodes.size()]; nodes.toArray(disNodesArr); GetAllPitNodesRequest getAllPITNodesRequest = new GetAllPitNodesRequest(disNodesArr); - ActionFuture execute1 = client.execute(GetAllPitsAction.INSTANCE, getAllPITNodesRequest); + ActionFuture execute1 = client.execute(NodesGetAllPitsAction.INSTANCE, getAllPITNodesRequest); GetAllPitNodesResponse getPitResponse = execute1.get(); assertTrue(getPitResponse.getPitInfos().get(0).getPitId().contains(id)); Assert.assertEquals(getPitResponse.getPitInfos().get(0).getCreationTime(), creationTime); @@ -129,13 +130,17 @@ public static void assertGetAllPitsEmpty(Client client) throws ExecutionExceptio DiscoveryNode[] disNodesArr = new DiscoveryNode[nodes.size()]; nodes.toArray(disNodesArr); GetAllPitNodesRequest getAllPITNodesRequest = new GetAllPitNodesRequest(disNodesArr); - ActionFuture execute1 = client.execute(GetAllPitsAction.INSTANCE, getAllPITNodesRequest); + ActionFuture execute1 = client.execute(NodesGetAllPitsAction.INSTANCE, getAllPITNodesRequest); GetAllPitNodesResponse getPitResponse = execute1.get(); Assert.assertEquals(0, getPitResponse.getPitInfos().size()); } - public static void assertSegments(boolean isEmpty, String index, long expectedShardSize, Client client) { - PitSegmentsRequest pitSegmentsRequest = new PitSegmentsRequest("_all"); + public static void assertSegments(boolean isEmpty, String index, long expectedShardSize, Client client, String pitId) { + PitSegmentsRequest pitSegmentsRequest; + pitSegmentsRequest = new PitSegmentsRequest(); + List pitIds = new ArrayList<>(); + pitIds.add(pitId); + pitSegmentsRequest.clearAndSetPitIds(pitIds); IndicesSegmentResponse indicesSegmentResponse = client.execute(PitSegmentsAction.INSTANCE, pitSegmentsRequest).actionGet(); assertTrue(indicesSegmentResponse.getShardFailures() == null || indicesSegmentResponse.getShardFailures().length == 0); assertEquals(indicesSegmentResponse.getIndices().isEmpty(), isEmpty); @@ -146,7 +151,7 @@ public static void assertSegments(boolean isEmpty, String index, long expectedSh } } - public static void assertSegments(boolean isEmpty, Client client) { - assertSegments(isEmpty, "index", 2, client); + public static void assertSegments(boolean isEmpty, Client client, String pitId) { + assertSegments(isEmpty, "index", 2, client, pitId); } } diff --git a/server/src/test/java/org/opensearch/search/CreatePitSingleNodeTests.java b/server/src/test/java/org/opensearch/search/CreatePitSingleNodeTests.java index 9a28f1800847e..284e018c7f8ed 100644 --- a/server/src/test/java/org/opensearch/search/CreatePitSingleNodeTests.java +++ b/server/src/test/java/org/opensearch/search/CreatePitSingleNodeTests.java @@ -32,6 +32,8 @@ import org.opensearch.index.shard.IndexShard; import org.opensearch.indices.IndicesService; +import java.util.ArrayList; +import java.util.List; import java.util.Map; import java.util.concurrent.CountDownLatch; import java.util.concurrent.ExecutionException; @@ -75,7 +77,7 @@ public void testCreatePITSuccess() throws ExecutionException, InterruptedExcepti ActionFuture execute = client().execute(CreatePitAction.INSTANCE, request); CreatePitResponse pitResponse = execute.get(); PitTestsUtil.assertUsingGetAllPits(client(), pitResponse.getId(), pitResponse.getCreationTime()); - assertSegments(false, client()); + assertSegments(false, client(), pitResponse.getId()); client().prepareIndex("index").setId("2").setSource("field", "value").setRefreshPolicy(IMMEDIATE).get(); SearchResponse searchResponse = client().prepareSearch("index") .setSize(2) @@ -88,7 +90,6 @@ public void testCreatePITSuccess() throws ExecutionException, InterruptedExcepti validatePitStats("index", 1, 0, 0); validatePitStats("index", 1, 0, 1); service.doClose(); // this kills the keep-alive reaper we have to reset the node after this test - assertSegments(true, client()); validatePitStats("index", 0, 1, 0); validatePitStats("index", 0, 1, 1); } @@ -105,14 +106,13 @@ public void testCreatePITWithMultipleIndicesSuccess() throws ExecutionException, ActionFuture execute = client().execute(CreatePitAction.INSTANCE, request); CreatePitResponse response = execute.get(); PitTestsUtil.assertUsingGetAllPits(client(), response.getId(), response.getCreationTime()); - assertSegments(false, client()); + assertSegments(false, client(), response.getId()); assertEquals(4, response.getSuccessfulShards()); assertEquals(4, service.getActiveContexts()); validatePitStats("index", 1, 0, 0); validatePitStats("index1", 1, 0, 0); service.doClose(); - assertSegments(true, client()); validatePitStats("index", 0, 1, 0); validatePitStats("index1", 0, 1, 0); } @@ -126,7 +126,7 @@ public void testCreatePITWithShardReplicasSuccess() throws ExecutionException, I ActionFuture execute = client().execute(CreatePitAction.INSTANCE, request); CreatePitResponse pitResponse = execute.get(); PitTestsUtil.assertUsingGetAllPits(client(), pitResponse.getId(), pitResponse.getCreationTime()); - assertSegments(false, client()); + assertSegments(false, client(), pitResponse.getId()); client().prepareIndex("index").setId("2").setSource("field", "value").setRefreshPolicy(IMMEDIATE).get(); SearchResponse searchResponse = client().prepareSearch("index") .setSize(2) @@ -139,7 +139,6 @@ public void testCreatePITWithShardReplicasSuccess() throws ExecutionException, I validatePitStats("index", 1, 0, 0); validatePitStats("index", 1, 0, 1); service.doClose(); - assertSegments(true, client()); validatePitStats("index", 0, 1, 0); validatePitStats("index", 0, 1, 1); } @@ -157,7 +156,6 @@ public void testCreatePITWithNonExistentIndex() { assertTrue(ex.getMessage().contains("no such index [index1]")); assertEquals(0, service.getActiveContexts()); - assertSegments(true, client()); service.doClose(); } @@ -178,7 +176,6 @@ public void testCreatePITOnCloseIndex() throws ExecutionException, InterruptedEx SearchService service = getInstanceFromNode(SearchService.class); assertEquals(0, service.getActiveContexts()); PitTestsUtil.assertGetAllPitsEmpty(client()); - assertSegments(true, client()); service.doClose(); } @@ -202,7 +199,6 @@ public void testPitSearchOnDeletedIndex() throws ExecutionException, Interrupted SearchService service = getInstanceFromNode(SearchService.class); PitTestsUtil.assertGetAllPitsEmpty(client()); assertEquals(0, service.getActiveContexts()); - assertSegments(true, client()); service.doClose(); } @@ -228,7 +224,7 @@ public void testPitSearchOnCloseIndex() throws ExecutionException, InterruptedEx ActionFuture execute = client().execute(CreatePitAction.INSTANCE, request); CreatePitResponse pitResponse = execute.get(); PitTestsUtil.assertUsingGetAllPits(client(), pitResponse.getId(), pitResponse.getCreationTime()); - assertSegments(false, client()); + assertSegments(false, client(), pitResponse.getId()); SearchService service = getInstanceFromNode(SearchService.class); assertEquals(2, service.getActiveContexts()); validatePitStats("index", 1, 0, 0); @@ -244,7 +240,6 @@ public void testPitSearchOnCloseIndex() throws ExecutionException, InterruptedEx assertTrue(ex.shardFailures()[0].reason().contains("SearchContextMissingException")); assertEquals(0, service.getActiveContexts()); PitTestsUtil.assertGetAllPitsEmpty(client()); - assertSegments(true, client()); // PIT reader contexts are lost after close, verifying it with open index api client().admin().indices().prepareOpen("index").get(); ex = expectThrows(SearchPhaseExecutionException.class, () -> { @@ -295,14 +290,16 @@ public void testCreatePitMoreThanMaxOpenPitContexts() throws Exception { CreatePitRequest request = new CreatePitRequest(TimeValue.timeValueDays(1), true); request.setIndices(new String[] { "index" }); SearchService service = getInstanceFromNode(SearchService.class); + List pitIds = new ArrayList<>(); try { for (int i = 0; i < 1000; i++) { - client().execute(CreatePitAction.INSTANCE, request).get(); + CreatePitResponse cpr = client().execute(CreatePitAction.INSTANCE, request).actionGet(); + if (cpr.getId() != null) pitIds.add(cpr.getId()); } } catch (Exception ex) { assertTrue( - ex.getMessage() + ((SearchPhaseExecutionException) ex).getDetailedMessage() .contains( "Trying to create too many Point In Time contexts. " + "Must be less than or equal to: [" @@ -315,7 +312,7 @@ public void testCreatePitMoreThanMaxOpenPitContexts() throws Exception { final int maxPitContexts = SearchService.MAX_OPEN_PIT_CONTEXT.get(Settings.EMPTY); validatePitStats("index", maxPitContexts, 0, 0); // deleteall - DeletePitRequest deletePITRequest = new DeletePitRequest("_all"); + DeletePitRequest deletePITRequest = new DeletePitRequest(pitIds.toArray(new String[pitIds.size()])); /** * When we invoke delete again, returns success after clearing the remaining readers. Asserting reader context @@ -554,7 +551,6 @@ public void testPitAfterUpdateIndex() throws Exception { assertEquals(0, service.getActiveContexts()); validatePitStats("test", 0, 1, 0); PitTestsUtil.assertGetAllPitsEmpty(client()); - assertSegments(true, client()); } } @@ -567,7 +563,7 @@ public void testConcurrentSearches() throws Exception { ActionFuture execute = client().execute(CreatePitAction.INSTANCE, request); CreatePitResponse pitResponse = execute.get(); PitTestsUtil.assertUsingGetAllPits(client(), pitResponse.getId(), pitResponse.getCreationTime()); - assertSegments(false, client()); + assertSegments(false, client(), pitResponse.getId()); Thread[] threads = new Thread[5]; CountDownLatch latch = new CountDownLatch(threads.length); @@ -603,7 +599,6 @@ public void testConcurrentSearches() throws Exception { validatePitStats("index", 0, 1, 0); validatePitStats("index", 0, 1, 1); PitTestsUtil.assertGetAllPitsEmpty(client()); - assertSegments(true, client()); } public void validatePitStats(String index, long expectedPitCurrent, long expectedPitCount, int shardId) throws ExecutionException, diff --git a/server/src/test/java/org/opensearch/search/DeletePitMultiNodeTests.java b/server/src/test/java/org/opensearch/search/DeletePitMultiNodeTests.java index e69b2cc523638..f546cf544362d 100644 --- a/server/src/test/java/org/opensearch/search/DeletePitMultiNodeTests.java +++ b/server/src/test/java/org/opensearch/search/DeletePitMultiNodeTests.java @@ -38,9 +38,7 @@ import java.util.concurrent.atomic.AtomicBoolean; import java.util.concurrent.atomic.AtomicInteger; -import static org.hamcrest.Matchers.blankOrNullString; import static org.hamcrest.Matchers.containsString; -import static org.hamcrest.Matchers.not; import static org.opensearch.action.support.WriteRequest.RefreshPolicy.IMMEDIATE; /** @@ -150,31 +148,6 @@ public void testDeletePitWithValidAndInvalidIds() throws Exception { assertThat(e.getMessage(), containsString("invalid id")); } - public void testDeleteAllPits() throws Exception { - createPitOnIndex("index"); - createIndex("index1", Settings.builder().put("index.number_of_shards", 5).put("index.number_of_replicas", 1).build()); - client().prepareIndex("index1").setId("1").setSource("field", "value").setRefreshPolicy(IMMEDIATE).execute().get(); - ensureGreen(); - createPitOnIndex("index1"); - validatePitStats("index", 5, 0); - validatePitStats("index1", 5, 0); - DeletePitRequest deletePITRequest = new DeletePitRequest("_all"); - - /** - * When we invoke delete again, returns success after clearing the remaining readers. Asserting reader context - * not found exceptions don't result in failures ( as deletion in one node is successful ) - */ - ActionFuture execute = client().execute(DeletePitAction.INSTANCE, deletePITRequest); - DeletePitResponse deletePITResponse = execute.get(); - for (DeletePitInfo deletePitInfo : deletePITResponse.getDeletePitResults()) { - assertThat(deletePitInfo.getPitId(), not(blankOrNullString())); - assertTrue(deletePitInfo.isSuccessful()); - } - validatePitStats("index", 0, 5); - validatePitStats("index1", 0, 5); - client().admin().indices().prepareDelete("index1").get(); - } - public void testDeletePitWhileNodeDrop() throws Exception { CreatePitResponse pitResponse = createPitOnIndex("index"); createIndex("index1", Settings.builder().put("index.number_of_shards", 5).put("index.number_of_replicas", 1).build()); @@ -215,38 +188,6 @@ public Settings onNodeStopped(String nodeName) throws Exception { client().admin().indices().prepareDelete("index1").get(); } - public void testDeleteAllPitsWhileNodeDrop() throws Exception { - createIndex("index1", Settings.builder().put("index.number_of_shards", 5).put("index.number_of_replicas", 1).build()); - client().prepareIndex("index1").setId("1").setSource("field", "value").setRefreshPolicy(IMMEDIATE).execute().get(); - createPitOnIndex("index1"); - ensureGreen(); - DeletePitRequest deletePITRequest = new DeletePitRequest("_all"); - internalCluster().restartRandomDataNode(new InternalTestCluster.RestartCallback() { - @Override - public Settings onNodeStopped(String nodeName) throws Exception { - ActionFuture execute = client().execute(DeletePitAction.INSTANCE, deletePITRequest); - try { - DeletePitResponse deletePITResponse = execute.get(); - for (DeletePitInfo deletePitInfo : deletePITResponse.getDeletePitResults()) { - assertThat(deletePitInfo.getPitId(), not(blankOrNullString())); - } - } catch (Exception e) { - assertTrue(e.getMessage().contains("Node not connected")); - } - return super.onNodeStopped(nodeName); - } - }); - ensureGreen(); - /** - * When we invoke delete again, returns success as all readers are cleared. (Delete all on node which is Up and - * once the node restarts, all active contexts are cleared in the node ) - */ - ActionFuture execute = client().execute(DeletePitAction.INSTANCE, deletePITRequest); - DeletePitResponse deletePITResponse = execute.get(); - assertEquals(0, deletePITResponse.getDeletePitResults().size()); - client().admin().indices().prepareDelete("index1").get(); - } - public void testDeleteWhileSearch() throws Exception { CreatePitResponse pitResponse = createPitOnIndex("index"); ensureGreen(); diff --git a/server/src/test/java/org/opensearch/search/PitMultiNodeTests.java b/server/src/test/java/org/opensearch/search/PitMultiNodeTests.java index d29ccf5b97138..921755b67246b 100644 --- a/server/src/test/java/org/opensearch/search/PitMultiNodeTests.java +++ b/server/src/test/java/org/opensearch/search/PitMultiNodeTests.java @@ -25,7 +25,7 @@ import org.opensearch.action.search.DeletePitResponse; import org.opensearch.action.search.GetAllPitNodesRequest; import org.opensearch.action.search.GetAllPitNodesResponse; -import org.opensearch.action.search.GetAllPitsAction; +import org.opensearch.action.search.NodesGetAllPitsAction; import org.opensearch.action.search.PitTestsUtil; import org.opensearch.action.search.SearchResponse; import org.opensearch.cluster.node.DiscoveryNode; @@ -86,7 +86,7 @@ public void testPit() throws Exception { assertEquals(2, searchResponse.getTotalShards()); validatePitStats("index", 2, 2); PitTestsUtil.assertUsingGetAllPits(client(), pitResponse.getId(), pitResponse.getCreationTime()); - assertSegments(false, client()); + assertSegments(false, client(), pitResponse.getId()); } public void testCreatePitWhileNodeDropWithAllowPartialCreationFalse() throws Exception { @@ -114,7 +114,7 @@ public Settings onNodeStopped(String nodeName) throws Exception { ActionFuture execute = client().execute(CreatePitAction.INSTANCE, request); CreatePitResponse pitResponse = execute.get(); PitTestsUtil.assertUsingGetAllPits(client(), pitResponse.getId(), pitResponse.getCreationTime()); - assertSegments(false, "index", 1, client()); + assertSegments(false, "index", 1, client(), pitResponse.getId()); assertEquals(1, pitResponse.getSuccessfulShards()); assertEquals(2, pitResponse.getTotalShards()); SearchResponse searchResponse = client().prepareSearch("index") @@ -368,7 +368,7 @@ public void testGetAllPits() throws Exception { DiscoveryNode[] disNodesArr = new DiscoveryNode[nodes.size()]; nodes.toArray(disNodesArr); GetAllPitNodesRequest getAllPITNodesRequest = new GetAllPitNodesRequest(disNodesArr); - ActionFuture execute1 = client().execute(GetAllPitsAction.INSTANCE, getAllPITNodesRequest); + ActionFuture execute1 = client().execute(NodesGetAllPitsAction.INSTANCE, getAllPITNodesRequest); GetAllPitNodesResponse getPitResponse = execute1.get(); assertEquals(3, getPitResponse.getPitInfos().size()); List resultPitIds = getPitResponse.getPitInfos().stream().map(p -> p.getPitId()).collect(Collectors.toList()); @@ -388,7 +388,7 @@ public void testGetAllPitsDuringNodeDrop() throws Exception { internalCluster().restartRandomDataNode(new InternalTestCluster.RestartCallback() { @Override public Settings onNodeStopped(String nodeName) throws Exception { - ActionFuture execute1 = client().execute(GetAllPitsAction.INSTANCE, getAllPITNodesRequest); + ActionFuture execute1 = client().execute(NodesGetAllPitsAction.INSTANCE, getAllPITNodesRequest); GetAllPitNodesResponse getPitResponse = execute1.get(); // we still get a pit id from the data node which is up assertEquals(1, getPitResponse.getPitInfos().size()); @@ -447,7 +447,7 @@ public void onResponse(GetAllPitNodesResponse getAllPitNodesResponse) { @Override public void onFailure(Exception e) {} }, countDownLatch); - client().execute(GetAllPitsAction.INSTANCE, getAllPITNodesRequest, listener); + client().execute(NodesGetAllPitsAction.INSTANCE, getAllPITNodesRequest, listener); } else { LatchedActionListener listener = new LatchedActionListener<>(new ActionListener() { @Override diff --git a/server/src/test/java/org/opensearch/search/pit/RestGetAllPitsActionTests.java b/server/src/test/java/org/opensearch/search/pit/RestGetAllPitsActionTests.java deleted file mode 100644 index ab21a6c0b9327..0000000000000 --- a/server/src/test/java/org/opensearch/search/pit/RestGetAllPitsActionTests.java +++ /dev/null @@ -1,38 +0,0 @@ -/* - * SPDX-License-Identifier: Apache-2.0 - * - * The OpenSearch Contributors require contributions made to - * this file be licensed under the Apache-2.0 license or a - * compatible open source license. - */ - -package org.opensearch.search.pit; - -import org.opensearch.test.OpenSearchTestCase; - -/** - * Tests for rest get all PITs action - */ -public class RestGetAllPitsActionTests extends OpenSearchTestCase { - - // public void testGetAllPits() throws Exception { - // SetOnce pitCalled = new SetOnce<>(); - // try (NodeClient nodeClient = new NoOpNodeClient(this.getTestName()) { - //// @Override - //// public void getAllPits(GetAllPitNodesRequest request, ActionListener listener) { - //// pitCalled.set(true); - //// } - // }) { - // RestGetAllPitsAction action = new RestGetAllPitsAction(new Supplier() { - // @Override - // public DiscoveryNodes get() { - // return null; - // } - // }); - // RestRequest request = new FakeRestRequest.Builder(xContentRegistry()).withPath("/_all").build(); - // FakeRestChannel channel = new FakeRestChannel(request, false, 0); - // action.handleRequest(request, channel, nodeClient); - // assertThat(pitCalled.get(), equalTo(true)); - // } - // } -} From 46f9382082a74e0130712d2310eac969e0c7c085 Mon Sep 17 00:00:00 2001 From: Bharathwaj G Date: Tue, 6 Sep 2022 20:53:27 +0530 Subject: [PATCH 09/12] modifying action name Signed-off-by: Bharathwaj G --- .../org/opensearch/action/search/NodesGetAllPitsAction.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/server/src/main/java/org/opensearch/action/search/NodesGetAllPitsAction.java b/server/src/main/java/org/opensearch/action/search/NodesGetAllPitsAction.java index af41f7d49551c..ec66dc95024c0 100644 --- a/server/src/main/java/org/opensearch/action/search/NodesGetAllPitsAction.java +++ b/server/src/main/java/org/opensearch/action/search/NodesGetAllPitsAction.java @@ -15,7 +15,7 @@ */ public class NodesGetAllPitsAction extends ActionType { public static final NodesGetAllPitsAction INSTANCE = new NodesGetAllPitsAction(); - public static final String NAME = "cluster:admin/point_in_time/read_from_nodes"; + public static final String NAME = "cluster:admin/point_in_time/readall"; private NodesGetAllPitsAction() { super(NAME, GetAllPitNodesResponse::new); From 090ccffbf1cf20de7cfb2ccd6ff1907df6dcbe4f Mon Sep 17 00:00:00 2001 From: Bharathwaj G Date: Tue, 6 Sep 2022 21:35:05 +0530 Subject: [PATCH 10/12] Addressing comments Signed-off-by: Bharathwaj G --- .../action/search/CreatePitController.java | 20 +++++++++++++ .../opensearch/action/search/ListPitInfo.java | 6 ++-- .../search/pit/RestDeletePitActionTests.java | 30 ------------------- 3 files changed, 23 insertions(+), 33 deletions(-) diff --git a/server/src/main/java/org/opensearch/action/search/CreatePitController.java b/server/src/main/java/org/opensearch/action/search/CreatePitController.java index 3d7a92ec1ed06..57c10c0632ebd 100644 --- a/server/src/main/java/org/opensearch/action/search/CreatePitController.java +++ b/server/src/main/java/org/opensearch/action/search/CreatePitController.java @@ -201,6 +201,26 @@ void executeUpdatePitId( if (node == null) { node = this.clusterService.state().getNodes().get(entry.getValue().getNode()); } + if (node == null) { + logger.error( + () -> new ParameterizedMessage( + "Create pit update phase for PIT ID [{}] failed " + "because node [{}] not found", + searchResponse.pointInTimeId(), + entry.getValue().getNode() + ) + ); + groupedActionListener.onFailure( + new OpenSearchException( + "Create pit update phase for PIT ID [" + + searchResponse.pointInTimeId() + + "] failed because node[" + + node + + "] " + + "not found" + ) + ); + return; + } try { final Transport.Connection connection = searchTransportService.getConnection(entry.getValue().getClusterAlias(), node); searchTransportService.updatePitContext( diff --git a/server/src/main/java/org/opensearch/action/search/ListPitInfo.java b/server/src/main/java/org/opensearch/action/search/ListPitInfo.java index 92a730b2b0e69..249b0a9ab3baa 100644 --- a/server/src/main/java/org/opensearch/action/search/ListPitInfo.java +++ b/server/src/main/java/org/opensearch/action/search/ListPitInfo.java @@ -65,9 +65,9 @@ public void writeTo(StreamOutput out) throws IOException { private static final ParseField PIT_ID = new ParseField("pit_id"); private static final ParseField KEEP_ALIVE = new ParseField("keep_alive"); static { - PARSER.declareString(constructorArg(), new ParseField(PIT_ID.getPreferredName())); - PARSER.declareLong(constructorArg(), new ParseField(CREATION_TIME.getPreferredName())); - PARSER.declareLong(constructorArg(), new ParseField(KEEP_ALIVE.getPreferredName())); + PARSER.declareString(constructorArg(), PIT_ID); + PARSER.declareLong(constructorArg(), CREATION_TIME); + PARSER.declareLong(constructorArg(), KEEP_ALIVE); } @Override diff --git a/server/src/test/java/org/opensearch/search/pit/RestDeletePitActionTests.java b/server/src/test/java/org/opensearch/search/pit/RestDeletePitActionTests.java index 14ecbf3495582..ecd9c23762ff5 100644 --- a/server/src/test/java/org/opensearch/search/pit/RestDeletePitActionTests.java +++ b/server/src/test/java/org/opensearch/search/pit/RestDeletePitActionTests.java @@ -90,36 +90,6 @@ public DiscoveryNodes get() { } } - public void testDeleteAllPit() throws Exception { - SetOnce pitCalled = new SetOnce<>(); - try (NodeClient nodeClient = new NoOpNodeClient(this.getTestName()) { - @Override - public void deletePits(DeletePitRequest request, ActionListener listener) { - pitCalled.set(true); - assertThat(request.getPitIds(), hasSize(1)); - assertThat(request.getPitIds().get(0), equalTo("_all")); - } - }) { - RestDeletePitAction action = new RestDeletePitAction(new Supplier() { - @Override - public DiscoveryNodes get() { - DiscoveryNode node0 = new DiscoveryNode( - "node0", - new TransportAddress(TransportAddress.META_ADDRESS, 9000), - Version.CURRENT - ); - DiscoveryNodes nodes = new DiscoveryNodes.Builder().add(node0).clusterManagerNodeId(node0.getId()).build(); - return nodes; - } - }); - RestRequest request = new FakeRestRequest.Builder(xContentRegistry()).withPath("/_all").build(); - FakeRestChannel channel = new FakeRestChannel(request, false, 0); - action.handleRequest(request, channel, nodeClient); - - assertThat(pitCalled.get(), equalTo(true)); - } - } - public void testDeleteAllPitWithBody() { SetOnce pitCalled = new SetOnce<>(); try (NodeClient nodeClient = new NoOpNodeClient(this.getTestName()) { From bb950d39498eb0c849c416425dbc63544f835b85 Mon Sep 17 00:00:00 2001 From: Bharathwaj G Date: Mon, 19 Sep 2022 20:08:10 +0530 Subject: [PATCH 11/12] changes as per new security model Signed-off-by: Bharathwaj G --- .../org/opensearch/action/ActionModule.java | 9 +- .../segments/TransportPitSegmentsAction.java | 11 +- .../action/search/GetAllPitNodesRequest.java | 11 - .../action/search/GetAllPitNodesResponse.java | 4 +- .../action/search/GetAllPitsAction.java | 2 +- .../action/search/NodesGetAllPitsAction.java | 23 -- .../opensearch/action/search/PitService.java | 4 +- .../search/TransportDeletePitAction.java | 31 +- .../search/TransportGetAllPitsAction.java | 78 ++++- .../TransportNodesGetAllPitsAction.java | 105 ------ .../java/org/opensearch/client/Client.java | 7 + .../opensearch/client/ClusterAdminClient.java | 4 - .../client/support/AbstractClient.java | 12 +- .../action/cat/RestPitSegmentsAction.java | 58 +--- .../action/search/RestDeletePitAction.java | 62 +--- .../action/search/RestGetAllPitsAction.java | 49 ++- .../action/search/PitTestsUtil.java | 20 +- .../search/TransportDeletePitActionTests.java | 314 +++++++++++++++++- .../search/CreatePitSingleNodeTests.java | 8 + .../search/DeletePitMultiNodeTests.java | 59 ++++ .../opensearch/search/PitMultiNodeTests.java | 8 +- .../search/pit/RestDeletePitActionTests.java | 76 ++--- 22 files changed, 559 insertions(+), 396 deletions(-) delete mode 100644 server/src/main/java/org/opensearch/action/search/NodesGetAllPitsAction.java delete mode 100644 server/src/main/java/org/opensearch/action/search/TransportNodesGetAllPitsAction.java diff --git a/server/src/main/java/org/opensearch/action/ActionModule.java b/server/src/main/java/org/opensearch/action/ActionModule.java index 554b006a76b61..c5fcfdd047a09 100644 --- a/server/src/main/java/org/opensearch/action/ActionModule.java +++ b/server/src/main/java/org/opensearch/action/ActionModule.java @@ -238,16 +238,14 @@ import org.opensearch.action.search.ClearScrollAction; import org.opensearch.action.search.CreatePitAction; import org.opensearch.action.search.DeletePitAction; -import org.opensearch.action.search.GetAllPitsAction; import org.opensearch.action.search.MultiSearchAction; -import org.opensearch.action.search.NodesGetAllPitsAction; +import org.opensearch.action.search.GetAllPitsAction; import org.opensearch.action.search.SearchAction; import org.opensearch.action.search.SearchScrollAction; import org.opensearch.action.search.TransportClearScrollAction; import org.opensearch.action.search.TransportCreatePitAction; import org.opensearch.action.search.TransportDeletePitAction; import org.opensearch.action.search.TransportGetAllPitsAction; -import org.opensearch.action.search.TransportNodesGetAllPitsAction; import org.opensearch.action.search.TransportMultiSearchAction; import org.opensearch.action.search.TransportSearchAction; import org.opensearch.action.search.TransportSearchScrollAction; @@ -677,10 +675,9 @@ public void reg // point in time actions actions.register(CreatePitAction.INSTANCE, TransportCreatePitAction.class); - actions.register(GetAllPitsAction.INSTANCE, TransportGetAllPitsAction.class); actions.register(DeletePitAction.INSTANCE, TransportDeletePitAction.class); actions.register(PitSegmentsAction.INSTANCE, TransportPitSegmentsAction.class); - actions.register(NodesGetAllPitsAction.INSTANCE, TransportNodesGetAllPitsAction.class); + actions.register(GetAllPitsAction.INSTANCE, TransportGetAllPitsAction.class); // Remote Store actions.register(RestoreRemoteStoreAction.INSTANCE, TransportRestoreRemoteStoreAction.class); @@ -859,7 +856,7 @@ public void initRestHandlers(Supplier nodesInCluster) { // Point in time API registerHandler.accept(new RestCreatePitAction()); - registerHandler.accept(new RestDeletePitAction(nodesInCluster)); + registerHandler.accept(new RestDeletePitAction()); registerHandler.accept(new RestGetAllPitsAction(nodesInCluster)); registerHandler.accept(new RestPitSegmentsAction(nodesInCluster)); diff --git a/server/src/main/java/org/opensearch/action/admin/indices/segments/TransportPitSegmentsAction.java b/server/src/main/java/org/opensearch/action/admin/indices/segments/TransportPitSegmentsAction.java index b1a43aa73cd97..9d4ece74a7270 100644 --- a/server/src/main/java/org/opensearch/action/admin/indices/segments/TransportPitSegmentsAction.java +++ b/server/src/main/java/org/opensearch/action/admin/indices/segments/TransportPitSegmentsAction.java @@ -8,6 +8,7 @@ package org.opensearch.action.admin.indices.segments; import org.opensearch.action.ActionListener; +import org.opensearch.action.search.ListPitInfo; import org.opensearch.action.search.PitService; import org.opensearch.action.search.SearchContextId; import org.opensearch.action.search.SearchContextIdForNode; @@ -45,6 +46,7 @@ import java.util.Collections; import java.util.List; import java.util.Map; +import java.util.stream.Collectors; import static org.opensearch.action.search.SearchContextId.decode; @@ -93,10 +95,11 @@ public TransportPitSegmentsAction( @Override protected void doExecute(Task task, PitSegmentsRequest request, ActionListener listener) { List pitIds = request.getPitIds(); - // when security plugin intercepts the request, if PITs are not present in the cluster the PIT IDs in request will be empty - // and in this case return empty response - if (pitIds.isEmpty()) { - listener.onResponse(new IndicesSegmentResponse(new ShardSegments[] {}, 0, 0, 0, new ArrayList<>())); + if (pitIds.size() == 1 && "_all".equals(pitIds.get(0))) { + pitService.getAllPits(ActionListener.wrap(response -> { + request.clearAndSetPitIds(response.getPitInfos().stream().map(ListPitInfo::getPitId).collect(Collectors.toList())); + super.doExecute(task, request, listener); + }, listener::onFailure)); } else { super.doExecute(task, request, listener); } diff --git a/server/src/main/java/org/opensearch/action/search/GetAllPitNodesRequest.java b/server/src/main/java/org/opensearch/action/search/GetAllPitNodesRequest.java index 340f9b842adbf..b4ad2f6641087 100644 --- a/server/src/main/java/org/opensearch/action/search/GetAllPitNodesRequest.java +++ b/server/src/main/java/org/opensearch/action/search/GetAllPitNodesRequest.java @@ -21,22 +21,11 @@ */ public class GetAllPitNodesRequest extends BaseNodesRequest { - // Security plugin intercepts and sets the response with permitted PIT contexts - private GetAllPitNodesResponse getAllPitNodesResponse; - @Inject public GetAllPitNodesRequest(DiscoveryNode... concreteNodes) { super(concreteNodes); } - public void setGetAllPitNodesResponse(GetAllPitNodesResponse getAllPitNodesResponse) { - this.getAllPitNodesResponse = getAllPitNodesResponse; - } - - public GetAllPitNodesResponse getGetAllPitNodesResponse() { - return getAllPitNodesResponse; - } - public GetAllPitNodesRequest(StreamInput in) throws IOException { super(in); } diff --git a/server/src/main/java/org/opensearch/action/search/GetAllPitNodesResponse.java b/server/src/main/java/org/opensearch/action/search/GetAllPitNodesResponse.java index e293ff3309e9a..610520a4c1f9d 100644 --- a/server/src/main/java/org/opensearch/action/search/GetAllPitNodesResponse.java +++ b/server/src/main/java/org/opensearch/action/search/GetAllPitNodesResponse.java @@ -68,10 +68,10 @@ public GetAllPitNodesResponse(List listPitInfos, GetAllPitNodesResp public GetAllPitNodesResponse( List listPitInfos, ClusterName clusterName, - List getAllPitNodeResponse, + List getAllPitNodeResponseList, List failures ) { - super(clusterName, getAllPitNodeResponse, failures); + super(clusterName, getAllPitNodeResponseList, failures); pitInfos.addAll(listPitInfos); } diff --git a/server/src/main/java/org/opensearch/action/search/GetAllPitsAction.java b/server/src/main/java/org/opensearch/action/search/GetAllPitsAction.java index 16e65cb785a7d..8fe901add5e3a 100644 --- a/server/src/main/java/org/opensearch/action/search/GetAllPitsAction.java +++ b/server/src/main/java/org/opensearch/action/search/GetAllPitsAction.java @@ -11,7 +11,7 @@ import org.opensearch.action.ActionType; /** - * Action type for listing all PIT reader contexts + * Action type for retrieving all PIT reader contexts from nodes */ public class GetAllPitsAction extends ActionType { public static final GetAllPitsAction INSTANCE = new GetAllPitsAction(); diff --git a/server/src/main/java/org/opensearch/action/search/NodesGetAllPitsAction.java b/server/src/main/java/org/opensearch/action/search/NodesGetAllPitsAction.java deleted file mode 100644 index ec66dc95024c0..0000000000000 --- a/server/src/main/java/org/opensearch/action/search/NodesGetAllPitsAction.java +++ /dev/null @@ -1,23 +0,0 @@ -/* - * SPDX-License-Identifier: Apache-2.0 - * - * The OpenSearch Contributors require contributions made to - * this file be licensed under the Apache-2.0 license or a - * compatible open source license. - */ - -package org.opensearch.action.search; - -import org.opensearch.action.ActionType; - -/** - * Action type for retrieving all PIT reader contexts from nodes - */ -public class NodesGetAllPitsAction extends ActionType { - public static final NodesGetAllPitsAction INSTANCE = new NodesGetAllPitsAction(); - public static final String NAME = "cluster:admin/point_in_time/readall"; - - private NodesGetAllPitsAction() { - super(NAME, GetAllPitNodesResponse::new); - } -} diff --git a/server/src/main/java/org/opensearch/action/search/PitService.java b/server/src/main/java/org/opensearch/action/search/PitService.java index 596f8525a0293..f42d84477f9a3 100644 --- a/server/src/main/java/org/opensearch/action/search/PitService.java +++ b/server/src/main/java/org/opensearch/action/search/PitService.java @@ -170,16 +170,14 @@ public Map getIndicesForPits(List pitIds) { /** * Get all active point in time contexts */ - public void getAllPits(GetAllPitNodesResponse getAllPitNodesResponse, ActionListener getAllPitsListener) { + public void getAllPits(ActionListener getAllPitsListener) { final List nodes = new ArrayList<>(); for (ObjectCursor cursor : clusterService.state().nodes().getDataNodes().values()) { DiscoveryNode node = cursor.value; nodes.add(node); } - logger.debug("Number of active PITs in cluster: " + getAllPitNodesResponse.getPitInfos().size()); DiscoveryNode[] disNodesArr = nodes.toArray(new DiscoveryNode[nodes.size()]); GetAllPitNodesRequest getAllPitNodesRequest = new GetAllPitNodesRequest(disNodesArr); - getAllPitNodesRequest.setGetAllPitNodesResponse(getAllPitNodesResponse); transportService.sendRequest( transportService.getLocalNode(), GetAllPitsAction.NAME, diff --git a/server/src/main/java/org/opensearch/action/search/TransportDeletePitAction.java b/server/src/main/java/org/opensearch/action/search/TransportDeletePitAction.java index f5ae7d79529c8..b85fe302a748f 100644 --- a/server/src/main/java/org/opensearch/action/search/TransportDeletePitAction.java +++ b/server/src/main/java/org/opensearch/action/search/TransportDeletePitAction.java @@ -11,7 +11,6 @@ import org.opensearch.action.ActionListener; import org.opensearch.action.support.ActionFilters; import org.opensearch.action.support.HandledTransportAction; -import org.opensearch.cluster.service.ClusterService; import org.opensearch.common.inject.Inject; import org.opensearch.common.io.stream.NamedWriteableRegistry; import org.opensearch.tasks.Task; @@ -21,6 +20,7 @@ import java.util.HashMap; import java.util.List; import java.util.Map; +import java.util.stream.Collectors; /** * Transport action for deleting point in time searches - supports deleting list and all point in time searches @@ -34,9 +34,6 @@ public TransportDeletePitAction( TransportService transportService, ActionFilters actionFilters, NamedWriteableRegistry namedWriteableRegistry, - TransportSearchAction transportSearchAction, - ClusterService clusterService, - SearchTransportService searchTransportService, PitService pitService ) { super(DeletePitAction.NAME, transportService, actionFilters, DeletePitRequest::new); @@ -50,10 +47,8 @@ public TransportDeletePitAction( @Override protected void doExecute(Task task, DeletePitRequest request, ActionListener listener) { List pitIds = request.getPitIds(); - // when security plugin intercepts the request, if PITs are not present in the cluster the PIT IDs in request will be empty - // and in this case return empty response - if (pitIds.isEmpty()) { - listener.onResponse(new DeletePitResponse(new ArrayList<>())); + if (pitIds.size() == 1 && "_all".equals(pitIds.get(0))) { + deleteAllPits(listener); } else { deletePits(listener, request); } @@ -75,4 +70,24 @@ private void deletePits(ActionListener listener, DeletePitReq } pitService.deletePitContexts(nodeToContextsMap, listener); } + + /** + * Delete all active PIT reader contexts leveraging list all PITs + * + * For Cross cluster PITs : + * - mixed cluster PITs ( PIT comprising local and remote ) will be fully deleted. Since there will atleast be + * one reader context with PIT ID present in local cluster, 'Get all PITs' will retrieve the PIT ID with which + * we can completely delete the PIT contexts in both local and remote cluster. + * - fully remote PITs will not be deleted as 'Get all PITs' operates on local cluster only and no PIT info can + * be retrieved when it's fully remote. + */ + private void deleteAllPits(ActionListener listener) { + // Get all PITs and execute delete operation for the PITs. + pitService.getAllPits(ActionListener.wrap(getAllPitNodesResponse -> { + DeletePitRequest deletePitRequest = new DeletePitRequest( + getAllPitNodesResponse.getPitInfos().stream().map(r -> r.getPitId()).collect(Collectors.toList()) + ); + deletePits(listener, deletePitRequest); + }, listener::onFailure)); + } } diff --git a/server/src/main/java/org/opensearch/action/search/TransportGetAllPitsAction.java b/server/src/main/java/org/opensearch/action/search/TransportGetAllPitsAction.java index 893763f6a3e01..39299f9a33b18 100644 --- a/server/src/main/java/org/opensearch/action/search/TransportGetAllPitsAction.java +++ b/server/src/main/java/org/opensearch/action/search/TransportGetAllPitsAction.java @@ -8,29 +8,79 @@ package org.opensearch.action.search; -import org.opensearch.action.ActionListener; +import org.opensearch.action.FailedNodeException; import org.opensearch.action.support.ActionFilters; -import org.opensearch.action.support.HandledTransportAction; +import org.opensearch.action.support.nodes.TransportNodesAction; +import org.opensearch.cluster.service.ClusterService; import org.opensearch.common.inject.Inject; -import org.opensearch.tasks.Task; +import org.opensearch.common.io.stream.StreamInput; +import org.opensearch.search.SearchService; +import org.opensearch.threadpool.ThreadPool; import org.opensearch.transport.TransportService; +import java.io.IOException; +import java.util.List; + /** - * Transport action to get all active PIT contexts across the cluster + * Transport action to get all active PIT contexts across all nodes */ -public class TransportGetAllPitsAction extends HandledTransportAction { - private final PitService pitService; +public class TransportGetAllPitsAction extends TransportNodesAction< + GetAllPitNodesRequest, + GetAllPitNodesResponse, + GetAllPitNodeRequest, + GetAllPitNodeResponse> { + private final SearchService searchService; @Inject - public TransportGetAllPitsAction(ActionFilters actionFilters, TransportService transportService, PitService pitService) { - super(GetAllPitsAction.NAME, transportService, actionFilters, in -> new GetAllPitNodesRequest(in)); - this.pitService = pitService; + public TransportGetAllPitsAction( + ThreadPool threadPool, + ClusterService clusterService, + TransportService transportService, + ActionFilters actionFilters, + SearchService searchService + ) { + super( + GetAllPitsAction.NAME, + threadPool, + clusterService, + transportService, + actionFilters, + GetAllPitNodesRequest::new, + GetAllPitNodeRequest::new, + ThreadPool.Names.SAME, + GetAllPitNodeResponse.class + ); + this.searchService = searchService; + } + + @Override + protected GetAllPitNodesResponse newResponse( + GetAllPitNodesRequest request, + List getAllPitNodeResponses, + List failures + ) { + return new GetAllPitNodesResponse(clusterService.getClusterName(), getAllPitNodeResponses, failures); + } + + @Override + protected GetAllPitNodeRequest newNodeRequest(GetAllPitNodesRequest request) { + return new GetAllPitNodeRequest(); + } + + @Override + protected GetAllPitNodeResponse newNodeResponse(StreamInput in) throws IOException { + return new GetAllPitNodeResponse(in); } - protected void doExecute(Task task, GetAllPitNodesRequest request, ActionListener listener) { - // If security plugin intercepts the request, it'll replace all PIT IDs with permitted PIT IDs - if (request.getGetAllPitNodesResponse() != null) { - listener.onResponse(request.getGetAllPitNodesResponse()); - } + /** + * This retrieves all active PITs in the node + */ + @Override + protected GetAllPitNodeResponse nodeOperation(GetAllPitNodeRequest request) { + GetAllPitNodeResponse nodeResponse = new GetAllPitNodeResponse( + transportService.getLocalNode(), + searchService.getAllPITReaderContexts() + ); + return nodeResponse; } } diff --git a/server/src/main/java/org/opensearch/action/search/TransportNodesGetAllPitsAction.java b/server/src/main/java/org/opensearch/action/search/TransportNodesGetAllPitsAction.java deleted file mode 100644 index 39f7325f62409..0000000000000 --- a/server/src/main/java/org/opensearch/action/search/TransportNodesGetAllPitsAction.java +++ /dev/null @@ -1,105 +0,0 @@ -/* - * SPDX-License-Identifier: Apache-2.0 - * - * The OpenSearch Contributors require contributions made to - * this file be licensed under the Apache-2.0 license or a - * compatible open source license. - */ - -package org.opensearch.action.search; - -import org.opensearch.action.ActionListener; -import org.opensearch.action.FailedNodeException; -import org.opensearch.action.support.ActionFilters; -import org.opensearch.action.support.nodes.TransportNodesAction; -import org.opensearch.cluster.service.ClusterService; -import org.opensearch.common.inject.Inject; -import org.opensearch.common.io.stream.StreamInput; -import org.opensearch.search.SearchService; -import org.opensearch.tasks.Task; -import org.opensearch.threadpool.ThreadPool; -import org.opensearch.transport.TransportService; - -import java.io.IOException; -import java.util.List; - -/** - * Transport action to get all active PIT contexts across all nodes - */ -public class TransportNodesGetAllPitsAction extends TransportNodesAction< - GetAllPitNodesRequest, - GetAllPitNodesResponse, - GetAllPitNodeRequest, - GetAllPitNodeResponse> { - private final SearchService searchService; - - private final PitService pitService; - - @Inject - public TransportNodesGetAllPitsAction( - ThreadPool threadPool, - ClusterService clusterService, - TransportService transportService, - ActionFilters actionFilters, - SearchService searchService, - PitService pitService - ) { - super( - NodesGetAllPitsAction.NAME, - threadPool, - clusterService, - transportService, - actionFilters, - GetAllPitNodesRequest::new, - GetAllPitNodeRequest::new, - ThreadPool.Names.SAME, - GetAllPitNodeResponse.class - ); - this.searchService = searchService; - this.pitService = pitService; - } - - @Override - protected GetAllPitNodesResponse newResponse( - GetAllPitNodesRequest request, - List getAllPitNodeResponses, - List failures - ) { - return new GetAllPitNodesResponse(clusterService.getClusterName(), getAllPitNodeResponses, failures); - } - - @Override - protected GetAllPitNodeRequest newNodeRequest(GetAllPitNodesRequest request) { - return new GetAllPitNodeRequest(); - } - - @Override - protected GetAllPitNodeResponse newNodeResponse(StreamInput in) throws IOException { - return new GetAllPitNodeResponse(in); - } - - /** - * This retrieves all active PITs in the node - */ - @Override - protected GetAllPitNodeResponse nodeOperation(GetAllPitNodeRequest request) { - GetAllPitNodeResponse nodeResponse = new GetAllPitNodeResponse( - transportService.getLocalNode(), - searchService.getAllPITReaderContexts() - ); - return nodeResponse; - } - - /** - * Call list all pits of 'indices:data:read' type so that PITs get filtered in security plugin based - * user indices permission - */ - @Override - protected void doExecute(Task task, GetAllPitNodesRequest request, ActionListener listener) { - super.doExecute( - task, - request, - ActionListener.wrap(response -> { pitService.getAllPits(response, listener); }, e -> { listener.onFailure(e); }) - ); - } -} diff --git a/server/src/main/java/org/opensearch/client/Client.java b/server/src/main/java/org/opensearch/client/Client.java index 94043d5c3c89f..f20f0b4246cb6 100644 --- a/server/src/main/java/org/opensearch/client/Client.java +++ b/server/src/main/java/org/opensearch/client/Client.java @@ -64,6 +64,8 @@ import org.opensearch.action.search.CreatePitResponse; import org.opensearch.action.search.DeletePitRequest; import org.opensearch.action.search.DeletePitResponse; +import org.opensearch.action.search.GetAllPitNodesRequest; +import org.opensearch.action.search.GetAllPitNodesResponse; import org.opensearch.action.search.MultiSearchRequest; import org.opensearch.action.search.MultiSearchRequestBuilder; import org.opensearch.action.search.MultiSearchResponse; @@ -341,6 +343,11 @@ public interface Client extends OpenSearchClient, Releasable { */ void deletePits(DeletePitRequest deletePITRequest, ActionListener listener); + /** + * Get all active point in time searches + */ + void getAllPits(GetAllPitNodesRequest getAllPitNodesRequest, ActionListener listener); + /** * Get information of segments of one or more PITs */ diff --git a/server/src/main/java/org/opensearch/client/ClusterAdminClient.java b/server/src/main/java/org/opensearch/client/ClusterAdminClient.java index a1d618d5b7d18..7a7b98bf724f6 100644 --- a/server/src/main/java/org/opensearch/client/ClusterAdminClient.java +++ b/server/src/main/java/org/opensearch/client/ClusterAdminClient.java @@ -132,8 +132,6 @@ import org.opensearch.action.ingest.SimulatePipelineRequest; import org.opensearch.action.ingest.SimulatePipelineRequestBuilder; import org.opensearch.action.ingest.SimulatePipelineResponse; -import org.opensearch.action.search.GetAllPitNodesRequest; -import org.opensearch.action.search.GetAllPitNodesResponse; import org.opensearch.action.support.master.AcknowledgedResponse; import org.opensearch.common.bytes.BytesReference; import org.opensearch.common.xcontent.XContentType; @@ -342,8 +340,6 @@ public interface ClusterAdminClient extends OpenSearchClient { */ NodesHotThreadsRequestBuilder prepareNodesHotThreads(String... nodesIds); - void listAllPits(GetAllPitNodesRequest request, ActionListener listener); - /** * List tasks * diff --git a/server/src/main/java/org/opensearch/client/support/AbstractClient.java b/server/src/main/java/org/opensearch/client/support/AbstractClient.java index c4149a9840450..21cd01bf65a45 100644 --- a/server/src/main/java/org/opensearch/client/support/AbstractClient.java +++ b/server/src/main/java/org/opensearch/client/support/AbstractClient.java @@ -341,7 +341,7 @@ import org.opensearch.action.search.MultiSearchRequest; import org.opensearch.action.search.MultiSearchRequestBuilder; import org.opensearch.action.search.MultiSearchResponse; -import org.opensearch.action.search.NodesGetAllPitsAction; +import org.opensearch.action.search.GetAllPitsAction; import org.opensearch.action.search.SearchAction; import org.opensearch.action.search.SearchRequest; import org.opensearch.action.search.SearchRequestBuilder; @@ -598,6 +598,11 @@ public void deletePits(final DeletePitRequest deletePITRequest, final ActionList execute(DeletePitAction.INSTANCE, deletePITRequest, listener); } + @Override + public void getAllPits(final GetAllPitNodesRequest getAllPitNodesRequest, final ActionListener listener) { + execute(GetAllPitsAction.INSTANCE, getAllPitNodesRequest, listener); + } + @Override public void pitSegments(final PitSegmentsRequest request, final ActionListener listener) { execute(PitSegmentsAction.INSTANCE, request, listener); @@ -892,11 +897,6 @@ public NodesHotThreadsRequestBuilder prepareNodesHotThreads(String... nodesIds) return new NodesHotThreadsRequestBuilder(this, NodesHotThreadsAction.INSTANCE).setNodesIds(nodesIds); } - @Override - public void listAllPits(GetAllPitNodesRequest request, ActionListener listener) { - execute(NodesGetAllPitsAction.INSTANCE, request, listener); - } - @Override public ActionFuture listTasks(final ListTasksRequest request) { return execute(ListTasksAction.INSTANCE, request); diff --git a/server/src/main/java/org/opensearch/rest/action/cat/RestPitSegmentsAction.java b/server/src/main/java/org/opensearch/rest/action/cat/RestPitSegmentsAction.java index c5432f9157b4a..ba9606e8eb444 100644 --- a/server/src/main/java/org/opensearch/rest/action/cat/RestPitSegmentsAction.java +++ b/server/src/main/java/org/opensearch/rest/action/cat/RestPitSegmentsAction.java @@ -14,28 +14,20 @@ import org.opensearch.action.admin.indices.segments.PitSegmentsAction; import org.opensearch.action.admin.indices.segments.PitSegmentsRequest; import org.opensearch.action.admin.indices.segments.ShardSegments; -import org.opensearch.action.search.GetAllPitNodesRequest; -import org.opensearch.action.search.GetAllPitNodesResponse; import org.opensearch.client.node.NodeClient; -import org.opensearch.cluster.node.DiscoveryNode; import org.opensearch.cluster.node.DiscoveryNodes; import org.opensearch.common.Table; -import org.opensearch.common.logging.DeprecationLogger; import org.opensearch.index.engine.Segment; import org.opensearch.rest.BaseRestHandler; import org.opensearch.rest.RestHandler; import org.opensearch.rest.RestRequest; import org.opensearch.rest.RestResponse; -import org.opensearch.rest.action.RestActionListener; import org.opensearch.rest.action.RestResponseListener; import java.io.IOException; -import java.util.ArrayList; -import java.util.HashMap; import java.util.List; import java.util.Map; import java.util.function.Supplier; -import java.util.stream.Collectors; import static java.util.Arrays.asList; import static java.util.Collections.unmodifiableList; @@ -45,9 +37,6 @@ * Rest action for pit segments */ public class RestPitSegmentsAction extends AbstractCatAction { - - private static final DeprecationLogger deprecationLogger = DeprecationLogger.getLogger(RestPitSegmentsAction.class); - private final Supplier nodesInCluster; public RestPitSegmentsAction(Supplier nodesInCluster) { @@ -88,47 +77,14 @@ protected BaseRestHandler.RestChannelConsumer doCatRequest(final RestRequest req throw new IllegalArgumentException("Failed to parse request body", e); } } - if (request.path().contains(allPitIdsQualifier)) { - final List nodes = new ArrayList<>(); - for (DiscoveryNode node : nodesInCluster.get()) { - nodes.add(node); + return channel -> client.execute(PitSegmentsAction.INSTANCE, pitSegmentsRequest, new RestResponseListener<>(channel) { + @Override + public RestResponse buildResponse(final IndicesSegmentResponse indicesSegmentResponse) throws Exception { + final Map indicesSegments = indicesSegmentResponse.getIndices(); + Table tab = buildTable(request, indicesSegments); + return RestTable.buildResponse(tab, channel); } - DiscoveryNode[] disNodesArr = nodes.toArray(new DiscoveryNode[nodes.size()]); - GetAllPitNodesRequest getAllPitNodesRequest = new GetAllPitNodesRequest(disNodesArr); - return channel -> client.admin() - .cluster() - .listAllPits(getAllPitNodesRequest, new RestActionListener(channel) { - @Override - protected void processResponse(GetAllPitNodesResponse getAllPitNodesResponse) throws Exception { - if (getAllPitNodesResponse.getPitInfos().size() == 0) { - final Map indicesSegments = new HashMap<>(); - Table tab = buildTable(request, indicesSegments); - channel.sendResponse(RestTable.buildResponse(tab, channel)); - return; - } - pitSegmentsRequest.clearAndSetPitIds( - getAllPitNodesResponse.getPitInfos().stream().map(r -> r.getPitId()).collect(Collectors.toList()) - ); - client.execute(PitSegmentsAction.INSTANCE, pitSegmentsRequest, new RestResponseListener<>(channel) { - @Override - public RestResponse buildResponse(final IndicesSegmentResponse indicesSegmentResponse) throws Exception { - final Map indicesSegments = indicesSegmentResponse.getIndices(); - Table tab = buildTable(request, indicesSegments); - return RestTable.buildResponse(tab, channel); - } - }); - } - }); - } else { - return channel -> client.execute(PitSegmentsAction.INSTANCE, pitSegmentsRequest, new RestResponseListener<>(channel) { - @Override - public RestResponse buildResponse(final IndicesSegmentResponse indicesSegmentResponse) throws Exception { - final Map indicesSegments = indicesSegmentResponse.getIndices(); - Table tab = buildTable(request, indicesSegments); - return RestTable.buildResponse(tab, channel); - } - }); - } + }); } @Override diff --git a/server/src/main/java/org/opensearch/rest/action/search/RestDeletePitAction.java b/server/src/main/java/org/opensearch/rest/action/search/RestDeletePitAction.java index 30fcc7a88338f..b19a7505741cc 100644 --- a/server/src/main/java/org/opensearch/rest/action/search/RestDeletePitAction.java +++ b/server/src/main/java/org/opensearch/rest/action/search/RestDeletePitAction.java @@ -8,24 +8,15 @@ package org.opensearch.rest.action.search; -import org.opensearch.action.ActionListener; import org.opensearch.action.search.DeletePitRequest; import org.opensearch.action.search.DeletePitResponse; -import org.opensearch.action.search.GetAllPitNodesRequest; -import org.opensearch.action.search.GetAllPitNodesResponse; import org.opensearch.client.node.NodeClient; -import org.opensearch.cluster.node.DiscoveryNode; -import org.opensearch.cluster.node.DiscoveryNodes; import org.opensearch.rest.BaseRestHandler; import org.opensearch.rest.RestRequest; -import org.opensearch.rest.action.RestActionListener; import org.opensearch.rest.action.RestStatusToXContentListener; import java.io.IOException; -import java.util.ArrayList; import java.util.List; -import java.util.function.Supplier; -import java.util.stream.Collectors; import static java.util.Arrays.asList; import static java.util.Collections.unmodifiableList; @@ -35,14 +26,6 @@ * Rest action for deleting PIT contexts */ public class RestDeletePitAction extends BaseRestHandler { - - private final Supplier nodesInCluster; - - public RestDeletePitAction(Supplier nodesInCluster) { - super(); - this.nodesInCluster = nodesInCluster; - } - @Override public String getName() { return "delete_pit_action"; @@ -66,50 +49,7 @@ public RestChannelConsumer prepareRequest(RestRequest request, NodeClient client } })); } - if (request.path().contains(allPitIdsQualifier)) { - final List nodes = new ArrayList<>(); - for (DiscoveryNode node : nodesInCluster.get()) { - nodes.add(node); - } - DiscoveryNode[] disNodesArr = nodes.toArray(new DiscoveryNode[nodes.size()]); - GetAllPitNodesRequest getAllPitNodesRequest = new GetAllPitNodesRequest(disNodesArr); - return deleteAllPits(client, getAllPitNodesRequest, deletePITRequest); - } else { - return channel -> client.deletePits(deletePITRequest, new RestStatusToXContentListener(channel)); - } - } - - /** - * Delete all active PIT reader contexts leveraging list all PITs - * - * For Cross cluster PITs : - * - mixed cluster PITs ( PIT comprising local and remote ) will be fully deleted. Since there will atleast be - * one reader context with PIT ID present in local cluster, 'Get all PITs' will retrieve the PIT ID with which - * we can completely delete the PIT contexts in both local and remote cluster. - * - fully remote PITs will not be deleted as 'Get all PITs' operates on local cluster only and no PIT info can - * be retrieved when it's fully remote. - */ - private RestChannelConsumer deleteAllPits( - NodeClient client, - GetAllPitNodesRequest getAllPitNodesRequest, - DeletePitRequest deletePitRequest - ) { - return channel -> client.admin().cluster().listAllPits(getAllPitNodesRequest, new RestActionListener<>(channel) { - @Override - public void processResponse(final GetAllPitNodesResponse getAllPitNodesResponse) throws IOException { - // return empty response when no PITs are retrieved - if (getAllPitNodesResponse.getPitInfos().size() == 0) { - DeletePitResponse response = new DeletePitResponse(new ArrayList<>()); - ActionListener listener = new RestStatusToXContentListener(channel); - listener.onResponse(response); - return; - } - deletePitRequest.clearAndSetPitIds( - getAllPitNodesResponse.getPitInfos().stream().map(r -> r.getPitId()).collect(Collectors.toList()) - ); - client.deletePits(deletePitRequest, new RestStatusToXContentListener(channel)); - } - }); + return channel -> client.deletePits(deletePITRequest, new RestStatusToXContentListener(channel)); } @Override diff --git a/server/src/main/java/org/opensearch/rest/action/search/RestGetAllPitsAction.java b/server/src/main/java/org/opensearch/rest/action/search/RestGetAllPitsAction.java index 178c467dfec3f..0e1febe9d2a61 100644 --- a/server/src/main/java/org/opensearch/rest/action/search/RestGetAllPitsAction.java +++ b/server/src/main/java/org/opensearch/rest/action/search/RestGetAllPitsAction.java @@ -56,38 +56,35 @@ protected RestChannelConsumer prepareRequest(RestRequest request, NodeClient cli } DiscoveryNode[] disNodesArr = nodes.toArray(new DiscoveryNode[nodes.size()]); GetAllPitNodesRequest getAllPitNodesRequest = new GetAllPitNodesRequest(disNodesArr); - return channel -> client.admin() - .cluster() - .listAllPits(getAllPitNodesRequest, new RestBuilderListener(channel) { - @Override - public RestResponse buildResponse(final GetAllPitNodesResponse getAllPITNodesResponse, XContentBuilder builder) - throws Exception { - builder.startObject(); - if (getAllPITNodesResponse.hasFailures()) { - builder.startArray("failures"); - for (int idx = 0; idx < getAllPITNodesResponse.failures().size(); idx++) { - builder.startObject(); - builder.field( - getAllPITNodesResponse.failures().get(idx).nodeId(), - getAllPITNodesResponse.failures().get(idx).getDetailedMessage() - ); - builder.endObject(); - } - builder.endArray(); + return channel -> client.getAllPits(getAllPitNodesRequest, new RestBuilderListener(channel) { + @Override + public RestResponse buildResponse(final GetAllPitNodesResponse getAllPITNodesResponse, XContentBuilder builder) + throws Exception { + builder.startObject(); + if (getAllPITNodesResponse.hasFailures()) { + builder.startArray("failures"); + for (int idx = 0; idx < getAllPITNodesResponse.failures().size(); idx++) { + builder.startObject(); + builder.field( + getAllPITNodesResponse.failures().get(idx).nodeId(), + getAllPITNodesResponse.failures().get(idx).getDetailedMessage() + ); + builder.endObject(); } - builder.field("pits", getAllPITNodesResponse.getPitInfos()); - builder.endObject(); - if (getAllPITNodesResponse.hasFailures() && getAllPITNodesResponse.getPitInfos().isEmpty()) { - return new BytesRestResponse(RestStatus.INTERNAL_SERVER_ERROR, builder); - } - return new BytesRestResponse(RestStatus.OK, builder); + builder.endArray(); + } + builder.field("pits", getAllPITNodesResponse.getPitInfos()); + builder.endObject(); + if (getAllPITNodesResponse.hasFailures() && getAllPITNodesResponse.getPitInfos().isEmpty()) { + return new BytesRestResponse(RestStatus.INTERNAL_SERVER_ERROR, builder); } - }); + return new BytesRestResponse(RestStatus.OK, builder); + } + }); } @Override public List routes() { return unmodifiableList(Collections.singletonList(new Route(GET, "/_search/point_in_time/_all"))); } - } diff --git a/server/src/test/java/org/opensearch/action/search/PitTestsUtil.java b/server/src/test/java/org/opensearch/action/search/PitTestsUtil.java index f632db0b69748..3962a4a11fc90 100644 --- a/server/src/test/java/org/opensearch/action/search/PitTestsUtil.java +++ b/server/src/test/java/org/opensearch/action/search/PitTestsUtil.java @@ -111,7 +111,7 @@ public static void assertUsingGetAllPits(Client client, String id, long creation DiscoveryNode[] disNodesArr = new DiscoveryNode[nodes.size()]; nodes.toArray(disNodesArr); GetAllPitNodesRequest getAllPITNodesRequest = new GetAllPitNodesRequest(disNodesArr); - ActionFuture execute1 = client.execute(NodesGetAllPitsAction.INSTANCE, getAllPITNodesRequest); + ActionFuture execute1 = client.execute(GetAllPitsAction.INSTANCE, getAllPITNodesRequest); GetAllPitNodesResponse getPitResponse = execute1.get(); assertTrue(getPitResponse.getPitInfos().get(0).getPitId().contains(id)); Assert.assertEquals(getPitResponse.getPitInfos().get(0).getCreationTime(), creationTime); @@ -130,7 +130,7 @@ public static void assertGetAllPitsEmpty(Client client) throws ExecutionExceptio DiscoveryNode[] disNodesArr = new DiscoveryNode[nodes.size()]; nodes.toArray(disNodesArr); GetAllPitNodesRequest getAllPITNodesRequest = new GetAllPitNodesRequest(disNodesArr); - ActionFuture execute1 = client.execute(NodesGetAllPitsAction.INSTANCE, getAllPITNodesRequest); + ActionFuture execute1 = client.execute(GetAllPitsAction.INSTANCE, getAllPITNodesRequest); GetAllPitNodesResponse getPitResponse = execute1.get(); Assert.assertEquals(0, getPitResponse.getPitInfos().size()); } @@ -151,6 +151,22 @@ public static void assertSegments(boolean isEmpty, String index, long expectedSh } } + public static void assertSegments(boolean isEmpty, String index, long expectedShardSize, Client client) { + PitSegmentsRequest pitSegmentsRequest = new PitSegmentsRequest("_all"); + IndicesSegmentResponse indicesSegmentResponse = client.execute(PitSegmentsAction.INSTANCE, pitSegmentsRequest).actionGet(); + assertTrue(indicesSegmentResponse.getShardFailures() == null || indicesSegmentResponse.getShardFailures().length == 0); + assertEquals(indicesSegmentResponse.getIndices().isEmpty(), isEmpty); + if (!isEmpty) { + assertTrue(indicesSegmentResponse.getIndices().get(index) != null); + assertTrue(indicesSegmentResponse.getIndices().get(index).getIndex().equalsIgnoreCase(index)); + assertEquals(expectedShardSize, indicesSegmentResponse.getIndices().get(index).getShards().size()); + } + } + + public static void assertSegments(boolean isEmpty, Client client) { + assertSegments(isEmpty, "index", 2, client); + } + public static void assertSegments(boolean isEmpty, Client client, String pitId) { assertSegments(isEmpty, "index", 2, client, pitId); } diff --git a/server/src/test/java/org/opensearch/action/search/TransportDeletePitActionTests.java b/server/src/test/java/org/opensearch/action/search/TransportDeletePitActionTests.java index 2bf044d350679..d6de562d616fa 100644 --- a/server/src/test/java/org/opensearch/action/search/TransportDeletePitActionTests.java +++ b/server/src/test/java/org/opensearch/action/search/TransportDeletePitActionTests.java @@ -14,6 +14,7 @@ import org.opensearch.action.support.ActionFilters; import org.opensearch.action.support.PlainActionFuture; import org.opensearch.client.node.NodeClient; +import org.opensearch.cluster.ClusterName; import org.opensearch.cluster.ClusterState; import org.opensearch.cluster.metadata.Metadata; import org.opensearch.cluster.node.DiscoveryNode; @@ -171,9 +172,6 @@ public Transport.Connection getConnection(String clusterAlias, DiscoveryNode nod transportService, actionFilters, namedWriteableRegistry, - transportSearchAction, - clusterServiceMock, - searchTransportService, pitService ); DeletePitRequest deletePITRequest = new DeletePitRequest(pitId); @@ -188,6 +186,81 @@ public Transport.Connection getConnection(String clusterAlias, DiscoveryNode nod } } + public void testDeleteAllPITSuccess() throws InterruptedException, ExecutionException { + List deleteNodesInvoked = new CopyOnWriteArrayList<>(); + ActionFilters actionFilters = mock(ActionFilters.class); + when(actionFilters.filters()).thenReturn(new ActionFilter[0]); + List knownNodes = new CopyOnWriteArrayList<>(); + try ( + MockTransportService cluster1Transport = startTransport("cluster_1_node", knownNodes, Version.CURRENT); + MockTransportService cluster2Transport = startTransport("cluster_2_node", knownNodes, Version.CURRENT) + ) { + knownNodes.add(cluster1Transport.getLocalDiscoNode()); + knownNodes.add(cluster2Transport.getLocalDiscoNode()); + Collections.shuffle(knownNodes, random()); + + try ( + MockTransportService transportService = MockTransportService.createNewService( + Settings.EMPTY, + Version.CURRENT, + threadPool, + null + ) + ) { + transportService.start(); + transportService.acceptIncomingRequests(); + SearchTransportService searchTransportService = new SearchTransportService(transportService, null) { + public void sendFreePITContexts( + Transport.Connection connection, + List contextIds, + final ActionListener listener + ) { + deleteNodesInvoked.add(connection.getNode()); + DeletePitInfo deletePitInfo = new DeletePitInfo(true, "pitId"); + List deletePitInfos = new ArrayList<>(); + deletePitInfos.add(deletePitInfo); + Thread t = new Thread(() -> listener.onResponse(new DeletePitResponse(deletePitInfos))); + t.start(); + } + + @Override + public Transport.Connection getConnection(String clusterAlias, DiscoveryNode node) { + return new SearchAsyncActionTests.MockConnection(node); + } + }; + PitService pitService = new PitService(clusterServiceMock, searchTransportService, transportService, client) { + @Override + public void getAllPits(ActionListener getAllPitsListener) { + ListPitInfo listPitInfo = new ListPitInfo(getPitId(), 0, 0); + List list = new ArrayList<>(); + list.add(listPitInfo); + GetAllPitNodeResponse getAllPitNodeResponse = new GetAllPitNodeResponse( + cluster1Transport.getLocalDiscoNode(), + list + ); + List nodeList = new ArrayList(); + nodeList.add(getAllPitNodeResponse); + getAllPitsListener.onResponse(new GetAllPitNodesResponse(new ClusterName("cn"), nodeList, new ArrayList())); + } + }; + TransportDeletePitAction action = new TransportDeletePitAction( + transportService, + actionFilters, + namedWriteableRegistry, + pitService + ); + DeletePitRequest deletePITRequest = new DeletePitRequest("_all"); + PlainActionFuture future = newFuture(); + action.execute(task, deletePITRequest, future); + DeletePitResponse dr = future.get(); + assertTrue(dr.getDeletePitResults().get(0).getPitId().equals("pitId")); + assertTrue(dr.getDeletePitResults().get(0).isSuccessful()); + assertEquals(3, deleteNodesInvoked.size()); + + } + } + } + public void testDeletePitWhenNodeIsDown() throws InterruptedException, ExecutionException { List deleteNodesInvoked = new CopyOnWriteArrayList<>(); ActionFilters actionFilters = mock(ActionFilters.class); @@ -240,9 +313,6 @@ public Transport.Connection getConnection(String clusterAlias, DiscoveryNode nod transportService, actionFilters, namedWriteableRegistry, - transportSearchAction, - clusterServiceMock, - searchTransportService, pitService ); DeletePitRequest deletePITRequest = new DeletePitRequest(pitId); @@ -299,9 +369,6 @@ public Transport.Connection getConnection(String clusterAlias, DiscoveryNode nod transportService, actionFilters, namedWriteableRegistry, - transportSearchAction, - clusterServiceMock, - searchTransportService, pitService ); DeletePitRequest deletePITRequest = new DeletePitRequest(pitId); @@ -367,9 +434,6 @@ public Transport.Connection getConnection(String clusterAlias, DiscoveryNode nod transportService, actionFilters, namedWriteableRegistry, - transportSearchAction, - clusterServiceMock, - searchTransportService, pitService ); DeletePitRequest deletePITRequest = new DeletePitRequest(pitId); @@ -382,4 +446,230 @@ public Transport.Connection getConnection(String clusterAlias, DiscoveryNode nod } } + public void testDeleteAllPitWhenNodeIsDown() { + List deleteNodesInvoked = new CopyOnWriteArrayList<>(); + ActionFilters actionFilters = mock(ActionFilters.class); + when(actionFilters.filters()).thenReturn(new ActionFilter[0]); + + List knownNodes = new CopyOnWriteArrayList<>(); + try ( + MockTransportService cluster1Transport = startTransport("cluster_1_node", knownNodes, Version.CURRENT); + MockTransportService cluster2Transport = startTransport("cluster_2_node", knownNodes, Version.CURRENT) + ) { + knownNodes.add(cluster1Transport.getLocalDiscoNode()); + knownNodes.add(cluster2Transport.getLocalDiscoNode()); + Collections.shuffle(knownNodes, random()); + + try ( + MockTransportService transportService = MockTransportService.createNewService( + Settings.EMPTY, + Version.CURRENT, + threadPool, + null + ) + ) { + transportService.start(); + transportService.acceptIncomingRequests(); + SearchTransportService searchTransportService = new SearchTransportService(transportService, null) { + @Override + public void sendFreePITContexts( + Transport.Connection connection, + List contextIds, + final ActionListener listener + ) { + deleteNodesInvoked.add(connection.getNode()); + if (connection.getNode().getId() == "node_3") { + Thread t = new Thread(() -> listener.onFailure(new Exception("node 3 down"))); + t.start(); + } else { + Thread t = new Thread(() -> listener.onResponse(new DeletePitResponse(new ArrayList<>()))); + t.start(); + } + } + + @Override + public Transport.Connection getConnection(String clusterAlias, DiscoveryNode node) { + return new SearchAsyncActionTests.MockConnection(node); + } + }; + PitService pitService = new PitService(clusterServiceMock, searchTransportService, transportService, client) { + @Override + public void getAllPits(ActionListener getAllPitsListener) { + ListPitInfo listPitInfo = new ListPitInfo(getPitId(), 0, 0); + List list = new ArrayList<>(); + list.add(listPitInfo); + GetAllPitNodeResponse getAllPitNodeResponse = new GetAllPitNodeResponse( + cluster1Transport.getLocalDiscoNode(), + list + ); + List nodeList = new ArrayList(); + nodeList.add(getAllPitNodeResponse); + getAllPitsListener.onResponse(new GetAllPitNodesResponse(new ClusterName("cn"), nodeList, new ArrayList())); + } + }; + TransportDeletePitAction action = new TransportDeletePitAction( + transportService, + actionFilters, + namedWriteableRegistry, + pitService + ); + DeletePitRequest deletePITRequest = new DeletePitRequest("_all"); + PlainActionFuture future = newFuture(); + action.execute(task, deletePITRequest, future); + Exception e = assertThrows(ExecutionException.class, () -> future.get()); + assertThat(e.getMessage(), containsString("node 3 down")); + assertEquals(3, deleteNodesInvoked.size()); + } + } + } + + public void testDeleteAllPitWhenAllNodesAreDown() { + List deleteNodesInvoked = new CopyOnWriteArrayList<>(); + ActionFilters actionFilters = mock(ActionFilters.class); + when(actionFilters.filters()).thenReturn(new ActionFilter[0]); + + List knownNodes = new CopyOnWriteArrayList<>(); + try ( + MockTransportService cluster1Transport = startTransport("cluster_1_node", knownNodes, Version.CURRENT); + MockTransportService cluster2Transport = startTransport("cluster_2_node", knownNodes, Version.CURRENT) + ) { + knownNodes.add(cluster1Transport.getLocalDiscoNode()); + knownNodes.add(cluster2Transport.getLocalDiscoNode()); + Collections.shuffle(knownNodes, random()); + + try ( + MockTransportService transportService = MockTransportService.createNewService( + Settings.EMPTY, + Version.CURRENT, + threadPool, + null + ) + ) { + transportService.start(); + transportService.acceptIncomingRequests(); + SearchTransportService searchTransportService = new SearchTransportService(transportService, null) { + + @Override + public void sendFreePITContexts( + Transport.Connection connection, + List contextIds, + final ActionListener listener + ) { + deleteNodesInvoked.add(connection.getNode()); + Thread t = new Thread(() -> listener.onFailure(new Exception("node down"))); + t.start(); + } + + @Override + public Transport.Connection getConnection(String clusterAlias, DiscoveryNode node) { + return new SearchAsyncActionTests.MockConnection(node); + } + }; + PitService pitService = new PitService(clusterServiceMock, searchTransportService, transportService, client) { + @Override + public void getAllPits(ActionListener getAllPitsListener) { + ListPitInfo listPitInfo = new ListPitInfo(getPitId(), 0, 0); + List list = new ArrayList<>(); + list.add(listPitInfo); + GetAllPitNodeResponse getAllPitNodeResponse = new GetAllPitNodeResponse( + cluster1Transport.getLocalDiscoNode(), + list + ); + List nodeList = new ArrayList(); + nodeList.add(getAllPitNodeResponse); + getAllPitsListener.onResponse(new GetAllPitNodesResponse(new ClusterName("cn"), nodeList, new ArrayList())); + } + }; + TransportDeletePitAction action = new TransportDeletePitAction( + transportService, + actionFilters, + namedWriteableRegistry, + pitService + ); + DeletePitRequest deletePITRequest = new DeletePitRequest("_all"); + PlainActionFuture future = newFuture(); + action.execute(task, deletePITRequest, future); + Exception e = assertThrows(ExecutionException.class, () -> future.get()); + assertThat(e.getMessage(), containsString("node down")); + assertEquals(3, deleteNodesInvoked.size()); + } + } + } + + public void testDeleteAllPitFailure() { + List deleteNodesInvoked = new CopyOnWriteArrayList<>(); + ActionFilters actionFilters = mock(ActionFilters.class); + when(actionFilters.filters()).thenReturn(new ActionFilter[0]); + + List knownNodes = new CopyOnWriteArrayList<>(); + try ( + MockTransportService cluster1Transport = startTransport("cluster_1_node", knownNodes, Version.CURRENT); + MockTransportService cluster2Transport = startTransport("cluster_2_node", knownNodes, Version.CURRENT) + ) { + knownNodes.add(cluster1Transport.getLocalDiscoNode()); + knownNodes.add(cluster2Transport.getLocalDiscoNode()); + Collections.shuffle(knownNodes, random()); + + try ( + MockTransportService transportService = MockTransportService.createNewService( + Settings.EMPTY, + Version.CURRENT, + threadPool, + null + ) + ) { + transportService.start(); + transportService.acceptIncomingRequests(); + SearchTransportService searchTransportService = new SearchTransportService(transportService, null) { + + public void sendFreePITContexts( + Transport.Connection connection, + List contextId, + final ActionListener listener + ) { + deleteNodesInvoked.add(connection.getNode()); + if (connection.getNode().getId() == "node_3") { + Thread t = new Thread(() -> listener.onFailure(new Exception("node 3 is down"))); + t.start(); + } else { + Thread t = new Thread(() -> listener.onResponse(new DeletePitResponse(new ArrayList<>()))); + t.start(); + } + } + + @Override + public Transport.Connection getConnection(String clusterAlias, DiscoveryNode node) { + return new SearchAsyncActionTests.MockConnection(node); + } + }; + PitService pitService = new PitService(clusterServiceMock, searchTransportService, transportService, client) { + @Override + public void getAllPits(ActionListener getAllPitsListener) { + ListPitInfo listPitInfo = new ListPitInfo(getPitId(), 0, 0); + List list = new ArrayList<>(); + list.add(listPitInfo); + GetAllPitNodeResponse getAllPitNodeResponse = new GetAllPitNodeResponse( + cluster1Transport.getLocalDiscoNode(), + list + ); + List nodeList = new ArrayList(); + nodeList.add(getAllPitNodeResponse); + getAllPitsListener.onResponse(new GetAllPitNodesResponse(new ClusterName("cn"), nodeList, new ArrayList())); + } + }; + TransportDeletePitAction action = new TransportDeletePitAction( + transportService, + actionFilters, + namedWriteableRegistry, + pitService + ); + DeletePitRequest deletePITRequest = new DeletePitRequest("_all"); + PlainActionFuture future = newFuture(); + action.execute(task, deletePITRequest, future); + Exception e = assertThrows(ExecutionException.class, () -> future.get()); + assertThat(e.getMessage(), containsString("java.lang.Exception: node 3 is down")); + assertEquals(3, deleteNodesInvoked.size()); + } + } + } } diff --git a/server/src/test/java/org/opensearch/search/CreatePitSingleNodeTests.java b/server/src/test/java/org/opensearch/search/CreatePitSingleNodeTests.java index 284e018c7f8ed..ae7f795f57ee7 100644 --- a/server/src/test/java/org/opensearch/search/CreatePitSingleNodeTests.java +++ b/server/src/test/java/org/opensearch/search/CreatePitSingleNodeTests.java @@ -90,6 +90,7 @@ public void testCreatePITSuccess() throws ExecutionException, InterruptedExcepti validatePitStats("index", 1, 0, 0); validatePitStats("index", 1, 0, 1); service.doClose(); // this kills the keep-alive reaper we have to reset the node after this test + assertSegments(true, client()); validatePitStats("index", 0, 1, 0); validatePitStats("index", 0, 1, 1); } @@ -113,6 +114,7 @@ public void testCreatePITWithMultipleIndicesSuccess() throws ExecutionException, validatePitStats("index", 1, 0, 0); validatePitStats("index1", 1, 0, 0); service.doClose(); + assertSegments(true, client()); validatePitStats("index", 0, 1, 0); validatePitStats("index1", 0, 1, 0); } @@ -139,6 +141,7 @@ public void testCreatePITWithShardReplicasSuccess() throws ExecutionException, I validatePitStats("index", 1, 0, 0); validatePitStats("index", 1, 0, 1); service.doClose(); + assertSegments(true, client()); validatePitStats("index", 0, 1, 0); validatePitStats("index", 0, 1, 1); } @@ -156,6 +159,7 @@ public void testCreatePITWithNonExistentIndex() { assertTrue(ex.getMessage().contains("no such index [index1]")); assertEquals(0, service.getActiveContexts()); + assertSegments(true, client()); service.doClose(); } @@ -176,6 +180,7 @@ public void testCreatePITOnCloseIndex() throws ExecutionException, InterruptedEx SearchService service = getInstanceFromNode(SearchService.class); assertEquals(0, service.getActiveContexts()); PitTestsUtil.assertGetAllPitsEmpty(client()); + assertSegments(true, client()); service.doClose(); } @@ -199,6 +204,7 @@ public void testPitSearchOnDeletedIndex() throws ExecutionException, Interrupted SearchService service = getInstanceFromNode(SearchService.class); PitTestsUtil.assertGetAllPitsEmpty(client()); assertEquals(0, service.getActiveContexts()); + assertSegments(true, client()); service.doClose(); } @@ -240,6 +246,7 @@ public void testPitSearchOnCloseIndex() throws ExecutionException, InterruptedEx assertTrue(ex.shardFailures()[0].reason().contains("SearchContextMissingException")); assertEquals(0, service.getActiveContexts()); PitTestsUtil.assertGetAllPitsEmpty(client()); + assertSegments(true, client()); // PIT reader contexts are lost after close, verifying it with open index api client().admin().indices().prepareOpen("index").get(); ex = expectThrows(SearchPhaseExecutionException.class, () -> { @@ -551,6 +558,7 @@ public void testPitAfterUpdateIndex() throws Exception { assertEquals(0, service.getActiveContexts()); validatePitStats("test", 0, 1, 0); PitTestsUtil.assertGetAllPitsEmpty(client()); + assertSegments(true, client()); } } diff --git a/server/src/test/java/org/opensearch/search/DeletePitMultiNodeTests.java b/server/src/test/java/org/opensearch/search/DeletePitMultiNodeTests.java index f546cf544362d..e69b2cc523638 100644 --- a/server/src/test/java/org/opensearch/search/DeletePitMultiNodeTests.java +++ b/server/src/test/java/org/opensearch/search/DeletePitMultiNodeTests.java @@ -38,7 +38,9 @@ import java.util.concurrent.atomic.AtomicBoolean; import java.util.concurrent.atomic.AtomicInteger; +import static org.hamcrest.Matchers.blankOrNullString; import static org.hamcrest.Matchers.containsString; +import static org.hamcrest.Matchers.not; import static org.opensearch.action.support.WriteRequest.RefreshPolicy.IMMEDIATE; /** @@ -148,6 +150,31 @@ public void testDeletePitWithValidAndInvalidIds() throws Exception { assertThat(e.getMessage(), containsString("invalid id")); } + public void testDeleteAllPits() throws Exception { + createPitOnIndex("index"); + createIndex("index1", Settings.builder().put("index.number_of_shards", 5).put("index.number_of_replicas", 1).build()); + client().prepareIndex("index1").setId("1").setSource("field", "value").setRefreshPolicy(IMMEDIATE).execute().get(); + ensureGreen(); + createPitOnIndex("index1"); + validatePitStats("index", 5, 0); + validatePitStats("index1", 5, 0); + DeletePitRequest deletePITRequest = new DeletePitRequest("_all"); + + /** + * When we invoke delete again, returns success after clearing the remaining readers. Asserting reader context + * not found exceptions don't result in failures ( as deletion in one node is successful ) + */ + ActionFuture execute = client().execute(DeletePitAction.INSTANCE, deletePITRequest); + DeletePitResponse deletePITResponse = execute.get(); + for (DeletePitInfo deletePitInfo : deletePITResponse.getDeletePitResults()) { + assertThat(deletePitInfo.getPitId(), not(blankOrNullString())); + assertTrue(deletePitInfo.isSuccessful()); + } + validatePitStats("index", 0, 5); + validatePitStats("index1", 0, 5); + client().admin().indices().prepareDelete("index1").get(); + } + public void testDeletePitWhileNodeDrop() throws Exception { CreatePitResponse pitResponse = createPitOnIndex("index"); createIndex("index1", Settings.builder().put("index.number_of_shards", 5).put("index.number_of_replicas", 1).build()); @@ -188,6 +215,38 @@ public Settings onNodeStopped(String nodeName) throws Exception { client().admin().indices().prepareDelete("index1").get(); } + public void testDeleteAllPitsWhileNodeDrop() throws Exception { + createIndex("index1", Settings.builder().put("index.number_of_shards", 5).put("index.number_of_replicas", 1).build()); + client().prepareIndex("index1").setId("1").setSource("field", "value").setRefreshPolicy(IMMEDIATE).execute().get(); + createPitOnIndex("index1"); + ensureGreen(); + DeletePitRequest deletePITRequest = new DeletePitRequest("_all"); + internalCluster().restartRandomDataNode(new InternalTestCluster.RestartCallback() { + @Override + public Settings onNodeStopped(String nodeName) throws Exception { + ActionFuture execute = client().execute(DeletePitAction.INSTANCE, deletePITRequest); + try { + DeletePitResponse deletePITResponse = execute.get(); + for (DeletePitInfo deletePitInfo : deletePITResponse.getDeletePitResults()) { + assertThat(deletePitInfo.getPitId(), not(blankOrNullString())); + } + } catch (Exception e) { + assertTrue(e.getMessage().contains("Node not connected")); + } + return super.onNodeStopped(nodeName); + } + }); + ensureGreen(); + /** + * When we invoke delete again, returns success as all readers are cleared. (Delete all on node which is Up and + * once the node restarts, all active contexts are cleared in the node ) + */ + ActionFuture execute = client().execute(DeletePitAction.INSTANCE, deletePITRequest); + DeletePitResponse deletePITResponse = execute.get(); + assertEquals(0, deletePITResponse.getDeletePitResults().size()); + client().admin().indices().prepareDelete("index1").get(); + } + public void testDeleteWhileSearch() throws Exception { CreatePitResponse pitResponse = createPitOnIndex("index"); ensureGreen(); diff --git a/server/src/test/java/org/opensearch/search/PitMultiNodeTests.java b/server/src/test/java/org/opensearch/search/PitMultiNodeTests.java index 921755b67246b..a23e4141a78e4 100644 --- a/server/src/test/java/org/opensearch/search/PitMultiNodeTests.java +++ b/server/src/test/java/org/opensearch/search/PitMultiNodeTests.java @@ -25,7 +25,7 @@ import org.opensearch.action.search.DeletePitResponse; import org.opensearch.action.search.GetAllPitNodesRequest; import org.opensearch.action.search.GetAllPitNodesResponse; -import org.opensearch.action.search.NodesGetAllPitsAction; +import org.opensearch.action.search.GetAllPitsAction; import org.opensearch.action.search.PitTestsUtil; import org.opensearch.action.search.SearchResponse; import org.opensearch.cluster.node.DiscoveryNode; @@ -368,7 +368,7 @@ public void testGetAllPits() throws Exception { DiscoveryNode[] disNodesArr = new DiscoveryNode[nodes.size()]; nodes.toArray(disNodesArr); GetAllPitNodesRequest getAllPITNodesRequest = new GetAllPitNodesRequest(disNodesArr); - ActionFuture execute1 = client().execute(NodesGetAllPitsAction.INSTANCE, getAllPITNodesRequest); + ActionFuture execute1 = client().execute(GetAllPitsAction.INSTANCE, getAllPITNodesRequest); GetAllPitNodesResponse getPitResponse = execute1.get(); assertEquals(3, getPitResponse.getPitInfos().size()); List resultPitIds = getPitResponse.getPitInfos().stream().map(p -> p.getPitId()).collect(Collectors.toList()); @@ -388,7 +388,7 @@ public void testGetAllPitsDuringNodeDrop() throws Exception { internalCluster().restartRandomDataNode(new InternalTestCluster.RestartCallback() { @Override public Settings onNodeStopped(String nodeName) throws Exception { - ActionFuture execute1 = client().execute(NodesGetAllPitsAction.INSTANCE, getAllPITNodesRequest); + ActionFuture execute1 = client().execute(GetAllPitsAction.INSTANCE, getAllPITNodesRequest); GetAllPitNodesResponse getPitResponse = execute1.get(); // we still get a pit id from the data node which is up assertEquals(1, getPitResponse.getPitInfos().size()); @@ -447,7 +447,7 @@ public void onResponse(GetAllPitNodesResponse getAllPitNodesResponse) { @Override public void onFailure(Exception e) {} }, countDownLatch); - client().execute(NodesGetAllPitsAction.INSTANCE, getAllPITNodesRequest, listener); + client().execute(GetAllPitsAction.INSTANCE, getAllPITNodesRequest, listener); } else { LatchedActionListener listener = new LatchedActionListener<>(new ActionListener() { @Override diff --git a/server/src/test/java/org/opensearch/search/pit/RestDeletePitActionTests.java b/server/src/test/java/org/opensearch/search/pit/RestDeletePitActionTests.java index ecd9c23762ff5..0bfa16aafe1e3 100644 --- a/server/src/test/java/org/opensearch/search/pit/RestDeletePitActionTests.java +++ b/server/src/test/java/org/opensearch/search/pit/RestDeletePitActionTests.java @@ -9,15 +9,11 @@ package org.opensearch.search.pit; import org.apache.lucene.util.SetOnce; -import org.opensearch.Version; import org.opensearch.action.ActionListener; import org.opensearch.action.search.DeletePitRequest; import org.opensearch.action.search.DeletePitResponse; import org.opensearch.client.node.NodeClient; -import org.opensearch.cluster.node.DiscoveryNode; -import org.opensearch.cluster.node.DiscoveryNodes; import org.opensearch.common.bytes.BytesArray; -import org.opensearch.common.transport.TransportAddress; import org.opensearch.common.xcontent.XContentType; import org.opensearch.rest.RestRequest; import org.opensearch.rest.action.search.RestDeletePitAction; @@ -27,7 +23,6 @@ import org.opensearch.test.rest.FakeRestRequest; import java.util.Collections; -import java.util.function.Supplier; import static org.hamcrest.Matchers.equalTo; import static org.hamcrest.Matchers.hasSize; @@ -37,18 +32,7 @@ */ public class RestDeletePitActionTests extends OpenSearchTestCase { public void testParseDeletePitRequestWithInvalidJsonThrowsException() throws Exception { - RestDeletePitAction action = new RestDeletePitAction(new Supplier() { - @Override - public DiscoveryNodes get() { - DiscoveryNode node0 = new DiscoveryNode( - "node0", - new TransportAddress(TransportAddress.META_ADDRESS, 9000), - Version.CURRENT - ); - DiscoveryNodes nodes = new DiscoveryNodes.Builder().add(node0).clusterManagerNodeId(node0.getId()).build(); - return nodes; - } - }); + RestDeletePitAction action = new RestDeletePitAction(); RestRequest request = new FakeRestRequest.Builder(xContentRegistry()).withContent( new BytesArray("{invalid_json}"), XContentType.JSON @@ -67,18 +51,7 @@ public void deletePits(DeletePitRequest request, ActionListener() { - @Override - public DiscoveryNodes get() { - DiscoveryNode node0 = new DiscoveryNode( - "node0", - new TransportAddress(TransportAddress.META_ADDRESS, 9000), - Version.CURRENT - ); - DiscoveryNodes nodes = new DiscoveryNodes.Builder().add(node0).clusterManagerNodeId(node0.getId()).build(); - return nodes; - } - }); + RestDeletePitAction action = new RestDeletePitAction(); RestRequest request = new FakeRestRequest.Builder(xContentRegistry()).withContent( new BytesArray("{\"pit_id\": [\"BODY\"]}"), XContentType.JSON @@ -90,6 +63,25 @@ public DiscoveryNodes get() { } } + public void testDeleteAllPit() throws Exception { + SetOnce pitCalled = new SetOnce<>(); + try (NodeClient nodeClient = new NoOpNodeClient(this.getTestName()) { + @Override + public void deletePits(DeletePitRequest request, ActionListener listener) { + pitCalled.set(true); + assertThat(request.getPitIds(), hasSize(1)); + assertThat(request.getPitIds().get(0), equalTo("_all")); + } + }) { + RestDeletePitAction action = new RestDeletePitAction(); + RestRequest request = new FakeRestRequest.Builder(xContentRegistry()).withPath("/_all").build(); + FakeRestChannel channel = new FakeRestChannel(request, false, 0); + action.handleRequest(request, channel, nodeClient); + + assertThat(pitCalled.get(), equalTo(true)); + } + } + public void testDeleteAllPitWithBody() { SetOnce pitCalled = new SetOnce<>(); try (NodeClient nodeClient = new NoOpNodeClient(this.getTestName()) { @@ -100,18 +92,7 @@ public void deletePits(DeletePitRequest request, ActionListener() { - @Override - public DiscoveryNodes get() { - DiscoveryNode node0 = new DiscoveryNode( - "node0", - new TransportAddress(TransportAddress.META_ADDRESS, 9000), - Version.CURRENT - ); - DiscoveryNodes nodes = new DiscoveryNodes.Builder().add(node0).clusterManagerNodeId(node0.getId()).build(); - return nodes; - } - }); + RestDeletePitAction action = new RestDeletePitAction(); RestRequest request = new FakeRestRequest.Builder(xContentRegistry()).withContent( new BytesArray("{\"pit_id\": [\"BODY\"]}"), XContentType.JSON @@ -137,18 +118,7 @@ public void deletePits(DeletePitRequest request, ActionListener() { - @Override - public DiscoveryNodes get() { - DiscoveryNode node0 = new DiscoveryNode( - "node0", - new TransportAddress(TransportAddress.META_ADDRESS, 9000), - Version.CURRENT - ); - DiscoveryNodes nodes = new DiscoveryNodes.Builder().add(node0).clusterManagerNodeId(node0.getId()).build(); - return nodes; - } - }); + RestDeletePitAction action = new RestDeletePitAction(); RestRequest request = new FakeRestRequest.Builder(xContentRegistry()).withParams( Collections.singletonMap("pit_id", "QUERY_STRING,QUERY_STRING_1") ).build(); From aa3b57a4e8ef66b7e59ef9addf410fa16a6ac4f0 Mon Sep 17 00:00:00 2001 From: Bharathwaj G Date: Wed, 21 Sep 2022 19:42:35 +0530 Subject: [PATCH 12/12] Addressing comments Signed-off-by: Bharathwaj G --- .../action/admin/indices/segments/PitSegmentsRequest.java | 4 ++-- .../org/opensearch/action/search/CreatePitController.java | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/server/src/main/java/org/opensearch/action/admin/indices/segments/PitSegmentsRequest.java b/server/src/main/java/org/opensearch/action/admin/indices/segments/PitSegmentsRequest.java index c26ce48a210b4..de0d390cddc4a 100644 --- a/server/src/main/java/org/opensearch/action/admin/indices/segments/PitSegmentsRequest.java +++ b/server/src/main/java/org/opensearch/action/admin/indices/segments/PitSegmentsRequest.java @@ -100,13 +100,13 @@ public void fromXContent(XContentParser parser) throws IOException { if (token == XContentParser.Token.START_ARRAY) { while ((token = parser.nextToken()) != XContentParser.Token.END_ARRAY) { if (token.isValue() == false) { - throw new IllegalArgumentException("pit_id array element should only contain pit_id"); + throw new IllegalArgumentException("pit_id array element should only contain PIT identifier"); } pitIds.add(parser.text()); } } else { if (token.isValue() == false) { - throw new IllegalArgumentException("pit_id element should only contain pit_id"); + throw new IllegalArgumentException("pit_id element should only contain PIT identifier"); } pitIds.add(parser.text()); } diff --git a/server/src/main/java/org/opensearch/action/search/CreatePitController.java b/server/src/main/java/org/opensearch/action/search/CreatePitController.java index 57c10c0632ebd..745139fd1f1e8 100644 --- a/server/src/main/java/org/opensearch/action/search/CreatePitController.java +++ b/server/src/main/java/org/opensearch/action/search/CreatePitController.java @@ -214,7 +214,7 @@ void executeUpdatePitId( "Create pit update phase for PIT ID [" + searchResponse.pointInTimeId() + "] failed because node[" - + node + + entry.getValue().getNode() + "] " + "not found" )