diff --git a/CHANGELOG.md b/CHANGELOG.md index 5c7d7aa9bf780..86fb3050260f0 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -5,6 +5,7 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/), ## [Unreleased 2.x] ### Added +- Unset DiscoveryNodes in all transport node actions request ([#15131](https://github.com/opensearch-project/OpenSearch/pull/15131)) - Fix for hasInitiatedFetching to fix allocation explain and manual reroute APIs (([#14972](https://github.com/opensearch-project/OpenSearch/pull/14972)) - [Workload Management] Add queryGroupId to Task ([14708](https://github.com/opensearch-project/OpenSearch/pull/14708)) - Add setting to ignore throttling nodes for allocation of unassigned primaries in remote restore ([#14991](https://github.com/opensearch-project/OpenSearch/pull/14991)) diff --git a/server/src/main/java/org/opensearch/action/admin/cluster/node/reload/TransportNodesReloadSecureSettingsAction.java b/server/src/main/java/org/opensearch/action/admin/cluster/node/reload/TransportNodesReloadSecureSettingsAction.java index 92b8af0b8aa84..d2cb5872fc0e2 100644 --- a/server/src/main/java/org/opensearch/action/admin/cluster/node/reload/TransportNodesReloadSecureSettingsAction.java +++ b/server/src/main/java/org/opensearch/action/admin/cluster/node/reload/TransportNodesReloadSecureSettingsAction.java @@ -216,11 +216,7 @@ private boolean isNodeTransportTLSEnabled() { } private boolean isNodeLocal(NodesReloadSecureSettingsRequest request) { - if (null == request.concreteNodes()) { - resolveRequest(request, clusterService.state()); - assert request.concreteNodes() != null; - } - final DiscoveryNode[] nodes = request.concreteNodes(); + final DiscoveryNode[] nodes = resolveRequest(request, clusterService.state()); return nodes.length == 1 && nodes[0].getId().equals(clusterService.localNode().getId()); } } diff --git a/server/src/main/java/org/opensearch/action/support/nodes/BaseNodesRequest.java b/server/src/main/java/org/opensearch/action/support/nodes/BaseNodesRequest.java index a4f6d8afeaf38..3b86010cdf79e 100644 --- a/server/src/main/java/org/opensearch/action/support/nodes/BaseNodesRequest.java +++ b/server/src/main/java/org/opensearch/action/support/nodes/BaseNodesRequest.java @@ -66,13 +66,6 @@ public abstract class BaseNodesRequest * */ private DiscoveryNode[] concreteNodes; - /** - * Since do not use the discovery nodes coming from the request in all code paths following a request extended off from - * BaseNodeRequest, we do not require it to sent around across all nodes. - * - * Setting default behavior as `true` but can be explicitly changed in requests that do not require. - */ - private boolean includeDiscoveryNodes = true; private final TimeValue DEFAULT_TIMEOUT_SECS = TimeValue.timeValueSeconds(30); private TimeValue timeout; @@ -127,14 +120,6 @@ public void setConcreteNodes(DiscoveryNode[] concreteNodes) { this.concreteNodes = concreteNodes; } - public void setIncludeDiscoveryNodes(boolean value) { - includeDiscoveryNodes = value; - } - - public boolean getIncludeDiscoveryNodes() { - return includeDiscoveryNodes; - } - @Override public ActionRequestValidationException validate() { return null; diff --git a/server/src/main/java/org/opensearch/action/support/nodes/TransportNodesAction.java b/server/src/main/java/org/opensearch/action/support/nodes/TransportNodesAction.java index 3acd12f632e0f..f86ae9158992c 100644 --- a/server/src/main/java/org/opensearch/action/support/nodes/TransportNodesAction.java +++ b/server/src/main/java/org/opensearch/action/support/nodes/TransportNodesAction.java @@ -202,11 +202,16 @@ protected NodeResponse nodeOperation(NodeRequest request, Task task) { /** * resolve node ids to concrete nodes of the incoming request - **/ - protected void resolveRequest(NodesRequest request, ClusterState clusterState) { + * + * @return + */ + protected DiscoveryNode[] resolveRequest(NodesRequest request, ClusterState clusterState) { + if (request.concreteNodes() != null) { + return request.concreteNodes(); + } assert request.concreteNodes() == null : "request concreteNodes shouldn't be set"; String[] nodesIds = clusterState.nodes().resolveNodes(request.nodesIds()); - request.setConcreteNodes(Arrays.stream(nodesIds).map(clusterState.nodes()::get).toArray(DiscoveryNode[]::new)); + return Arrays.stream(nodesIds).map(clusterState.nodes()::get).toArray(DiscoveryNode[]::new); } /** @@ -234,23 +239,16 @@ class AsyncAction { this.task = task; this.request = request; this.listener = listener; - if (request.concreteNodes() == null) { - resolveRequest(request, clusterService.state()); - assert request.concreteNodes() != null; - } - this.responses = new AtomicReferenceArray<>(request.concreteNodes().length); - this.concreteNodes = request.concreteNodes(); - - if (request.getIncludeDiscoveryNodes() == false) { - // As we transfer the ownership of discovery nodes to route the request to into the AsyncAction class, we - // remove the list of DiscoveryNodes from the request. This reduces the payload of the request and improves - // the number of concrete nodes in the memory. - request.setConcreteNodes(null); - } + this.concreteNodes = resolveRequest(request, clusterService.state()); + this.responses = new AtomicReferenceArray<>(this.concreteNodes.length); } void start() { final DiscoveryNode[] nodes = this.concreteNodes; + // As we transfer the ownership of discovery nodes to route the request to into the AsyncAction class, + // we remove the list of DiscoveryNodes from the request. This reduces the payload of the request and improves + // the number of concrete nodes in the memory. + request.setConcreteNodes(null); if (nodes.length == 0) { // nothing to notify threadPool.generic().execute(() -> listener.onResponse(newResponse(request, responses))); diff --git a/server/src/main/java/org/opensearch/rest/action/admin/cluster/RestClusterStatsAction.java b/server/src/main/java/org/opensearch/rest/action/admin/cluster/RestClusterStatsAction.java index d4426a004af8e..ee33bd18db05d 100644 --- a/server/src/main/java/org/opensearch/rest/action/admin/cluster/RestClusterStatsAction.java +++ b/server/src/main/java/org/opensearch/rest/action/admin/cluster/RestClusterStatsAction.java @@ -66,7 +66,6 @@ public String getName() { public RestChannelConsumer prepareRequest(final RestRequest request, final NodeClient client) throws IOException { ClusterStatsRequest clusterStatsRequest = new ClusterStatsRequest().nodesIds(request.paramAsStringArray("nodeId", null)); clusterStatsRequest.timeout(request.param("timeout")); - clusterStatsRequest.setIncludeDiscoveryNodes(false); clusterStatsRequest.useAggregatedNodeLevelResponses(true); return channel -> client.admin().cluster().clusterStats(clusterStatsRequest, new NodesResponseRestListener<>(channel)); } diff --git a/server/src/main/java/org/opensearch/rest/action/admin/cluster/RestNodesInfoAction.java b/server/src/main/java/org/opensearch/rest/action/admin/cluster/RestNodesInfoAction.java index 4ac51933ea382..37e4c8783d0df 100644 --- a/server/src/main/java/org/opensearch/rest/action/admin/cluster/RestNodesInfoAction.java +++ b/server/src/main/java/org/opensearch/rest/action/admin/cluster/RestNodesInfoAction.java @@ -88,7 +88,6 @@ public RestChannelConsumer prepareRequest(final RestRequest request, final NodeC final NodesInfoRequest nodesInfoRequest = prepareRequest(request); nodesInfoRequest.timeout(request.param("timeout")); settingsFilter.addFilterSettingParams(request); - nodesInfoRequest.setIncludeDiscoveryNodes(false); return channel -> client.admin().cluster().nodesInfo(nodesInfoRequest, new NodesResponseRestListener<>(channel)); } diff --git a/server/src/main/java/org/opensearch/rest/action/admin/cluster/RestNodesStatsAction.java b/server/src/main/java/org/opensearch/rest/action/admin/cluster/RestNodesStatsAction.java index ed9c0b171aa56..267bfde576dec 100644 --- a/server/src/main/java/org/opensearch/rest/action/admin/cluster/RestNodesStatsAction.java +++ b/server/src/main/java/org/opensearch/rest/action/admin/cluster/RestNodesStatsAction.java @@ -232,7 +232,6 @@ public RestChannelConsumer prepareRequest(final RestRequest request, final NodeC // If no levels are passed in this results in an empty array. String[] levels = Strings.splitStringByCommaToArray(request.param("level")); nodesStatsRequest.indices().setLevels(levels); - nodesStatsRequest.setIncludeDiscoveryNodes(false); return channel -> client.admin().cluster().nodesStats(nodesStatsRequest, new NodesResponseRestListener<>(channel)); } diff --git a/server/src/main/java/org/opensearch/rest/action/cat/RestNodesAction.java b/server/src/main/java/org/opensearch/rest/action/cat/RestNodesAction.java index 0330fe627ccd0..bffb50cc63401 100644 --- a/server/src/main/java/org/opensearch/rest/action/cat/RestNodesAction.java +++ b/server/src/main/java/org/opensearch/rest/action/cat/RestNodesAction.java @@ -125,7 +125,6 @@ public RestChannelConsumer doCatRequest(final RestRequest request, final NodeCli public void processResponse(final ClusterStateResponse clusterStateResponse) { NodesInfoRequest nodesInfoRequest = new NodesInfoRequest(); nodesInfoRequest.timeout(request.param("timeout")); - nodesInfoRequest.setIncludeDiscoveryNodes(false); nodesInfoRequest.clear() .addMetrics( NodesInfoRequest.Metric.JVM.metricName(), @@ -138,7 +137,6 @@ public void processResponse(final ClusterStateResponse clusterStateResponse) { public void processResponse(final NodesInfoResponse nodesInfoResponse) { NodesStatsRequest nodesStatsRequest = new NodesStatsRequest(); nodesStatsRequest.timeout(request.param("timeout")); - nodesStatsRequest.setIncludeDiscoveryNodes(false); nodesStatsRequest.clear() .indices(true) .addMetrics( diff --git a/server/src/test/java/org/opensearch/action/support/nodes/TransportClusterStatsActionTests.java b/server/src/test/java/org/opensearch/action/support/nodes/TransportClusterStatsActionTests.java index f8e14b477b8ef..fc920b847cef9 100644 --- a/server/src/test/java/org/opensearch/action/support/nodes/TransportClusterStatsActionTests.java +++ b/server/src/test/java/org/opensearch/action/support/nodes/TransportClusterStatsActionTests.java @@ -33,48 +33,12 @@ public class TransportClusterStatsActionTests extends TransportNodesActionTests { - /** - * By default, we send discovery nodes list to each request that is sent across from the coordinator node. This - * behavior is asserted in this test. - */ - public void testClusterStatsActionWithRetentionOfDiscoveryNodesList() { - ClusterStatsRequest request = new ClusterStatsRequest(); - request.setIncludeDiscoveryNodes(true); - Map> combinedSentRequest = performNodesInfoAction(request); - - assertNotNull(combinedSentRequest); - combinedSentRequest.forEach((node, capturedRequestList) -> { - assertNotNull(capturedRequestList); - capturedRequestList.forEach(sentRequest -> { - assertNotNull(sentRequest.getDiscoveryNodes()); - assertEquals(sentRequest.getDiscoveryNodes().length, clusterService.state().nodes().getSize()); - }); - }); - } - - public void testClusterStatsActionWithPreFilledConcreteNodesAndWithRetentionOfDiscoveryNodesList() { - ClusterStatsRequest request = new ClusterStatsRequest(); - Collection discoveryNodes = clusterService.state().getNodes().getNodes().values(); - request.setConcreteNodes(discoveryNodes.toArray(DiscoveryNode[]::new)); - Map> combinedSentRequest = performNodesInfoAction(request); - - assertNotNull(combinedSentRequest); - combinedSentRequest.forEach((node, capturedRequestList) -> { - assertNotNull(capturedRequestList); - capturedRequestList.forEach(sentRequest -> { - assertNotNull(sentRequest.getDiscoveryNodes()); - assertEquals(sentRequest.getDiscoveryNodes().length, clusterService.state().nodes().getSize()); - }); - }); - } - /** * In the optimized ClusterStats Request, we do not send the DiscoveryNodes List to each node. This behavior is * asserted in this test. */ public void testClusterStatsActionWithoutRetentionOfDiscoveryNodesList() { ClusterStatsRequest request = new ClusterStatsRequest(); - request.setIncludeDiscoveryNodes(false); Map> combinedSentRequest = performNodesInfoAction(request); assertNotNull(combinedSentRequest); @@ -88,7 +52,6 @@ public void testClusterStatsActionWithPreFilledConcreteNodesAndWithoutRetentionO ClusterStatsRequest request = new ClusterStatsRequest(); Collection discoveryNodes = clusterService.state().getNodes().getNodes().values(); request.setConcreteNodes(discoveryNodes.toArray(DiscoveryNode[]::new)); - request.setIncludeDiscoveryNodes(false); Map> combinedSentRequest = performNodesInfoAction(request); assertNotNull(combinedSentRequest); diff --git a/server/src/test/java/org/opensearch/action/support/nodes/TransportNodesActionTests.java b/server/src/test/java/org/opensearch/action/support/nodes/TransportNodesActionTests.java index 7e968aa8fb199..946ffe1dedfcc 100644 --- a/server/src/test/java/org/opensearch/action/support/nodes/TransportNodesActionTests.java +++ b/server/src/test/java/org/opensearch/action/support/nodes/TransportNodesActionTests.java @@ -344,8 +344,8 @@ private static class DataNodesOnlyTransportNodesAction extends TestTransportNode } @Override - protected void resolveRequest(TestNodesRequest request, ClusterState clusterState) { - request.setConcreteNodes(clusterState.nodes().getDataNodes().values().toArray(new DiscoveryNode[0])); + protected DiscoveryNode[] resolveRequest(TestNodesRequest request, ClusterState clusterState) { + return clusterState.nodes().getDataNodes().values().toArray(new DiscoveryNode[0]); } } diff --git a/server/src/test/java/org/opensearch/action/support/nodes/TransportNodesInfoActionTests.java b/server/src/test/java/org/opensearch/action/support/nodes/TransportNodesInfoActionTests.java index e9e09d0dbbbf9..8277dcd363a8d 100644 --- a/server/src/test/java/org/opensearch/action/support/nodes/TransportNodesInfoActionTests.java +++ b/server/src/test/java/org/opensearch/action/support/nodes/TransportNodesInfoActionTests.java @@ -31,32 +31,12 @@ public class TransportNodesInfoActionTests extends TransportNodesActionTests { - /** - * By default, we send discovery nodes list to each request that is sent across from the coordinator node. This - * behavior is asserted in this test. - */ - public void testNodesInfoActionWithRetentionOfDiscoveryNodesList() { - NodesInfoRequest request = new NodesInfoRequest(); - request.setIncludeDiscoveryNodes(true); - Map> combinedSentRequest = performNodesInfoAction(request); - - assertNotNull(combinedSentRequest); - combinedSentRequest.forEach((node, capturedRequestList) -> { - assertNotNull(capturedRequestList); - capturedRequestList.forEach(sentRequest -> { - assertNotNull(sentRequest.getDiscoveryNodes()); - assertEquals(sentRequest.getDiscoveryNodes().length, clusterService.state().nodes().getSize()); - }); - }); - } - /** * In the optimized ClusterStats Request, we do not send the DiscoveryNodes List to each node. This behavior is * asserted in this test. */ public void testNodesInfoActionWithoutRetentionOfDiscoveryNodesList() { NodesInfoRequest request = new NodesInfoRequest(); - request.setIncludeDiscoveryNodes(false); Map> combinedSentRequest = performNodesInfoAction(request); assertNotNull(combinedSentRequest); diff --git a/server/src/test/java/org/opensearch/action/support/nodes/TransportNodesStatsActionTests.java b/server/src/test/java/org/opensearch/action/support/nodes/TransportNodesStatsActionTests.java index c7c420e353e1a..5e74dcdbc4953 100644 --- a/server/src/test/java/org/opensearch/action/support/nodes/TransportNodesStatsActionTests.java +++ b/server/src/test/java/org/opensearch/action/support/nodes/TransportNodesStatsActionTests.java @@ -31,31 +31,11 @@ public class TransportNodesStatsActionTests extends TransportNodesActionTests { /** - * By default, we send discovery nodes list to each request that is sent across from the coordinator node. This - * behavior is asserted in this test. - */ - public void testNodesStatsActionWithRetentionOfDiscoveryNodesList() { - NodesStatsRequest request = new NodesStatsRequest(); - request.setIncludeDiscoveryNodes(true); - Map> combinedSentRequest = performNodesStatsAction(request); - - assertNotNull(combinedSentRequest); - combinedSentRequest.forEach((node, capturedRequestList) -> { - assertNotNull(capturedRequestList); - capturedRequestList.forEach(sentRequest -> { - assertNotNull(sentRequest.getDiscoveryNodes()); - assertEquals(sentRequest.getDiscoveryNodes().length, clusterService.state().nodes().getSize()); - }); - }); - } - - /** - * By default, we send discovery nodes list to each request that is sent across from the coordinator node. This - * behavior is asserted in this test. + * We don't want to send discovery nodes list to each request that is sent across from the coordinator node. + * This behavior is asserted in this test. */ public void testNodesStatsActionWithoutRetentionOfDiscoveryNodesList() { NodesStatsRequest request = new NodesStatsRequest(); - request.setIncludeDiscoveryNodes(false); Map> combinedSentRequest = performNodesStatsAction(request); assertNotNull(combinedSentRequest);