[Zen2] VotingTombstone class #35832

andrershov · 2018-11-22T16:27:51Z

Today voting tombstones are stored in CoordinationMetaData as
Set<DiscoveryNode>.
DiscoveryNode is not a lightweight object and have a lot of fields.
It also has toXContent method, but no fromXContent method and the
output of toXContent is not enough to re-create DiscoveryNode
object.
And votingTombstone set should be persisted as a part of MetaData.
On the other hand, the only thing required from the tombstone is the
nodeId.
This PR adds VotingTombstone class for voting tombstones, which
consists of two fields for now - nodeId and nodeName. It could be
extended/shrank in the future if needed.
This PR also resolves TODO's related to the voting tombstones xcontent
story.
Example of CoordinationMetaData.toXContent with voting tombstones:

{
  "term": 1,
  "last_committed_config": [
    "fkwLdOBvXSlgRTBfgNAL",
    "tmQiPGHvUxXzPkkCDSJo",
    "HhOmtQBZAThpHIGWhxpz",
    "qZHWGpoDNPYRNIiqKsDl"
  ],
  "last_accepted_config": [
    "lhqacKmriwhHGFZcvqbx",
    "MYysmBuROkvJRlDcusyd"
  ],
  "voting_tombstones": [
    {
      "node_id": "McjbZbRkEz",
      "node_name": "pdKIWeNJUO"
    },
    {
      "node_id": "cpXkVibGwo",
      "node_name": "UnCvFgdVsc"
    },
    {
      "node_id": "EylRNOztbc",
      "node_name": "ohOhkbMWZX"
    }
  ]
}

elasticmachine · 2018-11-22T16:27:52Z

Pinging @elastic/es-distributed

DaveCTurner

Looks good, I left only nits. Again I would like @ywelsch to look over the XContent-related things.

DaveCTurner · 2018-11-23T08:34:07Z

server/src/main/java/org/elasticsearch/cluster/coordination/CoordinationMetaData.java

-            .field(LAST_ACCEPTED_CONFIGURATION_FIELD.getPreferredName(), lastAcceptedConfiguration);
-        // TODO include voting tombstones here
+                .field(LAST_ACCEPTED_CONFIGURATION_FIELD.getPreferredName(), lastAcceptedConfiguration)
+                .field(VOTING_TOMBSTONES_FIELD.getPreferredName(), votingTombstones);


nit: indenting seems inconsistent here.

DaveCTurner · 2018-11-23T08:38:00Z

server/src/test/java/org/elasticsearch/cluster/metadata/MetaDataTests.java

@@ -418,20 +415,18 @@ public void testXContentWithIndexGraveyard() throws IOException {
        return new CoordinationMetaData.VotingConfiguration(Sets.newHashSet(generateRandomStringArray(randomInt(10), 20, false)));
    }

