[Zen2] Write manifest file (new implementation) #35199

andrershov · 2018-11-02T11:07:26Z

Elasticsearch node is responsible for storing cluster metadata.
There are 2 types of metadata: global metadata and index metadata.
GatewayMetaState implements ClusterStateApplier and receives all
ClusterStateChanged events and is responsible for storing modified
metadata to disk.

When new ClusterStateChanged event is received, GatewayMetaState
checks if global metadata has changed and if it's the case writes new
global metadata to disk. After that GatewayMetaState checks if index
metadata has changed or there are new indices assigned to this node and
if it's the case writes new index metadata to disk. Atomicity of global
metadata and index metadata writes is ensured by MetaDataStateFormat
class.

Unfortunately, there is no atomicity when more when one metadata changes
(global and index, or metadata for two indices). And atomicity is
important for Zen2 correctness.
This commit adds atomicity by adding a notion of manifest file,
represented by MetaState class. MetaState contains pointers to
current metadata.
More precisely, it stores global state generation as long and map from
Index to index metadata generation as long. Atomicity of writes for
manifest file is ensured by MetaStateFormat class.

The algorithm of writing changes to the disk would be the following:

Write global metadata state file to disk and remember
it's generation.
For each new/changed index write state file to disk and remember
it's generation. For each not-changed index use generation from
previous manifest file. If index is removed or this node is no longer
responsible for this index - forget about the index.
Create MetaState object using previously remembered generations and
write it to disk.
Remove old state files for global metadata, indices metadata and
manifest.
Additonally new implementation relies on enhanced MetaDataStateFormat
failure semantics, applyClusterState throws IOException, whose
descendant WriteStateException could be (and should be in Zen2)
explicitly handled.

Obsoletes #35049. This PR
tries to minimize number of changes to GatewayMetaState to make it
easier to review.

elasticmachine · 2018-11-02T11:07:28Z

Pinging @elastic/es-distributed

DaveCTurner · 2018-11-05T09:01:09Z

CI failure is sporadic and addressed by #35198. @elasticmachine retest this please.

DaveCTurner

Great, much easier to review, partly thanks to our conversation last week and partly because there are fewer changes. I have left a number of comments, mainly small points about naming and other nits, but also asking for some stronger assertions or other tweaks to the tests. The overall shape of this change is good.

DaveCTurner · 2018-11-05T09:04:46Z

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

+        }
+    }
+
+    private static final String META_STATE_FILE_PREFIX = "meta-";


I think we overuse the prefix meta- in the names of things. The class name MetaState makes me a bit uneasy: there's already lots of Meta* classes that I find hard to keep track of. We've been calling this a manifest, and I think we should use this terminology in the code to better describe what this class is actually doing.

DaveCTurner · 2018-11-05T09:07:44Z

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

