[Zen2] Move ClusterState fields to be persisted to ClusterState.MetaData #35625

andrershov · 2018-11-16T09:56:20Z

Today we have a way to atomically persist global MetaData and
IndexMetaData to disk when new ClusterState is received. All other
ClusterState fields are not persisted.
However, there are other parts of ClusterState that should be
persisted, namely:

version
term
lastCommittedConfiguration
lastAcceptedConfiguration
votingTombstones

version is changed frequently, other fields are not. We decided
to group term, lastCommittedConfiguration,
lastAcceptedConfiguration and votingTombstones into
CoordinationMetaData class and make CoordinationMetaData a field
inside MetaData.
MetaData.toXContent and MetaData.fromXContent should take care of
CoordinationMetaData.
version stays as a top level field in ClusterState and will be
persisted as part of Manifest in a follow-up PR.
Also MetaData.isGlobalStateEquals should be extended to include
coordinationMetaData in comparison.

This PR favors to expose getters, such as getTerm directly in
ClusterState to avoid massive code changes.

An example of CoordinationMetaState.toXContent:

{
  "term": 1,
  "last_committed_config": [
    "TiIuBcbBtpuXyDDVHXeD",
    "ZIAoVbkjjLPLUuYLaTkw"
  ],
  "last_accepted_config": [
    "OwkXbXZNOZPJqccdFHdz",
    "LouzsGYwmQzpeQMrboZe",
    "fCKGRZdjLTqzXAqPUtGL",
    "pLoxshjpJXwDhbgjfYJy",
    "SjINLwFIlIEFZCbjrSFo",
    "MDkVncJEVyZLJktopWje"
  ]
}

elasticmachine · 2018-11-16T09:56:21Z

Pinging @elastic/es-distributed

DaveCTurner

Looks good. It is a little sad that building these cluster states gets so much more verbose in places, and there's a few places where I'm not sure we're using the builders quite right yet. I left a selection of small comments/requests.

DaveCTurner · 2018-11-16T13:58:57Z

...org/elasticsearch/action/admin/cluster/configuration/TransportAddVotingTombstonesAction.java

                resolvedNodes = resolveNodesAndCheckMaximum(request, currentState);

-                final Builder builder = ClusterState.builder(currentState);
+                CoordinationMetaData.Builder builder = CoordinationMetaData.builder(currentState.coordinationMetaData());


For consistency, could this be final please?

DaveCTurner · 2018-11-16T14:03:29Z

server/src/main/java/org/elasticsearch/cluster/ClusterState.java