-    private Set<DiscoveryNode> randomDiscoveryNodeSet() {
+    private Set<VotingTombstone> randomVotingTombstones() {
        final int size = randomIntBetween(1, 10);


We should probably test the empty case too.

DaveCTurner · 2018-11-23T08:39:24Z

server/src/main/java/org/elasticsearch/cluster/coordination/CoordinationMetaData.java

+        @Override
+        public String toString() {
+            StringBuilder sb = new StringBuilder();
+            sb.append('{').append(nodeId).append('}');


In DiscoveryNode#toString() the node name comes first (if present). I think we should do the same here.

DaveCTurner · 2018-11-23T08:41:01Z

server/src/main/java/org/elasticsearch/cluster/coordination/CoordinationMetaData.java

@@ -220,6 +228,97 @@ public CoordinationMetaData build() {
        }
    }

+    public static class VotingTombstone implements Writeable, ToXContentFragment {
+        private String nodeId;


I hate that things are not final by default in Java. 1037370

DaveCTurner · 2018-11-23T08:41:06Z

server/src/main/java/org/elasticsearch/cluster/coordination/CoordinationMetaData.java

@@ -220,6 +228,97 @@ public CoordinationMetaData build() {
        }
    }

+    public static class VotingTombstone implements Writeable, ToXContentFragment {
+        private String nodeId;
+        private String nodeName;


andrershov · 2018-11-23T09:50:13Z

Ok, I'm done with the changes, waiting for @ywelsch xcontent review.

ywelsch

Some minor comments. Can you also add the JSON output of a CoordinationMetaData example object with voting tombstones to the PR description? Makes it easier for me to see what the change is :) Thank you

ywelsch · 2018-11-23T12:53:58Z

...in/java/org/elasticsearch/action/admin/cluster/configuration/AddVotingTombstonesRequest.java

@@ -69,10 +70,10 @@ public AddVotingTombstonesRequest(StreamInput in) throws IOException {
        timeout = in.readTimeValue();
    }

-    Set<DiscoveryNode> resolveNodes(ClusterState currentState) {
+    Set<VotingTombstone> resolveNodes(ClusterState currentState) {


maybe this should now be called resolveVotingTombStones?

ywelsch · 2018-11-23T12:54:12Z

...in/java/org/elasticsearch/action/admin/cluster/configuration/AddVotingTombstonesRequest.java

@@ -83,8 +84,8 @@ public AddVotingTombstonesRequest(StreamInput in) throws IOException {
        return resolvedNodes;
    }

-    Set<DiscoveryNode> resolveNodesAndCheckMaximum(ClusterState currentState, int maxTombstoneCount, String maximumSettingKey) {
-        final Set<DiscoveryNode> resolvedNodes = resolveNodes(currentState);
+    Set<VotingTombstone> resolveNodesAndCheckMaximum(ClusterState currentState, int maxTombstoneCount, String maximumSettingKey) {


idem here (resolveVoting....

And forgot "AndCheckMaximumPart" 8f63927

ywelsch · 2018-11-23T12:57:27Z

...g/elasticsearch/action/admin/cluster/configuration/TransportClearVotingTombstonesAction.java

@@ -77,10 +77,10 @@ protected void masterOperation(ClearVotingTombstonesRequest request, ClusterStat
        final long startTimeMillis = threadPool.relativeTimeInMillis();

        final Predicate<ClusterState> allTombstonedNodesRemoved = newState -> {
-            for (DiscoveryNode tombstone : initialState.getVotingTombstones()) {
+            for (VotingTombstone tombstone : initialState.getVotingTombstones()) {
                // NB checking for the existence of any node with this persistent ID, because persistent IDs are how votes are counted.
                // Calling nodeExists(tombstone) is insufficient because this compares on the ephemeral ID.


the second part of the comment is now superfluous as VotingTombstone does not have the ephemeral id and there is no nodeExists method that takes a voting tombstone

andrershov · 2018-11-23T13:20:57Z

@ywelsch I've made changes you've requested for

[Zen2] VotingTombstone class

cd64b16

andrershov added >enhancement :Distributed Coordination/Cluster Coordination Cluster formation and cluster state publication, including cluster membership and fault detection. labels Nov 22, 2018

andrershov self-assigned this Nov 22, 2018

andrershov requested a review from DaveCTurner November 22, 2018 16:27

DaveCTurner reviewed Nov 23, 2018

View reviewed changes

DaveCTurner requested a review from ywelsch November 23, 2018 08:42

DaveCTurner added the v7.0.0 label Nov 23, 2018

Andrey Ershov added 4 commits November 23, 2018 10:43

Fix indentation

3657f75

Empty randomVotingTombstones

9c8d7a4

Reorder name, id in VotingTombstone to be consistent with DiscoveryNode

dbb21b2

final fields in VotingTombstone

1037370

ywelsch suggested changes Nov 23, 2018

View reviewed changes

Andrey Ershov added 2 commits November 23, 2018 14:15

resolveNodes -> resolveVotingTombstones

f72bcbd

Remove second part of the comment

4454009

resolveVotingTombstonesAndCheckMaximum

8f63927

ywelsch approved these changes Nov 23, 2018

View reviewed changes

Fix line length

e3bd0dd

andrershov merged commit f47636b into elastic:zen2 Nov 23, 2018

ywelsch mentioned this pull request Nov 28, 2018

A new cluster coordination layer #32006

Closed

61 tasks

colings86 added v7.0.0-beta1 and removed v7.0.0 labels Feb 7, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Zen2] VotingTombstone class #35832

[Zen2] VotingTombstone class #35832

andrershov commented Nov 22, 2018 •

edited

Loading

elasticmachine commented Nov 22, 2018

DaveCTurner left a comment

DaveCTurner Nov 23, 2018

andrershov Nov 23, 2018

DaveCTurner Nov 23, 2018

andrershov Nov 23, 2018

DaveCTurner Nov 23, 2018

andrershov Nov 23, 2018

DaveCTurner Nov 23, 2018

andrershov Nov 23, 2018

DaveCTurner Nov 23, 2018

andrershov Nov 23, 2018

andrershov commented Nov 23, 2018

ywelsch left a comment

ywelsch Nov 23, 2018

andrershov Nov 23, 2018

ywelsch Nov 23, 2018

andrershov Nov 23, 2018

andrershov Nov 23, 2018

ywelsch Nov 23, 2018

andrershov Nov 23, 2018

andrershov commented Nov 23, 2018

[Zen2] VotingTombstone class #35832

[Zen2] VotingTombstone class #35832

Conversation

andrershov commented Nov 22, 2018 • edited Loading

elasticmachine commented Nov 22, 2018

DaveCTurner left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andrershov commented Nov 23, 2018

ywelsch left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andrershov commented Nov 23, 2018

andrershov commented Nov 22, 2018 •

edited

Loading