+    }
+
+    private static final class IndexEntry implements ToXContentFragment {
+        private static final ParseField INDEX_GENERATION_PARSE_FIELD = new ParseField("generation");


Nit: could you reverse the order of these two fields so they're in the same order as they appear in the parser?

DaveCTurner · 2018-11-05T09:08:54Z

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

+ */
+public class MetaState implements ToXContentFragment {
+    private final long globalStateGeneration;
+    private final Map<Index, Long> indices;


Perhaps globalMetadataGeneration and indexMetadataGeneration? Maybe the Metadata in the middle is unncessary.

globalGeneration and indexGeneration

DaveCTurner · 2018-11-05T09:12:25Z

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

+    }
+
+    public static MetaState empty() {
+        return new MetaState(-1, Collections.emptyMap());


I think -1 should be a named constant (e.g. MANIFEST_MISSING) and we should have assertions that we never write out a manifest file with such a generation.

No longer needed after MetaStateService refactoring

DaveCTurner · 2018-11-05T09:18:21Z

server/src/main/java/org/elasticsearch/gateway/GatewayMetaState.java

                    }
                    for (IndexMetaData indexMetaData : upgradedMetaData) {
                        if (metaData.hasIndexMetaData(indexMetaData) == false) {
                            metaStateService.writeIndex("upgrade", indexMetaData);
+                        } else {
+                            metaStateService.writeIndex("startup", indexMetaData);


I think we only need to write out an unchanged index metadata file if the manifest file was missing. If it's present in a manifest file then we know it was fsynced, and I think rewriting every index's metadata on startup will substantially slow down the startup of a node. (The same is true of the global metadata, but the effects of that on startup time will be less pronounced.)

Added isBwcMode to MetaStateService to check this

DaveCTurner · 2018-11-05T10:14:33Z

server/src/test/java/org/elasticsearch/gateway/MetaStateServiceTests.java

+            assertThat(loadedMetaData.index("index"), equalTo(index_v2));
+        }
+    }
+


Nit: extra whitespace here.

DaveCTurner · 2018-11-05T10:17:03Z

server/src/test/java/org/elasticsearch/gateway/MetaStateServiceTests.java

 import org.elasticsearch.test.ESTestCase;

 import static org.hamcrest.Matchers.equalTo;
+import static org.hamcrest.Matchers.is;
 import static org.hamcrest.Matchers.nullValue;

 public class MetaStateServiceTests extends ESTestCase {


I think these tests should assert that the MetaStateService is in its reset state after writing the manifest file. Maybe they should also sometimes construct a new MetaStateService when reading things back in.

DaveCTurner · 2018-11-05T10:21:01Z

test/framework/src/main/java/org/elasticsearch/test/InternalTestCluster.java

+     */
+    public static class FullRestartCallback extends RestartCallback {
+        public void onAllNodesStopped(List<String> nodeNames) throws Exception {
+


Nit: blank line

DaveCTurner · 2018-11-05T10:21:50Z

server/src/test/java/org/elasticsearch/gateway/MetaStateServiceTests.java

+            assertThat(loadedMetaData.index("index"), equalTo(index_v1));
+
+            MetaState.FORMAT.cleanupOldFiles(Long.MAX_VALUE, env.nodeDataPaths()); // this will erase manifest file
+            loadedMetaData = metaStateService.loadMetaData(); //this must load new metadata, because manifest file is gone


I think this should be a different MetaStateService instance again.

DaveCTurner · 2018-11-05T10:24:48Z

server/src/test/java/org/elasticsearch/gateway/MetaStateServiceTests.java

+            MetaState.FORMAT.cleanupOldFiles(Long.MAX_VALUE, env.nodeDataPaths()); // this will erase manifest file
+            loadedMetaData = metaStateService.loadMetaData(); //this must load new metadata, because manifest file is gone
+            assertTrue(MetaData.isGlobalStateEquals(loadedMetaData, metaData_v2));
+            assertThat(loadedMetaData.index("index"), equalTo(index_v2));


I think we should also verify that we can correctly go from not having a manifest file to having one, because that's the sequence that we want to support.

andrershov · 2018-11-06T11:57:08Z

@DaveCTurner thank you for the review. I've resolved your comments and have performed the major refactoring of MetaStateService - now it only loads Manifest file and MetaData on the startup and after that keeps track of them in memory. The previous implementation was incorrect because loadMetadata could be called concurrently with applying new cluster event. Could you do you please make a second pass?

ywelsch

Prior to this PR, MetaStateService was stateless and only GatewayMetaState implemented the logic for incrementally writing out the state. Now this seems to be somewhat shared with the two classes being stateful. I don't see the benefit of having this extra level of abstraction that's now offered by MetaStateService. Can we fold those two classes together into one?

On a related note, I think we can simplify the implementation by only writing the cluster state atomically on master-eligible nodes. On data-only nodes (which have the extra logic for closed indices as well as only writing out the index metadata for the indices that they have) I think it's ok if we don't write out the cluster state atomically, as it's only used for dangling indices functionality, which already has weak consistency guarantees.

With the two points above, I think we should look at having two separate methods for master-eligible and data-only nodes, where both can hopefully be simpler. Let's discuss this tomorrow.

ywelsch · 2018-11-06T13:45:46Z

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

+        return (Long) generationAndListOfIndexEntries[0];
+    }
+
+    private static Map<Index, Long> indicies(Object[] generationAndListOfIndexEntries) {


s/indicies/indices

ywelsch · 2018-11-06T13:47:41Z

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

+    }
+
+    private static final ConstructingObjectParser<Manifest, Void> PARSER = new ConstructingObjectParser<>(
+            "state",


s/state/manifest?

ywelsch · 2018-11-06T14:05:58Z

server/src/main/java/org/elasticsearch/gateway/GatewayMetaState.java

                    if (MetaData.isGlobalStateEquals(metaData, upgradedMetaData) == false) {
                        metaStateService.writeGlobalState("upgrade", upgradedMetaData);
+                    } else {
+                        if (metaStateService.isBwcMode()) {


do we need to expose bwc mode? Could we instead just do the keepOrWrite internally based on BWC mode?

MetaStateService does not perform metadata upgrade on startup. Actually, it never initiates writes itself. It's the responsibility to GatewayMetaState to understand what needs to be written. Here we have pretty sophisticated requirement that we want to write non-upgraded metadata only if we're working in BWC mode. In case we're working in non-BWC mode, we want to re-write manifest only. That's why this flag is exposed.

ywelsch · 2018-11-06T14:08:15Z

server/src/main/java/org/elasticsearch/gateway/GatewayMetaState.java

            } catch (Exception e) {
-                logger.error("failed to read local state, exiting...", e);
+                logger.error("failed to bootstrap, exiting...", e);


why call this bootstrap? Maybe "failed to process on-disk cluster state at startup"?

ywelsch · 2018-11-06T14:19:21Z

server/src/main/java/org/elasticsearch/gateway/GatewayMetaState.java

+    public MetaData getMetaData() {
+        MetaData metaData = metaStateService.getMetaData();
+        //although metadata returned by MetaStateService is nullable, our callers expect non-nullable metadata
+        return metaData == null ? MetaData.builder().build() : metaData;


MetaData.EMPTY_META_DATA

ywelsch · 2018-11-06T14:20:02Z

server/src/main/java/org/elasticsearch/gateway/GatewayMetaState.java

                throw e;
            }
        }
    }

-    public MetaData loadMetaState() throws IOException {
-        return metaStateService.loadFullState();
+    public MetaData getMetaData() {


maybe call this getMetaDataOrDefault()

ywelsch · 2018-11-06T16:17:10Z

server/src/main/java/org/elasticsearch/gateway/GatewayMetaState.java

+                metaStateService.writeManifest("changed");
+                previousMetaData = newMetaData;
+            } catch (WriteStateException e) {
+                logger.error("Exception occurred", e);


can you provide a better exception message here? e.g. "failed to persist cluster state " + event.state().version()

andrershov · 2018-11-06T19:45:33Z

@ywelsch I've resolved issues that you pointed out to inline in aae96cc.
Regarding GatewayMetaState and MetaStateService responsibilities. MetaStateService is responsible for writing/loading metadata to/from disk, so that callers do not worry about the need to keep manifest file up-to-date and the need to be able to support bwc mode, when no manifest file is available. MetaStateService never writes something w/o explicit request from the outside.
GatewayMetaState on the other hand is responsible for processing ClusterStateChange events and figuring out what needs to be re-written.
To cut a long story short, GatewayMetaState is what and MetaStateService is how.
Also, David has requested to make only small modifications to GatewayMetaState to make code easier to review.
One more thing is that our ultimate goal is to implement PersistedState interface that would be different from GatewayMetaState and this implementation will most likely re-use existing MetaStateService functionality.

Andrey Ershov added 5 commits November 1, 2018 15:56

Add MetaState class and tests

aa590e9

MetaDataStateFormat changes

c6150ab

Write manifest file and fix tests

49cbd17

Remove load* methods, MetaStateService tests, autoCleanup override

7a6b2e7

Auto-cleanup as method argument

e3e179b

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

andrershov self-assigned this Nov 2, 2018

andrershov requested a review from DaveCTurner November 2, 2018 11:07

DaveCTurner reviewed Nov 5, 2018

View reviewed changes

Andrey Ershov added 18 commits November 5, 2018 22:56

MetaStateService cache metaData, write metadata on startup for bwc only

123eb79

Rename MetaState to Manifest

ebf0a1b

Change order of ParseField in Manifest

ad8efaa

Rename globalGeneration and indexGenerations

47a9397

write flag -> writeAndCleanup method

55eaedd

of -> if

465347f

state id -> state generation

cc953ab

zen 1 -> manifest-less

8f8e32f

hasNoPendingWrites assertion

9175285

Add pending* prefix to MetaStateService fields

55be8dd

ManifestTest: reformat, use eq&hash utils, enrich mutation

1eee534

FileToCorrupt = file

b072c4f

Remove ToDO

d4250c3

indexSettings -> indexMetadata

6d178c5

test rename

d9ea0a1

empty lines remove

239a242

maybeNewMetaStateService and assert no pending writes

15b5059

Restore manifest file in MetaStateService bwc test

ad0fcdb

Fix codestyle

35698ad

ywelsch suggested changes Nov 6, 2018

View reviewed changes

Fix code review 2 issues

aae96cc

andrershov mentioned this pull request Nov 6, 2018

Zen2 ClusterState storage #33958

Closed

6 tasks

andrershov closed this Nov 12, 2018

[Zen2] Write manifest file (new implementation) #35199

[Zen2] Write manifest file (new implementation) #35199

Conversation

andrershov commented Nov 2, 2018 • edited Loading

elasticmachine commented Nov 2, 2018

DaveCTurner commented Nov 5, 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

andrershov Nov 5, 2018 • edited Loading

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

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 6, 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

Choose a reason for hiding this comment

andrershov commented Nov 6, 2018

andrershov commented Nov 2, 2018 •

edited

Loading

andrershov Nov 5, 2018 •

edited

Loading