From fddae4822093c8df6485ef770b4bb53377fa8181 Mon Sep 17 00:00:00 2001 From: Tanguy Leroux Date: Fri, 12 Jan 2018 15:09:09 +0100 Subject: [PATCH 1/4] Add information when master node left to DiscoveryNodes' shortSummary() This commit changes `DiscoveryNodes.Delta.shortSummary()` in order to add information to the summary when the master node left. --- .../cluster/node/DiscoveryNodes.java | 27 ++++++++++++++----- .../cluster/node/DiscoveryNodesTests.java | 7 +++-- 2 files changed, 23 insertions(+), 11 deletions(-) diff --git a/server/src/main/java/org/elasticsearch/cluster/node/DiscoveryNodes.java b/server/src/main/java/org/elasticsearch/cluster/node/DiscoveryNodes.java index 4373069a5f77c..9e4b2a071ef77 100644 --- a/server/src/main/java/org/elasticsearch/cluster/node/DiscoveryNodes.java +++ b/server/src/main/java/org/elasticsearch/cluster/node/DiscoveryNodes.java @@ -400,10 +400,14 @@ public Delta delta(DiscoveryNodes other) { DiscoveryNode previousMasterNode = null; DiscoveryNode newMasterNode = null; if (masterNodeId != null) { - if (other.masterNodeId == null || !other.masterNodeId.equals(masterNodeId)) { + if (other.masterNodeId == null) { + newMasterNode = getMasterNode(); + } else if (other.masterNodeId.equals(masterNodeId) == false) { previousMasterNode = other.getMasterNode(); newMasterNode = getMasterNode(); } + } else if (other.masterNodeId != null) { + previousMasterNode = other.getMasterNode(); } return new Delta(previousMasterNode, newMasterNode, localNodeId, Collections.unmodifiableList(removed), Collections.unmodifiableList(added)); @@ -448,7 +452,7 @@ public boolean hasChanges() { } public boolean masterNodeChanged() { - return newMasterNode != null; + return newMasterNode != previousMasterNode; } public DiscoveryNode previousMasterNode() { @@ -478,16 +482,25 @@ public List addedNodes() { public String shortSummary() { StringBuilder sb = new StringBuilder(); if (!removed() && masterNodeChanged()) { - if (newMasterNode.getId().equals(localNodeId)) { - // we are the master, no nodes we removed, we are actually the first master - sb.append("new_master ").append(newMasterNode()); + if (newMasterNode() != null) { + if (newMasterNode().getId().equals(localNodeId)) { + // we are the master, no nodes we removed, we are actually the first master + sb.append("new_master ").append(newMasterNode()); + } else { + // we are not the master, so we just got this event. No nodes were removed, so its not a *new* master + sb.append("detected_master ").append(newMasterNode()); + } } else { - // we are not the master, so we just got this event. No nodes were removed, so its not a *new* master - sb.append("detected_master ").append(newMasterNode()); + sb.append("master_left ").append(previousMasterNode()); } } else { if (masterNodeChanged()) { sb.append("master {new ").append(newMasterNode()); + if (newMasterNode() != null) { + sb.append("new ").append(newMasterNode()); + } else { + sb.append("left"); + } if (previousMasterNode() != null) { sb.append(", previous ").append(previousMasterNode()); } diff --git a/server/src/test/java/org/elasticsearch/cluster/node/DiscoveryNodesTests.java b/server/src/test/java/org/elasticsearch/cluster/node/DiscoveryNodesTests.java index 9200e04c7127a..6bfb78a2ade9c 100644 --- a/server/src/test/java/org/elasticsearch/cluster/node/DiscoveryNodesTests.java +++ b/server/src/test/java/org/elasticsearch/cluster/node/DiscoveryNodesTests.java @@ -140,15 +140,14 @@ public void testDeltas() { DiscoveryNodes.Delta delta = discoNodesB.delta(discoNodesA); - if (masterB == null || Objects.equals(masterAId, masterBId)) { + if (Objects.equals(masterAId, masterBId)) { assertFalse(delta.masterNodeChanged()); assertThat(delta.previousMasterNode(), nullValue()); assertThat(delta.newMasterNode(), nullValue()); } else { assertTrue(delta.masterNodeChanged()); - assertThat(delta.newMasterNode().getId(), equalTo(masterBId)); - assertThat(delta.previousMasterNode() != null ? delta.previousMasterNode().getId() : null, - equalTo(masterAId)); + assertThat(delta.newMasterNode() != null ? delta.newMasterNode().getId() : null, equalTo(masterBId)); + assertThat(delta.previousMasterNode() != null ? delta.previousMasterNode().getId() : null, equalTo(masterAId)); } Set newNodes = new HashSet<>(nodesB); From bf9e1b1f32c4742857814ffc3a05b98f52993e7e Mon Sep 17 00:00:00 2001 From: Tanguy Leroux Date: Mon, 15 Jan 2018 12:40:05 +0100 Subject: [PATCH 2/4] Apply feedback --- .../cluster/node/DiscoveryNodes.java | 88 ++++++++----------- 1 file changed, 35 insertions(+), 53 deletions(-) diff --git a/server/src/main/java/org/elasticsearch/cluster/node/DiscoveryNodes.java b/server/src/main/java/org/elasticsearch/cluster/node/DiscoveryNodes.java index 9e4b2a071ef77..ea5bd2e51d134 100644 --- a/server/src/main/java/org/elasticsearch/cluster/node/DiscoveryNodes.java +++ b/server/src/main/java/org/elasticsearch/cluster/node/DiscoveryNodes.java @@ -39,6 +39,7 @@ import java.util.Iterator; import java.util.List; import java.util.Map; +import java.util.Objects; /** * This class holds all {@link DiscoveryNode} in the cluster and provides convenience methods to @@ -385,30 +386,28 @@ public DiscoveryNodes newNode(DiscoveryNode node) { * Returns the changes comparing this nodes to the provided nodes. */ public Delta delta(DiscoveryNodes other) { - List removed = new ArrayList<>(); - List added = new ArrayList<>(); + final List removed = new ArrayList<>(); + final List added = new ArrayList<>(); for (DiscoveryNode node : other) { - if (!this.nodeExists(node)) { + if (this.nodeExists(node) == false) { removed.add(node); } } for (DiscoveryNode node : this) { - if (!other.nodeExists(node)) { + if (other.nodeExists(node) == false) { added.add(node); } } + DiscoveryNode previousMasterNode = null; + if (other.masterNodeId != null) { + previousMasterNode = other.getMasterNode(); + } DiscoveryNode newMasterNode = null; if (masterNodeId != null) { - if (other.masterNodeId == null) { - newMasterNode = getMasterNode(); - } else if (other.masterNodeId.equals(masterNodeId) == false) { - previousMasterNode = other.getMasterNode(); - newMasterNode = getMasterNode(); - } - } else if (other.masterNodeId != null) { - previousMasterNode = other.getMasterNode(); + newMasterNode = getMasterNode(); } + return new Delta(previousMasterNode, newMasterNode, localNodeId, Collections.unmodifiableList(removed), Collections.unmodifiableList(added)); } @@ -452,7 +451,9 @@ public boolean hasChanges() { } public boolean masterNodeChanged() { - return newMasterNode != previousMasterNode; + String newMasterId = (newMasterNode != null) ? newMasterNode.getEphemeralId() : null; + String previousMasterId = (previousMasterNode != null) ? previousMasterNode.getEphemeralId() : null; + return Objects.equals(newMasterId, previousMasterId) == false; } public DiscoveryNode previousMasterNode() { @@ -480,60 +481,41 @@ public List addedNodes() { } public String shortSummary() { - StringBuilder sb = new StringBuilder(); - if (!removed() && masterNodeChanged()) { - if (newMasterNode() != null) { - if (newMasterNode().getId().equals(localNodeId)) { - // we are the master, no nodes we removed, we are actually the first master - sb.append("new_master ").append(newMasterNode()); - } else { - // we are not the master, so we just got this event. No nodes were removed, so its not a *new* master - sb.append("detected_master ").append(newMasterNode()); - } - } else { - sb.append("master_left ").append(previousMasterNode()); - } - } else { - if (masterNodeChanged()) { - sb.append("master {new ").append(newMasterNode()); - if (newMasterNode() != null) { - sb.append("new ").append(newMasterNode()); - } else { - sb.append("left"); - } - if (previousMasterNode() != null) { - sb.append(", previous ").append(previousMasterNode()); - } - sb.append("}"); + final StringBuilder summary = new StringBuilder(); + if (masterNodeChanged()) { + summary.append("master node updated {previous ["); + summary.append(previousMasterNode()); + summary.append("], current ["); + summary.append(newMasterNode()); + summary.append("]}"); + } + if (removed()) { + if (summary.length() > 0) { + summary.append(", "); } - if (removed()) { - if (masterNodeChanged()) { - sb.append(", "); - } - sb.append("removed {"); - for (DiscoveryNode node : removedNodes()) { - sb.append(node).append(','); - } - sb.append("}"); + summary.append("removed {"); + for (DiscoveryNode node : removedNodes()) { + summary.append(node).append(','); } + summary.append("}"); } if (added()) { // don't print if there is one added, and it is us if (!(addedNodes().size() == 1 && addedNodes().get(0).getId().equals(localNodeId))) { - if (removed() || masterNodeChanged()) { - sb.append(", "); + if (summary.length() > 0) { + summary.append(", "); } - sb.append("added {"); + summary.append("added {"); for (DiscoveryNode node : addedNodes()) { if (!node.getId().equals(localNodeId)) { // don't print ourself - sb.append(node).append(','); + summary.append(node).append(','); } } - sb.append("}"); + summary.append("}"); } } - return sb.toString(); + return summary.toString(); } } From 5c814b7f9ea7f26adf1c13ee3355f2a4c3857b63 Mon Sep 17 00:00:00 2001 From: Tanguy Leroux Date: Thu, 18 Jan 2018 12:00:16 +0100 Subject: [PATCH 3/4] Apply feedback 3 --- .../cluster/node/DiscoveryNodes.java | 33 ++++++++----------- 1 file changed, 13 insertions(+), 20 deletions(-) diff --git a/server/src/main/java/org/elasticsearch/cluster/node/DiscoveryNodes.java b/server/src/main/java/org/elasticsearch/cluster/node/DiscoveryNodes.java index ea5bd2e51d134..43b9a565b40cc 100644 --- a/server/src/main/java/org/elasticsearch/cluster/node/DiscoveryNodes.java +++ b/server/src/main/java/org/elasticsearch/cluster/node/DiscoveryNodes.java @@ -206,12 +206,14 @@ public DiscoveryNode getLocalNode() { } /** - * Get the master node - * - * @return master node + * Returns the master node, or {@code null} if there is no master node */ + @Nullable public DiscoveryNode getMasterNode() { - return nodes.get(masterNodeId); + if (masterNodeId != null) { + return nodes.get(masterNodeId); + } + return null; } /** @@ -399,16 +401,7 @@ public Delta delta(DiscoveryNodes other) { } } - DiscoveryNode previousMasterNode = null; - if (other.masterNodeId != null) { - previousMasterNode = other.getMasterNode(); - } - DiscoveryNode newMasterNode = null; - if (masterNodeId != null) { - newMasterNode = getMasterNode(); - } - - return new Delta(previousMasterNode, newMasterNode, localNodeId, Collections.unmodifiableList(removed), + return new Delta(other.getMasterNode(), getMasterNode(), localNodeId, Collections.unmodifiableList(removed), Collections.unmodifiableList(added)); } @@ -432,8 +425,8 @@ public String toString() { public static class Delta { private final String localNodeId; - private final DiscoveryNode previousMasterNode; - private final DiscoveryNode newMasterNode; + @Nullable private final DiscoveryNode previousMasterNode; + @Nullable private final DiscoveryNode newMasterNode; private final List removed; private final List added; @@ -451,15 +444,15 @@ public boolean hasChanges() { } public boolean masterNodeChanged() { - String newMasterId = (newMasterNode != null) ? newMasterNode.getEphemeralId() : null; - String previousMasterId = (previousMasterNode != null) ? previousMasterNode.getEphemeralId() : null; - return Objects.equals(newMasterId, previousMasterId) == false; + return Objects.equals(newMasterNode, previousMasterNode) == false; } + @Nullable public DiscoveryNode previousMasterNode() { return previousMasterNode; } + @Nullable public DiscoveryNode newMasterNode() { return newMasterNode; } @@ -483,7 +476,7 @@ public List addedNodes() { public String shortSummary() { final StringBuilder summary = new StringBuilder(); if (masterNodeChanged()) { - summary.append("master node updated {previous ["); + summary.append("master node changed {previous ["); summary.append(previousMasterNode()); summary.append("], current ["); summary.append(newMasterNode()); From 6d2faea7e937efe2335b4841a8a7e5e9408841bd Mon Sep 17 00:00:00 2001 From: Tanguy Leroux Date: Thu, 18 Jan 2018 12:07:14 +0100 Subject: [PATCH 4/4] print empty brackets instead of [null] --- .../org/elasticsearch/cluster/node/DiscoveryNodes.java | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/server/src/main/java/org/elasticsearch/cluster/node/DiscoveryNodes.java b/server/src/main/java/org/elasticsearch/cluster/node/DiscoveryNodes.java index 43b9a565b40cc..057d37d5999a2 100644 --- a/server/src/main/java/org/elasticsearch/cluster/node/DiscoveryNodes.java +++ b/server/src/main/java/org/elasticsearch/cluster/node/DiscoveryNodes.java @@ -477,9 +477,13 @@ public String shortSummary() { final StringBuilder summary = new StringBuilder(); if (masterNodeChanged()) { summary.append("master node changed {previous ["); - summary.append(previousMasterNode()); + if (previousMasterNode() != null) { + summary.append(previousMasterNode()); + } summary.append("], current ["); - summary.append(newMasterNode()); + if (newMasterNode() != null) { + summary.append(newMasterNode()); + } summary.append("]}"); } if (removed()) {