-    public VotingConfiguration getLastAcceptedConfiguration() {
-        return lastAcceptedConfiguration;
+    public CoordinationMetaData.VotingConfiguration getLastAcceptedConfiguration() {


I think we could just import VotingConfiguration rather than qualifying it like this.

server/src/main/java/org/elasticsearch/cluster/ClusterState.java

DaveCTurner · 2018-11-16T14:40:23Z

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

-            setLastAcceptedState(ClusterState.builder(lastAcceptedState)
-                .lastCommittedConfiguration(lastAcceptedState.getLastAcceptedConfiguration())
-                .build());
+            final CoordinationMetaData cmd = CoordinationMetaData.builder(lastAcceptedState.coordinationMetaData())


Nit: cmd reads as "command" to me. Could we just give it the full type name coordinationMetaData?

DaveCTurner · 2018-11-16T14:44:43Z

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

-            builder.lastAcceptedConfiguration(votingConfiguration);
-            builder.lastCommittedConfiguration(votingConfiguration);
+            final CoordinationMetaData coordinationMetaData = CoordinationMetaData.builder()
+                .term(currentState.term())


currentState.term() is 0 here, so I think this isn't required. But I also wonder if it'd be better to say CoordinationMetaData.builder(currentState.coordinationMetaData()) in the line above to be clear that we're updating the existing metadata rather than starting afresh.

DaveCTurner · 2018-11-16T14:54:31Z

...lasticsearch/action/admin/cluster/configuration/TransportAddVotingTombstonesActionTests.java

                    return builder(currentState)
-                        .lastAcceptedConfiguration(votingConfiguration)
-                        .lastCommittedConfiguration(votingConfiguration).build();
+                        .metaData(MetaData.builder()


This discards the metadata in the cluster state (except it copies the coordination metadata over)

DaveCTurner · 2018-11-16T14:56:37Z

server/src/test/java/org/elasticsearch/cluster/ClusterStateDiffIT.java

-                new ClusterState.VotingConfiguration(Sets.newHashSet(generateRandomStringArray(10, 10, false))));
+            metaBuilder.lastAcceptedConfiguration(
+                new CoordinationMetaData.VotingConfiguration(Sets.newHashSet(generateRandomStringArray(10, 10, false))));
        }


Oops I missed the voting tombstones here. Could you add one?

DaveCTurner · 2018-11-16T14:57:47Z

server/src/test/java/org/elasticsearch/cluster/ClusterStateDiffIT.java

-    private ClusterState.Builder randomVotingConfiguration(ClusterState clusterState) {
+    private ClusterState.Builder randomCoordinationMetaData(ClusterState clusterState) {
        ClusterState.Builder builder = ClusterState.builder(clusterState);
+        CoordinationMetaData.Builder metaBuilder = CoordinationMetaData.builder();


You need to put this builder into the other builder, otherwise it's just discarded.

DaveCTurner · 2018-11-16T15:03:20Z

server/src/test/java/org/elasticsearch/cluster/ClusterStateDiffIT.java

                }
            }
-            clusterState = builder.incrementVersion().term(randomLong()).build();
+            clusterState = builder.incrementVersion().build();


This test asserts that various parts of the coordination metadata are equal. I think it should now just assert that the whole thing is equal.

DaveCTurner · 2018-11-16T15:07:53Z

server/src/main/java/org/elasticsearch/cluster/metadata/MetaData.java

+            if (in.getVersion().onOrAfter(Version.V_7_0_0)) {
+                coordinationMetaData = new CoordinationMetaData(in);
+            } else {
+                coordinationMetaData = CoordinationMetaData.EMPTY_META_DATA;


Depending on how we plan to do rolling upgrades, this may or may not be safe, and might need backporting. We're not sure yet. Could you leave a // TODO check that this is safe here so we know to revisit it later on.

DaveCTurner · 2018-11-16T15:12:59Z

Ah yes and there's another place where we assert the literal JSON representation of the cluster state: org.elasticsearch.xpack.monitoring.collector.cluster.ClusterStatsMonitoringDocTests.testToXContent. That's caught me out before.

andrershov · 2018-11-19T17:01:03Z

@DaveCTurner thanks for the diligent review! I've made changes that you're asking for. Ready for the next pass.

DaveCTurner

LGTM, I left three further suggestions.

However I am not too sure about XContent parsing so would like @ywelsch to look over that part specifically.

DaveCTurner · 2018-11-20T14:51:09Z

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

+        return (long)termAndConfigs[0];
+    }
+
+    private static VotingConfiguration lastCommittedConfig(Object[] termAndConfig) {


Could you add @SuppressWarnings("unchecked") to this method?

DaveCTurner · 2018-11-20T14:51:14Z

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

+        return new VotingConfiguration(new HashSet<>(nodeIds));
+    }
+
+    private static VotingConfiguration lastAcceptedConfig(Object[] termAndConfig) {


Could you add @SuppressWarnings("unchecked") to this method?

DaveCTurner · 2018-11-20T14:53:36Z

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

+
+        public CoordinationMetaData build() {
+            return new CoordinationMetaData(term, lastCommittedConfiguration, lastAcceptedConfiguration,
+                Collections.unmodifiableSet(new HashSet<>(votingTombstones)));


Now that I see all this in one place, I see that in VotingConfiguration we copy the set and wrap it in Collections.unmodifiableSet in the constructor whereas here I did it in the builder. Could you move it to within the constructor like with VotingConfiguration?

andrershov · 2018-11-20T18:37:40Z

I'm done with the changes. Waiting for @ywelsch to take a look at CoordinationMetaData.toXContent and CoordinationMetaData.fromXContent, as well as MetaData.toXContent and MetaData.fromXContent.

ywelsch

I've left some smaller comments. I would like for this PR to implement toXContent/fromXContent for the voting tombstones as well.

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

ywelsch · 2018-11-20T22:06:03Z

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

+
+    @Override
+    public XContentBuilder toXContent(XContentBuilder builder, Params params) throws IOException {
+        return builder


can you add an example to the PR description how the new toXContent will look like?

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

server/src/main/java/org/elasticsearch/cluster/metadata/MetaData.java

...ava/org/elasticsearch/xpack/monitoring/collector/cluster/ClusterStatsMonitoringDocTests.java

ywelsch

LGTM

Andrey Ershov added 4 commits November 15, 2018 12:57

CoordinationState inside ClusterState

dc6eca4

Bug fix

b392435

Move CoordinationMetaData to MetaData

f04e55b

CoordinationMetaData.fromXContent, toString and isGlobalStateEquals

8d546ae

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

andrershov requested a review from ywelsch November 16, 2018 09:56

Merge zen2 into zen2_coordination_state

41f0394

DaveCTurner requested changes Nov 16, 2018

View reviewed changes

DaveCTurner added the v7.0.0 label Nov 16, 2018

Andrey Ershov added 12 commits November 19, 2018 17:00

final builder

4480147

import VotingConfiguration

58c856f

cmd -> coordinationMetaData, cmdb -> coordinationMetaDataBuilder

a0e86bf

Reuse old coordination metadata

ef622d7

do not loose old metadata

9232c57

do not loose old metadata 2

143a9ec

do not loose old metadata 3

c3b0776

Add voting tombstone to coordination meta in test

4393666

do not loose coordination meta data

452b91e

squash fields equals assert into coordination metadata eq assert

04e2324

Add Zen2 BWC TODO

9006e32

Fix ClusterStatsMonitoringDocTests

ef5341e

andrershov removed the request for review from ywelsch November 19, 2018 16:59

andrershov mentioned this pull request Nov 19, 2018

Zen2 ClusterState storage #33958

Closed

6 tasks

Fix bug

91171e9

DaveCTurner approved these changes Nov 20, 2018

View reviewed changes

DaveCTurner requested a review from ywelsch November 20, 2018 15:10

Andrey Ershov added 2 commits November 20, 2018 19:31

@SuppressWarnings("unchecked")

16c4884

Move set copy from builder to constructor

310d65a

ywelsch suggested changes Nov 20, 2018

View reviewed changes

Andrey Ershov added 2 commits November 21, 2018 13:38

Remove log from test

39392e1

Rename JSON fields

d99c52d

ywelsch approved these changes Nov 21, 2018

View reviewed changes

Andrey Ershov added 3 commits November 21, 2018 13:53

MetaData.fromXContent fix

219b569

Merge zen2 into zen2_coordination_state

ec94914

Merge branch zen2 into zen2_coordination_state

482ae3f

andrershov merged commit a056bd8 into elastic:zen2 Nov 21, 2018

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

[Zen2] Move ClusterState fields to be persisted to ClusterState.MetaData #35625

[Zen2] Move ClusterState fields to be persisted to ClusterState.MetaData #35625

Uh oh!

Conversation

andrershov commented Nov 16, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

elasticmachine commented Nov 16, 2018

Uh oh!

DaveCTurner left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

andrershov Nov 19, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

DaveCTurner commented Nov 16, 2018

Uh oh!

andrershov commented Nov 19, 2018

Uh oh!

DaveCTurner left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

andrershov commented Nov 20, 2018

Uh oh!

ywelsch left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

andrershov commented Nov 16, 2018 •

edited

Loading

andrershov Nov 19, 2018 •

edited

Loading