From 5fffc47a8dbaed05e1b5c814e132ebb8927bac8d Mon Sep 17 00:00:00 2001 From: Pranshu Shukla Date: Thu, 18 Jul 2024 14:48:44 +0530 Subject: [PATCH] fix naming and simplifying code Signed-off-by: Pranshu Shukla --- .../support/nodes/BaseNodesRequest.java | 10 +++--- .../support/nodes/TransportNodesAction.java | 34 ++++++------------- .../admin/cluster/RestClusterStatsAction.java | 2 +- .../admin/cluster/RestNodesInfoAction.java | 2 +- .../admin/cluster/RestNodesStatsAction.java | 2 +- .../rest/action/cat/RestNodesAction.java | 4 +-- .../action/RestStatsActionTests.java | 25 ++++++++++---- .../TransportClusterStatsActionTests.java | 14 ++++---- .../nodes/TransportNodesInfoActionTests.java | 8 ++--- .../nodes/TransportNodesStatsActionTests.java | 8 ++--- 10 files changed, 54 insertions(+), 55 deletions(-) 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 ab660a81f83db..37659bc7b3435 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 @@ -72,7 +72,7 @@ public abstract class BaseNodesRequest * * Setting default behavior as `true` but can be explicitly changed in requests that do not require. */ - private boolean populateDiscoveryNodesInTransportRequest = true; + private boolean retainDiscoveryNodes = true; private final TimeValue DEFAULT_TIMEOUT_SECS = TimeValue.timeValueSeconds(30); private TimeValue timeout; @@ -127,12 +127,12 @@ public void setConcreteNodes(DiscoveryNode[] concreteNodes) { this.concreteNodes = concreteNodes; } - public void populateDiscoveryNodesInTransportRequest(boolean value) { - populateDiscoveryNodesInTransportRequest = value; + public void retainDiscoveryNodes(boolean value) { + retainDiscoveryNodes = value; } - public boolean populateDiscoveryNodesInTransportRequest() { - return populateDiscoveryNodesInTransportRequest; + public boolean retainDiscoveryNodes() { + return retainDiscoveryNodes; } @Override 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 445e2f1c18223..bafc847169c25 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 @@ -246,32 +246,20 @@ class AsyncAction { this.task = task; this.request = request; this.listener = listener; - - // Check if concrete nodes are already available - if (request.concreteNodes() != null) { - this.responses = new AtomicReferenceArray<>(request.concreteNodes().length); - - if (request.populateDiscoveryNodesInTransportRequest()) { - this.concreteNodes = null; - } else { - this.concreteNodes = request.concreteNodes(); - request.setConcreteNodes(null); - assert request.concreteNodes() == null; - } - return; - } - - // Check if we want to populate the DiscoveryNodes in the transport Request and accordingly backfill the - // concrete nodes. - if (request.populateDiscoveryNodesInTransportRequest()) { + if (request.concreteNodes() == null) { resolveRequest(request, clusterService.state()); assert request.concreteNodes() != null; - this.responses = new AtomicReferenceArray<>(request.concreteNodes().length); - this.concreteNodes = null; + } + this.responses = new AtomicReferenceArray<>(request.concreteNodes().length); + + if (request.retainDiscoveryNodes() == false) { + // We transfer the ownership of discovery nodes to route the request to into the AsyncAction class. + // This reduces the payload of the request and improves the number of concrete nodes in the memory + this.concreteNodes = request.concreteNodes(); + request.setConcreteNodes(null); } else { - this.concreteNodes = getConcreteNodes(request, clusterService.state()); - assert request.concreteNodes() == null; - this.responses = new AtomicReferenceArray<>(concreteNodes.length); + // initializing it separately as we keep the `concreteNodes` as final since we want it to be immutable. + this.concreteNodes = null; } } 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 0ea651ba8b7d6..d136c733876e5 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,7 @@ 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.populateDiscoveryNodesInTransportRequest(false); + clusterStatsRequest.retainDiscoveryNodes(false); 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 e5e94f1cefc3b..f285add0e8189 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,7 @@ public RestChannelConsumer prepareRequest(final RestRequest request, final NodeC final NodesInfoRequest nodesInfoRequest = prepareRequest(request); nodesInfoRequest.timeout(request.param("timeout")); settingsFilter.addFilterSettingParams(request); - nodesInfoRequest.populateDiscoveryNodesInTransportRequest(false); + nodesInfoRequest.retainDiscoveryNodes(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 a277b08e7a34c..2cf10af94d694 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,7 @@ 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.populateDiscoveryNodesInTransportRequest(false); + nodesStatsRequest.retainDiscoveryNodes(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 f95e464b6d649..1cc8bb111c65c 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,7 @@ public RestChannelConsumer doCatRequest(final RestRequest request, final NodeCli public void processResponse(final ClusterStateResponse clusterStateResponse) { NodesInfoRequest nodesInfoRequest = new NodesInfoRequest(); nodesInfoRequest.timeout(request.param("timeout")); - nodesInfoRequest.populateDiscoveryNodesInTransportRequest(false); + nodesInfoRequest.retainDiscoveryNodes(false); nodesInfoRequest.clear() .addMetrics( NodesInfoRequest.Metric.JVM.metricName(), @@ -138,7 +138,7 @@ public void processResponse(final ClusterStateResponse clusterStateResponse) { public void processResponse(final NodesInfoResponse nodesInfoResponse) { NodesStatsRequest nodesStatsRequest = new NodesStatsRequest(); nodesStatsRequest.timeout(request.param("timeout")); - nodesStatsRequest.populateDiscoveryNodesInTransportRequest(false); + nodesStatsRequest.retainDiscoveryNodes(false); nodesStatsRequest.clear() .indices(true) .addMetrics( diff --git a/server/src/test/java/org/opensearch/action/RestStatsActionTests.java b/server/src/test/java/org/opensearch/action/RestStatsActionTests.java index df38d601f8c92..9b8a0640ee343 100644 --- a/server/src/test/java/org/opensearch/action/RestStatsActionTests.java +++ b/server/src/test/java/org/opensearch/action/RestStatsActionTests.java @@ -19,7 +19,6 @@ import org.opensearch.threadpool.TestThreadPool; import org.junit.After; -import java.io.IOException; import java.util.Collections; public class RestStatsActionTests extends OpenSearchTestCase { @@ -31,18 +30,30 @@ public void terminateThreadPool() { terminate(threadPool); } - public void testClusterStatsActionPrepareRequestNoError() throws IOException { + public void testClusterStatsActionPrepareRequestNoError() { RestClusterStatsAction action = new RestClusterStatsAction(); - action.prepareRequest(new FakeRestRequest(), client); + try { + action.prepareRequest(new FakeRestRequest(), client); + } catch (Throwable t) { + fail(t.getMessage()); + } } - public void testNodesStatsActionPrepareRequestNoError() throws IOException { + public void testNodesStatsActionPrepareRequestNoError() { RestNodesStatsAction action = new RestNodesStatsAction(); - action.prepareRequest(new FakeRestRequest(), client); + try { + action.prepareRequest(new FakeRestRequest(), client); + } catch (Throwable t) { + fail(t.getMessage()); + } } - public void testNodesInfoActionPrepareRequestNoError() throws IOException { + public void testNodesInfoActionPrepareRequestNoError() { RestNodesInfoAction action = new RestNodesInfoAction(new SettingsFilter(Collections.singleton("foo.filtered"))); - action.prepareRequest(new FakeRestRequest(), client); + try { + action.prepareRequest(new FakeRestRequest(), client); + } catch (Throwable t) { + fail(t.getMessage()); + } } } 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 82a2d5e6b990d..1391e04664566 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 @@ -37,9 +37,9 @@ 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 testClusterStatsActionWithDiscoveryNodesListPopulated() { + public void testClusterStatsActionWithRetentionOfDiscoveryNodesList() { ClusterStatsRequest request = new ClusterStatsRequest(); - request.populateDiscoveryNodesInTransportRequest(true); + request.retainDiscoveryNodes(true); Map> combinedSentRequest = performNodesInfoAction(request); assertNotNull(combinedSentRequest); @@ -52,7 +52,7 @@ public void testClusterStatsActionWithDiscoveryNodesListPopulated() { }); } - public void testClusterStatsActionWithDiscoveryNodesListInRestRequestPopulated() { + public void testClusterStatsActionWithPreFilledConcreteNodesAndWithRetentionOfDiscoveryNodesList() { ClusterStatsRequest request = new ClusterStatsRequest(); Collection discoveryNodes = clusterService.state().getNodes().getNodes().values(); request.setConcreteNodes(discoveryNodes.toArray(DiscoveryNode[]::new)); @@ -72,9 +72,9 @@ public void testClusterStatsActionWithDiscoveryNodesListInRestRequestPopulated() * In the optimized ClusterStats Request, we do not send the DiscoveryNodes List to each node. This behavior is * asserted in this test. */ - public void testClusterStatsActionWithDiscoveryNodesListNotPopulated() { + public void testClusterStatsActionWithoutRetentionOfDiscoveryNodesList() { ClusterStatsRequest request = new ClusterStatsRequest(); - request.populateDiscoveryNodesInTransportRequest(false); + request.retainDiscoveryNodes(false); Map> combinedSentRequest = performNodesInfoAction(request); assertNotNull(combinedSentRequest); @@ -84,11 +84,11 @@ public void testClusterStatsActionWithDiscoveryNodesListNotPopulated() { }); } - public void testClusterStatsWithDiscoveryNodesListInRestRequestNotPopulated() { + public void testClusterStatsActionWithPreFilledConcreteNodesAndWithoutRetentionOfDiscoveryNodesList() { ClusterStatsRequest request = new ClusterStatsRequest(); Collection discoveryNodes = clusterService.state().getNodes().getNodes().values(); request.setConcreteNodes(discoveryNodes.toArray(DiscoveryNode[]::new)); - request.populateDiscoveryNodesInTransportRequest(false); + request.retainDiscoveryNodes(false); Map> combinedSentRequest = performNodesInfoAction(request); assertNotNull(combinedSentRequest); 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 5b0e3541742c5..eaf4013f12b6a 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 @@ -35,9 +35,9 @@ 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 testNodesInfoActionWithDiscoveryNodesListPopulated() { + public void testNodesInfoActionWithRetentionOfDiscoveryNodesList() { NodesInfoRequest request = new NodesInfoRequest(); - request.populateDiscoveryNodesInTransportRequest(true); + request.retainDiscoveryNodes(true); Map> combinedSentRequest = performNodesInfoAction(request); assertNotNull(combinedSentRequest); @@ -54,9 +54,9 @@ public void testNodesInfoActionWithDiscoveryNodesListPopulated() { * In the optimized ClusterStats Request, we do not send the DiscoveryNodes List to each node. This behavior is * asserted in this test. */ - public void testNodesInfoActionWithDiscoveryNodesListNotPopulated() { + public void testNodesInfoActionWithoutRetentionOfDiscoveryNodesList() { NodesInfoRequest request = new NodesInfoRequest(); - request.populateDiscoveryNodesInTransportRequest(false); + request.retainDiscoveryNodes(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 b695641f979a1..3d192e2a35633 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 @@ -34,9 +34,9 @@ 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 testNodesStatsActionWithDiscoveryNodesListPopulated() { + public void testNodesStatsActionWithRetentionOfDiscoveryNodesList() { NodesStatsRequest request = new NodesStatsRequest(); - request.populateDiscoveryNodesInTransportRequest(true); + request.retainDiscoveryNodes(true); Map> combinedSentRequest = performNodesStatsAction(request); assertNotNull(combinedSentRequest); @@ -53,9 +53,9 @@ public void testNodesStatsActionWithDiscoveryNodesListPopulated() { * 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 testNodesStatsActionWithDiscoveryNodesListNotPopulated() { + public void testNodesStatsActionWithoutRetentionOfDiscoveryNodesList() { NodesStatsRequest request = new NodesStatsRequest(); - request.populateDiscoveryNodesInTransportRequest(false); + request.retainDiscoveryNodes(false); Map> combinedSentRequest = performNodesStatsAction(request); assertNotNull(combinedSentRequest);