fix naming and simplifying code

Signed-off-by: Pranshu Shukla <[email protected]>

server/src/main/java/org/opensearch/action/support/nodes/BaseNodesRequest.java

@@ -72,7 +72,7 @@ public abstract class BaseNodesRequest<Request extends BaseNodesRequest<Request>
      *
      * 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

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;
             }
         }
 

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));
     }
 

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));
     }
 

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));
     }

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(

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());
+        }
     }
 }

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<String, List<MockClusterStatsNodeRequest>> combinedSentRequest = performNodesInfoAction(request);
 
         assertNotNull(combinedSentRequest);
@@ -52,7 +52,7 @@ public void testClusterStatsActionWithDiscoveryNodesListPopulated() {
         });
     }
 
-    public void testClusterStatsActionWithDiscoveryNodesListInRestRequestPopulated() {
+    public void testClusterStatsActionWithPreFilledConcreteNodesAndWithRetentionOfDiscoveryNodesList() {
         ClusterStatsRequest request = new ClusterStatsRequest();
         Collection<DiscoveryNode> 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<String, List<MockClusterStatsNodeRequest>> combinedSentRequest = performNodesInfoAction(request);
 
         assertNotNull(combinedSentRequest);
@@ -84,11 +84,11 @@ public void testClusterStatsActionWithDiscoveryNodesListNotPopulated() {
         });
     }
 
-    public void testClusterStatsWithDiscoveryNodesListInRestRequestNotPopulated() {
+    public void testClusterStatsActionWithPreFilledConcreteNodesAndWithoutRetentionOfDiscoveryNodesList() {
         ClusterStatsRequest request = new ClusterStatsRequest();
         Collection<DiscoveryNode> discoveryNodes = clusterService.state().getNodes().getNodes().values();
         request.setConcreteNodes(discoveryNodes.toArray(DiscoveryNode[]::new));
-        request.populateDiscoveryNodesInTransportRequest(false);
+        request.retainDiscoveryNodes(false);
         Map<String, List<MockClusterStatsNodeRequest>> combinedSentRequest = performNodesInfoAction(request);
 
         assertNotNull(combinedSentRequest);

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<String, List<MockNodesInfoRequest>> 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<String, List<MockNodesInfoRequest>> combinedSentRequest = performNodesInfoAction(request);
 
         assertNotNull(combinedSentRequest);

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<String, List<MockNodeStatsRequest>> 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<String, List<MockNodeStatsRequest>> combinedSentRequest = performNodesStatsAction(request);
 
         assertNotNull(combinedSentRequest);