Skip to content

Commit 64ca9ba

Browse files
author
Swetha Guptha
committed
Unset discovery nodes in all transport node actions request.
Signed-off-by: Swetha Guptha <gupthasg@amazon.com>
1 parent 49b7cd4 commit 64ca9ba

File tree

12 files changed

+20
-122
lines changed

12 files changed

+20
-122
lines changed

CHANGELOG.md

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -5,6 +5,7 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/),
55

66
## [Unreleased 2.x]
77
### Added
8+
- Unset DiscoveryNodes in all transport node actions request ([#15131](https://github.com/opensearch-project/OpenSearch/pull/15131))
89
- Fix for hasInitiatedFetching to fix allocation explain and manual reroute APIs (([#14972](https://github.com/opensearch-project/OpenSearch/pull/14972))
910
- [Workload Management] Add queryGroupId to Task ([14708](https://github.com/opensearch-project/OpenSearch/pull/14708))
1011
- Add setting to ignore throttling nodes for allocation of unassigned primaries in remote restore ([#14991](https://github.com/opensearch-project/OpenSearch/pull/14991))

server/src/main/java/org/opensearch/action/admin/cluster/node/reload/TransportNodesReloadSecureSettingsAction.java

Lines changed: 1 addition & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -216,11 +216,7 @@ private boolean isNodeTransportTLSEnabled() {
216216
}
217217

218218
private boolean isNodeLocal(NodesReloadSecureSettingsRequest request) {
219-
if (null == request.concreteNodes()) {
220-
resolveRequest(request, clusterService.state());
221-
assert request.concreteNodes() != null;
222-
}
223-
final DiscoveryNode[] nodes = request.concreteNodes();
219+
final DiscoveryNode[] nodes = resolveRequest(request, clusterService.state());
224220
return nodes.length == 1 && nodes[0].getId().equals(clusterService.localNode().getId());
225221
}
226222
}

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

Lines changed: 0 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -66,13 +66,6 @@ public abstract class BaseNodesRequest<Request extends BaseNodesRequest<Request>
6666
* */
6767
private DiscoveryNode[] concreteNodes;
6868

69-
/**
70-
* Since do not use the discovery nodes coming from the request in all code paths following a request extended off from
71-
* BaseNodeRequest, we do not require it to sent around across all nodes.
72-
*
73-
* Setting default behavior as `true` but can be explicitly changed in requests that do not require.
74-
*/
75-
private boolean includeDiscoveryNodes = true;
7669
private final TimeValue DEFAULT_TIMEOUT_SECS = TimeValue.timeValueSeconds(30);
7770

7871
private TimeValue timeout;
@@ -127,14 +120,6 @@ public void setConcreteNodes(DiscoveryNode[] concreteNodes) {
127120
this.concreteNodes = concreteNodes;
128121
}
129122

130-
public void setIncludeDiscoveryNodes(boolean value) {
131-
includeDiscoveryNodes = value;
132-
}
133-
134-
public boolean getIncludeDiscoveryNodes() {
135-
return includeDiscoveryNodes;
136-
}
137-
138123
@Override
139124
public ActionRequestValidationException validate() {
140125
return null;

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

Lines changed: 14 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -202,11 +202,16 @@ protected NodeResponse nodeOperation(NodeRequest request, Task task) {
202202

203203
/**
204204
* resolve node ids to concrete nodes of the incoming request
205-
**/
206-
protected void resolveRequest(NodesRequest request, ClusterState clusterState) {
205+
*
206+
* @return
207+
*/
208+
protected DiscoveryNode[] resolveRequest(NodesRequest request, ClusterState clusterState) {
209+
if (request.concreteNodes() != null) {
210+
return request.concreteNodes();
211+
}
207212
assert request.concreteNodes() == null : "request concreteNodes shouldn't be set";
208213
String[] nodesIds = clusterState.nodes().resolveNodes(request.nodesIds());
209-
request.setConcreteNodes(Arrays.stream(nodesIds).map(clusterState.nodes()::get).toArray(DiscoveryNode[]::new));
214+
return Arrays.stream(nodesIds).map(clusterState.nodes()::get).toArray(DiscoveryNode[]::new);
210215
}
211216

212217
/**
@@ -234,23 +239,16 @@ class AsyncAction {
234239
this.task = task;
235240
this.request = request;
236241
this.listener = listener;
237-
if (request.concreteNodes() == null) {
238-
resolveRequest(request, clusterService.state());
239-
assert request.concreteNodes() != null;
240-
}
241-
this.responses = new AtomicReferenceArray<>(request.concreteNodes().length);
242-
this.concreteNodes = request.concreteNodes();
243-
244-
if (request.getIncludeDiscoveryNodes() == false) {
245-
// As we transfer the ownership of discovery nodes to route the request to into the AsyncAction class, we
246-
// remove the list of DiscoveryNodes from the request. This reduces the payload of the request and improves
247-
// the number of concrete nodes in the memory.
248-
request.setConcreteNodes(null);
249-
}
242+
this.concreteNodes = resolveRequest(request, clusterService.state());
243+
this.responses = new AtomicReferenceArray<>(this.concreteNodes.length);
250244
}
251245

252246
void start() {
253247
final DiscoveryNode[] nodes = this.concreteNodes;
248+
// As we transfer the ownership of discovery nodes to route the request to into the AsyncAction class,
249+
// we remove the list of DiscoveryNodes from the request. This reduces the payload of the request and improves
250+
// the number of concrete nodes in the memory.
251+
request.setConcreteNodes(null);
254252
if (nodes.length == 0) {
255253
// nothing to notify
256254
threadPool.generic().execute(() -> listener.onResponse(newResponse(request, responses)));

server/src/main/java/org/opensearch/rest/action/admin/cluster/RestClusterStatsAction.java

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -66,7 +66,6 @@ public String getName() {
6666
public RestChannelConsumer prepareRequest(final RestRequest request, final NodeClient client) throws IOException {
6767
ClusterStatsRequest clusterStatsRequest = new ClusterStatsRequest().nodesIds(request.paramAsStringArray("nodeId", null));
6868
clusterStatsRequest.timeout(request.param("timeout"));
69-
clusterStatsRequest.setIncludeDiscoveryNodes(false);
7069
clusterStatsRequest.useAggregatedNodeLevelResponses(true);
7170
return channel -> client.admin().cluster().clusterStats(clusterStatsRequest, new NodesResponseRestListener<>(channel));
7271
}

server/src/main/java/org/opensearch/rest/action/admin/cluster/RestNodesInfoAction.java

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -88,7 +88,6 @@ public RestChannelConsumer prepareRequest(final RestRequest request, final NodeC
8888
final NodesInfoRequest nodesInfoRequest = prepareRequest(request);
8989
nodesInfoRequest.timeout(request.param("timeout"));
9090
settingsFilter.addFilterSettingParams(request);
91-
nodesInfoRequest.setIncludeDiscoveryNodes(false);
9291
return channel -> client.admin().cluster().nodesInfo(nodesInfoRequest, new NodesResponseRestListener<>(channel));
9392
}
9493

server/src/main/java/org/opensearch/rest/action/admin/cluster/RestNodesStatsAction.java

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -232,7 +232,6 @@ public RestChannelConsumer prepareRequest(final RestRequest request, final NodeC
232232
// If no levels are passed in this results in an empty array.
233233
String[] levels = Strings.splitStringByCommaToArray(request.param("level"));
234234
nodesStatsRequest.indices().setLevels(levels);
235-
nodesStatsRequest.setIncludeDiscoveryNodes(false);
236235

237236
return channel -> client.admin().cluster().nodesStats(nodesStatsRequest, new NodesResponseRestListener<>(channel));
238237
}

server/src/main/java/org/opensearch/rest/action/cat/RestNodesAction.java

Lines changed: 0 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -125,7 +125,6 @@ public RestChannelConsumer doCatRequest(final RestRequest request, final NodeCli
125125
public void processResponse(final ClusterStateResponse clusterStateResponse) {
126126
NodesInfoRequest nodesInfoRequest = new NodesInfoRequest();
127127
nodesInfoRequest.timeout(request.param("timeout"));
128-
nodesInfoRequest.setIncludeDiscoveryNodes(false);
129128
nodesInfoRequest.clear()
130129
.addMetrics(
131130
NodesInfoRequest.Metric.JVM.metricName(),
@@ -138,7 +137,6 @@ public void processResponse(final ClusterStateResponse clusterStateResponse) {
138137
public void processResponse(final NodesInfoResponse nodesInfoResponse) {
139138
NodesStatsRequest nodesStatsRequest = new NodesStatsRequest();
140139
nodesStatsRequest.timeout(request.param("timeout"));
141-
nodesStatsRequest.setIncludeDiscoveryNodes(false);
142140
nodesStatsRequest.clear()
143141
.indices(true)
144142
.addMetrics(

server/src/test/java/org/opensearch/action/support/nodes/TransportClusterStatsActionTests.java

Lines changed: 0 additions & 37 deletions
Original file line numberDiff line numberDiff line change
@@ -33,48 +33,12 @@
3333

3434
public class TransportClusterStatsActionTests extends TransportNodesActionTests {
3535

36-
/**
37-
* By default, we send discovery nodes list to each request that is sent across from the coordinator node. This
38-
* behavior is asserted in this test.
39-
*/
40-
public void testClusterStatsActionWithRetentionOfDiscoveryNodesList() {
41-
ClusterStatsRequest request = new ClusterStatsRequest();
42-
request.setIncludeDiscoveryNodes(true);
43-
Map<String, List<MockClusterStatsNodeRequest>> combinedSentRequest = performNodesInfoAction(request);
44-
45-
assertNotNull(combinedSentRequest);
46-
combinedSentRequest.forEach((node, capturedRequestList) -> {
47-
assertNotNull(capturedRequestList);
48-
capturedRequestList.forEach(sentRequest -> {
49-
assertNotNull(sentRequest.getDiscoveryNodes());
50-
assertEquals(sentRequest.getDiscoveryNodes().length, clusterService.state().nodes().getSize());
51-
});
52-
});
53-
}
54-
55-
public void testClusterStatsActionWithPreFilledConcreteNodesAndWithRetentionOfDiscoveryNodesList() {
56-
ClusterStatsRequest request = new ClusterStatsRequest();
57-
Collection<DiscoveryNode> discoveryNodes = clusterService.state().getNodes().getNodes().values();
58-
request.setConcreteNodes(discoveryNodes.toArray(DiscoveryNode[]::new));
59-
Map<String, List<MockClusterStatsNodeRequest>> combinedSentRequest = performNodesInfoAction(request);
60-
61-
assertNotNull(combinedSentRequest);
62-
combinedSentRequest.forEach((node, capturedRequestList) -> {
63-
assertNotNull(capturedRequestList);
64-
capturedRequestList.forEach(sentRequest -> {
65-
assertNotNull(sentRequest.getDiscoveryNodes());
66-
assertEquals(sentRequest.getDiscoveryNodes().length, clusterService.state().nodes().getSize());
67-
});
68-
});
69-
}
70-
7136
/**
7237
* In the optimized ClusterStats Request, we do not send the DiscoveryNodes List to each node. This behavior is
7338
* asserted in this test.
7439
*/
7540
public void testClusterStatsActionWithoutRetentionOfDiscoveryNodesList() {
7641
ClusterStatsRequest request = new ClusterStatsRequest();
77-
request.setIncludeDiscoveryNodes(false);
7842
Map<String, List<MockClusterStatsNodeRequest>> combinedSentRequest = performNodesInfoAction(request);
7943

8044
assertNotNull(combinedSentRequest);
@@ -88,7 +52,6 @@ public void testClusterStatsActionWithPreFilledConcreteNodesAndWithoutRetentionO
8852
ClusterStatsRequest request = new ClusterStatsRequest();
8953
Collection<DiscoveryNode> discoveryNodes = clusterService.state().getNodes().getNodes().values();
9054
request.setConcreteNodes(discoveryNodes.toArray(DiscoveryNode[]::new));
91-
request.setIncludeDiscoveryNodes(false);
9255
Map<String, List<MockClusterStatsNodeRequest>> combinedSentRequest = performNodesInfoAction(request);
9356

9457
assertNotNull(combinedSentRequest);

server/src/test/java/org/opensearch/action/support/nodes/TransportNodesActionTests.java

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -344,8 +344,8 @@ private static class DataNodesOnlyTransportNodesAction extends TestTransportNode
344344
}
345345

346346
@Override
347-
protected void resolveRequest(TestNodesRequest request, ClusterState clusterState) {
348-
request.setConcreteNodes(clusterState.nodes().getDataNodes().values().toArray(new DiscoveryNode[0]));
347+
protected DiscoveryNode[] resolveRequest(TestNodesRequest request, ClusterState clusterState) {
348+
return clusterState.nodes().getDataNodes().values().toArray(new DiscoveryNode[0]);
349349
}
350350
}
351351

server/src/test/java/org/opensearch/action/support/nodes/TransportNodesInfoActionTests.java

Lines changed: 0 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -31,32 +31,12 @@
3131

3232
public class TransportNodesInfoActionTests extends TransportNodesActionTests {
3333

34-
/**
35-
* By default, we send discovery nodes list to each request that is sent across from the coordinator node. This
36-
* behavior is asserted in this test.
37-
*/
38-
public void testNodesInfoActionWithRetentionOfDiscoveryNodesList() {
39-
NodesInfoRequest request = new NodesInfoRequest();
40-
request.setIncludeDiscoveryNodes(true);
41-
Map<String, List<MockNodesInfoRequest>> combinedSentRequest = performNodesInfoAction(request);
42-
43-
assertNotNull(combinedSentRequest);
44-
combinedSentRequest.forEach((node, capturedRequestList) -> {
45-
assertNotNull(capturedRequestList);
46-
capturedRequestList.forEach(sentRequest -> {
47-
assertNotNull(sentRequest.getDiscoveryNodes());
48-
assertEquals(sentRequest.getDiscoveryNodes().length, clusterService.state().nodes().getSize());
49-
});
50-
});
51-
}
52-
5334
/**
5435
* In the optimized ClusterStats Request, we do not send the DiscoveryNodes List to each node. This behavior is
5536
* asserted in this test.
5637
*/
5738
public void testNodesInfoActionWithoutRetentionOfDiscoveryNodesList() {
5839
NodesInfoRequest request = new NodesInfoRequest();
59-
request.setIncludeDiscoveryNodes(false);
6040
Map<String, List<MockNodesInfoRequest>> combinedSentRequest = performNodesInfoAction(request);
6141

6242
assertNotNull(combinedSentRequest);

server/src/test/java/org/opensearch/action/support/nodes/TransportNodesStatsActionTests.java

Lines changed: 2 additions & 22 deletions
Original file line numberDiff line numberDiff line change
@@ -31,31 +31,11 @@
3131
public class TransportNodesStatsActionTests extends TransportNodesActionTests {
3232

3333
/**
34-
* By default, we send discovery nodes list to each request that is sent across from the coordinator node. This
35-
* behavior is asserted in this test.
36-
*/
37-
public void testNodesStatsActionWithRetentionOfDiscoveryNodesList() {
38-
NodesStatsRequest request = new NodesStatsRequest();
39-
request.setIncludeDiscoveryNodes(true);
40-
Map<String, List<MockNodeStatsRequest>> combinedSentRequest = performNodesStatsAction(request);
41-
42-
assertNotNull(combinedSentRequest);
43-
combinedSentRequest.forEach((node, capturedRequestList) -> {
44-
assertNotNull(capturedRequestList);
45-
capturedRequestList.forEach(sentRequest -> {
46-
assertNotNull(sentRequest.getDiscoveryNodes());
47-
assertEquals(sentRequest.getDiscoveryNodes().length, clusterService.state().nodes().getSize());
48-
});
49-
});
50-
}
51-
52-
/**
53-
* By default, we send discovery nodes list to each request that is sent across from the coordinator node. This
54-
* behavior is asserted in this test.
34+
* We don't want to send discovery nodes list to each request that is sent across from the coordinator node.
35+
* This behavior is asserted in this test.
5536
*/
5637
public void testNodesStatsActionWithoutRetentionOfDiscoveryNodesList() {
5738
NodesStatsRequest request = new NodesStatsRequest();
58-
request.setIncludeDiscoveryNodes(false);
5939
Map<String, List<MockNodeStatsRequest>> combinedSentRequest = performNodesStatsAction(request);
6040

6141
assertNotNull(combinedSentRequest);

0 commit comments

Comments
 (0)