Skip to content

Commit 2bb710b

Browse files
DaveCTurnerDaveCTurner
authored
Encapsulate JoinReason (#92750)
Today the reason attached to a `node-join` event is just a string, but we'd rather it be a proper object so we can attach some extra metadata in future. This refactors things to encapsulate the string within a dedicated record. Relates #92741
1 parent 6203560 commit 2bb710b

File tree

7 files changed

+63
-37
lines changed

7 files changed

+63
-37
lines changed

‎server/src/main/java/org/elasticsearch/cluster/coordination/JoinReason.java‎

Lines changed: 14 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,14 @@
1+
/*
2+
* Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one
3+
* or more contributor license agreements. Licensed under the Elastic License
4+
* 2.0 and the Server Side Public License, v 1; you may not use this file except
5+
* in compliance with, at your election, the Elastic License 2.0 or the Server
6+
* Side Public License, v 1.
7+
*/
8+
9+
package org.elasticsearch.cluster.coordination;
10+
11+
/**
12+
* @param message Message describing the reason for the node joining
13+
*/
14+
public record JoinReason(String message) {}

‎server/src/main/java/org/elasticsearch/cluster/coordination/JoinReasonService.java‎

Lines changed: 14 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -107,9 +107,9 @@ public void onNodeRemoved(DiscoveryNode discoveryNode, String reason) {
107107
* @param currentMode The current mode of the master that the node is joining.
108108
* @return A description of the reason for the join, possibly including some details of its earlier removal.
109109
*/
110-
public String getJoinReason(DiscoveryNode discoveryNode, Coordinator.Mode currentMode) {
110+
public JoinReason getJoinReason(DiscoveryNode discoveryNode, Coordinator.Mode currentMode) {
111111
return trackedNodes.getOrDefault(discoveryNode.getId(), UNKNOWN_NODE)
112-
.getDescription(relativeTimeInMillisSupplier.getAsLong(), discoveryNode.getEphemeralId(), currentMode);
112+
.getJoinReason(relativeTimeInMillisSupplier.getAsLong(), discoveryNode.getEphemeralId(), currentMode);
113113
}
114114

115115
/**
@@ -134,7 +134,7 @@ private interface TrackedNode {
134134

135135
TrackedNode withRemovalReason(String removalReason);
136136

137-
String getDescription(long currentTimeMillis, String joiningNodeEphemeralId, Coordinator.Mode currentMode);
137+
JoinReason getJoinReason(long currentTimeMillis, String joiningNodeEphemeralId, Coordinator.Mode currentMode);
138138

139139
long getRemovalAgeMillis(long currentTimeMillis);
140140
}
@@ -161,11 +161,11 @@ public TrackedNode withRemovalReason(String removalReason) {
161161
}
162162

163163
@Override
164-
public String getDescription(long currentTimeMillis, String joiningNodeEphemeralId, Coordinator.Mode currentMode) {
164+
public JoinReason getJoinReason(long currentTimeMillis, String joiningNodeEphemeralId, Coordinator.Mode currentMode) {
165165
if (currentMode == CANDIDATE) {
166-
return "completing election";
166+
return COMPLETING_ELECTION;
167167
} else {
168-
return "joining";
168+
return NEW_NODE_JOINING;
169169
}
170170
}
171171

@@ -193,11 +193,11 @@ public TrackedNode withRemovalReason(String removalReason) {
193193
}
194194

195195
@Override
196-
public String getDescription(long currentTimeMillis, String joiningNodeEphemeralId, Coordinator.Mode currentMode) {
196+
public JoinReason getJoinReason(long currentTimeMillis, String joiningNodeEphemeralId, Coordinator.Mode currentMode) {
197197
if (currentMode == CANDIDATE) {
198-
return "completing election";
198+
return COMPLETING_ELECTION;
199199
} else {
200-
return "rejoining";
200+
return KNOWN_NODE_REJOINING;
201201
}
202202
}
203203

@@ -231,7 +231,7 @@ public TrackedNode withRemovalReason(String removalReason) {
231231
}
232232

233233
@Override
234-
public String getDescription(long currentTimeMillis, String joiningNodeEphemeralId, Coordinator.Mode currentMode) {
234+
public JoinReason getJoinReason(long currentTimeMillis, String joiningNodeEphemeralId, Coordinator.Mode currentMode) {
235235
final StringBuilder description = new StringBuilder();
236236
if (currentMode == CANDIDATE) {
237237
description.append("completing election");
@@ -261,7 +261,7 @@ public String getDescription(long currentTimeMillis, String joiningNodeEphemeral
261261
description.append(", [").append(removalCount).append("] total removals");
262262
}
263263

264-
return description.toString();
264+
return new JoinReason(description.toString());
265265
}
266266

267267
@Override
@@ -270,4 +270,7 @@ public long getRemovalAgeMillis(long currentTimeMillis) {
270270
}
271271
}
272272

273+
private static final JoinReason COMPLETING_ELECTION = new JoinReason("completing election");
274+
private static final JoinReason NEW_NODE_JOINING = new JoinReason("joining");
275+
private static final JoinReason KNOWN_NODE_REJOINING = new JoinReason("rejoining");
273276
}

‎server/src/main/java/org/elasticsearch/cluster/coordination/JoinTask.java‎

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -18,7 +18,7 @@
1818

1919
public record JoinTask(List<NodeJoinTask> nodeJoinTasks, boolean isBecomingMaster, long term) implements ClusterStateTaskListener {
2020

21-
public static JoinTask singleNode(DiscoveryNode node, String reason, ActionListener<Void> listener, long term) {
21+
public static JoinTask singleNode(DiscoveryNode node, JoinReason reason, ActionListener<Void> listener, long term) {
2222
return new JoinTask(List.of(new NodeJoinTask(node, reason, listener)), false, term);
2323
}
2424

@@ -59,9 +59,9 @@ public Iterable<DiscoveryNode> nodes() {
5959
return () -> nodeJoinTasks.stream().map(j -> j.node).iterator();
6060
}
6161

62-
public record NodeJoinTask(DiscoveryNode node, String reason, ActionListener<Void> listener) {
62+
public record NodeJoinTask(DiscoveryNode node, JoinReason reason, ActionListener<Void> listener) {
6363

64-
public NodeJoinTask(DiscoveryNode node, String reason, ActionListener<Void> listener) {
64+
public NodeJoinTask(DiscoveryNode node, JoinReason reason, ActionListener<Void> listener) {
6565
this.node = Objects.requireNonNull(node);
6666
this.reason = reason;
6767
this.listener = listener;
@@ -76,7 +76,7 @@ public String toString() {
7676

7777
public void appendDescription(StringBuilder stringBuilder) {
7878
node.appendDescriptionWithoutAttributes(stringBuilder);
79-
stringBuilder.append(' ').append(reason);
79+
stringBuilder.append(' ').append(reason.message());
8080
}
8181
}
8282
}

‎server/src/main/java/org/elasticsearch/cluster/coordination/NodeJoinExecutor.java‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -139,7 +139,7 @@ public ClusterState execute(BatchExecutionContext<JoinTask> batchExecutionContex
139139
logger.info(
140140
"node-join: [{}] with reason [{}]",
141141
nodeJoinTask.node().descriptionWithoutAttributes(),
142-
nodeJoinTask.reason()
142+
nodeJoinTask.reason().message()
143143
);
144144
nodeJoinTask.listener().onResponse(null);
145145
});

‎server/src/test/java/org/elasticsearch/cluster/coordination/JoinReasonServiceTests.java‎

Lines changed: 23 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -15,6 +15,7 @@
1515
import org.elasticsearch.common.UUIDs;
1616
import org.elasticsearch.common.transport.TransportAddress;
1717
import org.elasticsearch.test.ESTestCase;
18+
import org.hamcrest.Matcher;
1819

1920
import java.util.Collections;
2021
import java.util.concurrent.atomic.AtomicLong;
@@ -40,48 +41,48 @@ public void testJoinReasonService() {
4041

4142
final DiscoveryNodes withNode = DiscoveryNodes.builder(withoutNode).add(discoveryNode).build();
4243

43-
assertThat(joinReasonService.getJoinReason(discoveryNode, CANDIDATE), equalTo("completing election"));
44-
assertThat(joinReasonService.getJoinReason(discoveryNode, LEADER), equalTo("joining"));
44+
assertThat(joinReasonService.getJoinReason(discoveryNode, CANDIDATE), matches("completing election"));
45+
assertThat(joinReasonService.getJoinReason(discoveryNode, LEADER), matches("joining"));
4546

4647
joinReasonService.onClusterStateApplied(withoutNode);
4748

48-
assertThat(joinReasonService.getJoinReason(discoveryNode, CANDIDATE), equalTo("completing election"));
49-
assertThat(joinReasonService.getJoinReason(discoveryNode, LEADER), equalTo("joining"));
49+
assertThat(joinReasonService.getJoinReason(discoveryNode, CANDIDATE), matches("completing election"));
50+
assertThat(joinReasonService.getJoinReason(discoveryNode, LEADER), matches("joining"));
5051

5152
joinReasonService.onClusterStateApplied(withNode);
5253

53-
assertThat(joinReasonService.getJoinReason(discoveryNode, CANDIDATE), equalTo("completing election"));
54-
assertThat(joinReasonService.getJoinReason(discoveryNode, LEADER), equalTo("rejoining"));
54+
assertThat(joinReasonService.getJoinReason(discoveryNode, CANDIDATE), matches("completing election"));
55+
assertThat(joinReasonService.getJoinReason(discoveryNode, LEADER), matches("rejoining"));
5556

5657
joinReasonService.onClusterStateApplied(withoutNode);
5758
currentTimeMillis.addAndGet(1234L);
5859

5960
assertThat(
6061
joinReasonService.getJoinReason(discoveryNode, LEADER),
61-
equalTo("joining, removed [1.2s/1234ms] ago by [" + master.getName() + "]")
62+
matches("joining, removed [1.2s/1234ms] ago by [" + master.getName() + "]")
6263
);
6364

6465
joinReasonService.onNodeRemoved(discoveryNode, "test removal");
6566
currentTimeMillis.addAndGet(4321L);
6667

6768
assertThat(
6869
joinReasonService.getJoinReason(discoveryNode, LEADER),
69-
equalTo("joining, removed [5.5s/5555ms] ago with reason [test removal]")
70+
matches("joining, removed [5.5s/5555ms] ago with reason [test removal]")
7071
);
7172

7273
joinReasonService.onClusterStateApplied(withNode);
7374
joinReasonService.onClusterStateApplied(withoutNode);
7475

7576
assertThat(
7677
joinReasonService.getJoinReason(discoveryNode, LEADER),
77-
equalTo("joining, removed [0ms] ago by [" + master.getName() + "], [2] total removals")
78+
matches("joining, removed [0ms] ago by [" + master.getName() + "], [2] total removals")
7879
);
7980

8081
joinReasonService.onNodeRemoved(discoveryNode, "second test removal");
8182

8283
assertThat(
8384
joinReasonService.getJoinReason(discoveryNode, LEADER),
84-
equalTo("joining, removed [0ms] ago with reason [second test removal], [2] total removals")
85+
matches("joining, removed [0ms] ago with reason [second test removal], [2] total removals")
8586
);
8687

8788
final DiscoveryNode rebootedNode = new DiscoveryNode(
@@ -98,7 +99,7 @@ public void testJoinReasonService() {
9899

99100
assertThat(
100101
joinReasonService.getJoinReason(rebootedNode, LEADER),
101-
equalTo("joining after restart, removed [0ms] ago with reason [second test removal]")
102+
matches("joining after restart, removed [0ms] ago with reason [second test removal]")
102103
);
103104

104105
final DiscoveryNodes withRebootedNode = DiscoveryNodes.builder(withoutNode).add(rebootedNode).build();
@@ -108,7 +109,7 @@ public void testJoinReasonService() {
108109

109110
assertThat(
110111
joinReasonService.getJoinReason(rebootedNode, LEADER),
111-
equalTo("joining, removed [0ms] ago with reason [third test removal]")
112+
matches("joining, removed [0ms] ago with reason [third test removal]")
112113
);
113114

114115
joinReasonService.onClusterStateApplied(withRebootedNode);
@@ -117,7 +118,7 @@ public void testJoinReasonService() {
117118

118119
assertThat(
119120
joinReasonService.getJoinReason(rebootedNode, LEADER),
120-
equalTo("joining, removed [0ms] ago with reason [fourth test removal], [2] total removals")
121+
matches("joining, removed [0ms] ago with reason [fourth test removal], [2] total removals")
121122
);
122123

123124
joinReasonService.onClusterStateApplied(withRebootedNode);
@@ -126,12 +127,12 @@ public void testJoinReasonService() {
126127

127128
assertThat(
128129
joinReasonService.getJoinReason(discoveryNode, LEADER),
129-
equalTo("joining, removed [0ms] ago by [" + master.getName() + "]")
130+
matches("joining, removed [0ms] ago by [" + master.getName() + "]")
130131
);
131132

132133
assertThat(
133134
joinReasonService.getJoinReason(rebootedNode, LEADER),
134-
equalTo("joining after restart, removed [0ms] ago by [" + master.getName() + "]")
135+
matches("joining after restart, removed [0ms] ago by [" + master.getName() + "]")
135136
);
136137
}
137138

@@ -178,11 +179,11 @@ public void testCleanup() {
178179

179180
// remove almost enough other nodes and verify that we're still tracking the target node
180181
joinReasonService.onClusterStateApplied(almostCleanupNodes);
181-
assertThat(joinReasonService.getJoinReason(targetNode, LEADER), equalTo("joining, removed [1ms] ago with reason [test]"));
182+
assertThat(joinReasonService.getJoinReason(targetNode, LEADER), matches("joining, removed [1ms] ago with reason [test]"));
182183

183184
// remove one more node to trigger the cleanup and forget about the target node
184185
joinReasonService.onClusterStateApplied(cleanupNodes);
185-
assertThat(joinReasonService.getJoinReason(targetNode, LEADER), equalTo("joining"));
186+
assertThat(joinReasonService.getJoinReason(targetNode, LEADER), matches("joining"));
186187
}
187188

188189
private DiscoveryNode randomDiscoveryNode() {
@@ -199,4 +200,9 @@ private DiscoveryNode randomDiscoveryNode() {
199200
Version.CURRENT
200201
);
201202
}
203+
204+
private static Matcher<JoinReason> matches(String message) {
205+
return equalTo(new JoinReason(message));
206+
}
207+
202208
}

‎server/src/test/java/org/elasticsearch/cluster/coordination/NodeJoinExecutorTests.java‎

Lines changed: 5 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -174,7 +174,7 @@ private static Version getRandomCompatibleVersion() {
174174
return VersionUtils.randomVersionBetween(random(), Version.CURRENT.minimumIndexCompatibilityVersion(), Version.CURRENT);
175175
}
176176

177-
private static final String TEST_REASON = "test";
177+
private static final JoinReason TEST_REASON = new JoinReason("test");
178178

179179
public void testUpdatesNodeWithNewRoles() throws Exception {
180180
// Node roles vary by version, and new roles are suppressed for BWC. This means we can receive a join from a node that's already
@@ -607,7 +607,7 @@ public void testPerNodeLogging() {
607607
"info message",
608608
LOGGER_NAME,
609609
Level.INFO,
610-
"node-join: [" + node1.descriptionWithoutAttributes() + "] with reason [" + TEST_REASON + "]"
610+
"node-join: [" + node1.descriptionWithoutAttributes() + "] with reason [" + TEST_REASON.message() + "]"
611611
)
612612
);
613613
assertNull(
@@ -618,7 +618,9 @@ public void testPerNodeLogging() {
618618
JoinTask.singleNode(node1, TEST_REASON, future, 0L),
619619
ClusterStateTaskConfig.build(Priority.NORMAL),
620620
executor
621-
)
621+
),
622+
10,
623+
TimeUnit.SECONDS
622624
)
623625
);
624626
appender.assertAllExpectationsMatched();

‎server/src/test/java/org/elasticsearch/indices/cluster/ClusterStateChanges.java‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -45,6 +45,7 @@
4545
import org.elasticsearch.cluster.action.shard.ShardStateAction.StartedShardEntry;
4646
import org.elasticsearch.cluster.action.shard.ShardStateAction.StartedShardUpdateTask;
4747
import org.elasticsearch.cluster.block.ClusterBlock;
48+
import org.elasticsearch.cluster.coordination.JoinReason;
4849
import org.elasticsearch.cluster.coordination.JoinTask;
4950
import org.elasticsearch.cluster.coordination.NodeJoinExecutor;
5051
import org.elasticsearch.cluster.coordination.NodeLeftExecutor;
@@ -396,7 +397,7 @@ public ClusterState reroute(ClusterState state, ClusterRerouteRequest request) {
396397
return execute(transportClusterRerouteAction, request, state);
397398
}
398399

399-
private static final String DUMMY_REASON = "dummy reason";
400+
private static final JoinReason DUMMY_REASON = new JoinReason("dummy reason");
400401

401402
public ClusterState addNode(ClusterState clusterState, DiscoveryNode discoveryNode) {
402403
return runTasks(

0 commit comments

Comments
 (0)