Switch MetaDataStateFormat to Lucene directory abstraction #33989

andrershov · 2018-09-24T10:38:21Z

Switching to Lucene directory abstraction for MetaDataStateFormat to be able to inject failure during writing state to disk.
Adding new tests to MetaDataStateFormat to inject these failures.

elasticmachine · 2018-09-24T10:38:22Z

Pinging @elastic/es-distributed

ywelsch

I've done an initial pass. I'm missing a test where we inject failures at random during writes and check whether we either successfully load the previous or the newly written state (i.e. atomicity).

ywelsch · 2018-09-24T13:29:56Z

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

-                    Files.deleteIfExists(stateFile);
-                    logger.trace("cleanupOldFiles: cleaned up {}", stateFile);
+    private void cleanupOldFiles(final String currentStateFile, Path[] locations) throws IOException {
+        for (Path location: locations){


space missing before : and before {

ywelsch · 2018-09-24T13:30:41Z

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

+        for (Path location: locations){
+            Path stateLocation = location.resolve(STATE_DIR_NAME);
+            try (Directory stateDir = newDirectory(stateLocation)){
+                for (String file : stateDir.listAll()){


space missing before { (same for previous line)
Also check the rest of PR for these occurrences

Tip, just in case: you can let intellij auto format a selection (and not the entire file) but selecting it and choosing reformat code from the Code menu (or using whatever keyboard shortcut you have).

Sorry about that, I know about format option and use it, but sometimes forget to apply it. Unfortunately, there is no good way for IDEA to do it safe and automatically for you

ywelsch · 2018-09-24T13:32:44Z

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

-                    Files.deleteIfExists(tmpPath);
-                    logger.trace("cleaned up {}", tmpPath);
-                }
+            for (Path extraLocation : locations) {


overriding the first location again here?

Good catch!

ywelsch · 2018-09-24T13:39:49Z

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

+        final Path firstStateLocation = locations[0].resolve(STATE_DIR_NAME);
+        try (Directory stateDir = newDirectory(firstStateLocation)) {
+            writeStateToFirstLocation(state, stateDir, fileName, tmpFileName);
+            logger.trace("written state to {}", firstStateLocation.resolve(fileName));


move log line into writeStateToFirstLocation method?

It's done in this way because there I did not want to pass firstStateLocation path to writeStateToFirstLocation method just for logging purposes. Unfortunately, there is no method like resolve in Lucene directory. But fixed anyway

ywelsch · 2018-09-24T13:40:07Z

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

+            for (Path extraLocation : locations) {
+                final Path extraStateLocation = extraLocation.resolve(STATE_DIR_NAME);
+                copyStateToExtraLocation(stateDir, extraStateLocation, fileName, tmpFileName);
+                logger.trace("copied state to {}", extraStateLocation.resolve(fileName));


move log line into copyStateToExtraLocation method?

Here it was done to make symmetrical with writeStateToFirstLocation, fixed

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

ywelsch · 2018-09-24T14:05:26Z

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

+        DummyState initialState = writeAndReadStateSuccessfully(format, injectFailures, path);
+        possibleStates.add(initialState);
+
+        for (int i=0; i<randomIntBetween(1,5); i++) {


spaces missing...

ywelsch · 2018-09-24T14:09:17Z

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

+                    public void eval(MockDirectoryWrapper dir) throws IOException {
+                        for (StackTraceElement e : Thread.currentThread().getStackTrace()) {
+                                if (failMethod.equals(e.getMethodName())) {
+                                    throw new MockDirectoryWrapper.FakeIOException();


this means we always fail on the first call in this method. I wonder if will not get enough coverage by always failing on the first call

Yes, it will fail on the first method call which name equals to one of randomly selected from failure methods.
For example, if you look at "testFailWriteAndReadPreviousState" you will find that failure methods are the following: "createOutput", "sync", "rename". If we run the sufficient number of test iterations, we should get good coverage. Plus we perform more than one state write in each test method, and each of this method could fail at randomly chosen method.

ywelsch · 2018-09-24T16:02:55Z

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

+         * @param failureMethods - one of these method calls will fail on Directory object if injectFailure is set to true.
+         *                       Method that will fail is randomly selected on each newDirectory method call.
+         */
+        Format(String prefix, AtomicBoolean injectFailure, String... failureMethods) {


instead of injecting the AtomicBoolean, it might be nicer to just create it internally, and access it on the Format object

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

andrershov · 2018-09-26T18:58:01Z

@ywelsch thanks for the review! I've made necessary changes in afa9a3b. Please let me know if something else is needed.
By injecting failures during writes did you mean, that we also should test for failures when writing bytes of the temporary file to disk? If yes, I've addressed it.

ywelsch

I meant adding a test that injects a failure completely at random (without matching on specific method names). There are two more logging issues.

ywelsch · 2018-09-27T15:34:14Z

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

-        };
-        // now clean up the old files
-        for (Path dataLocation : locations) {
-            logger.trace("cleanupOldFiles: cleaning up {}", dataLocation);


This looks to be still missing.

ywelsch · 2018-09-27T15:35:31Z

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

+                for (String file : stateDir.listAll()) {
+                    if (file.startsWith(prefix) && file.equals(currentStateFile) == false) {
+                        deleteTempFile(stateLocation, stateDir, file);
+                        logger.trace("cleanupOldFiles: cleaned up {}", stateLocation.resolve(file));


this is already logged part of deleteTempFile?

Fixed, also renamed deleteTempFile method, because when used in cleanup it's not temp file, but previous state

andrershov · 2018-10-01T11:46:32Z

@ywelsch I've fixed logging issues and added the random test. Random test discovered the bug, that we don't take temp files into account when looking up max state id. I've fixed that.
Also, I've refactored loadLatestState method to remove code duplication.
May I ask you to kindly perform one more code review pass?

ywelsch

Thanks for adding the extra test. I've added a few more comments.

ywelsch · 2018-10-01T20:54:37Z

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

                return state;
            } catch (Exception e) {
-                exceptions.add(new IOException("failed to read " + pathAndStateId.toString(), e));
+                exceptions.add(new IOException("failed to read " + stateFile.getFileName(), e));


this should have the absolute path in it, I think, not just the file name.

ywelsch · 2018-10-01T21:00:43Z

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

-                .filter(pathAndStateId -> pathAndStateId.id == finalMaxStateId)
-                .collect(Collectors.toCollection(ArrayList::new));
+        long maxStateId = findMaxStateId(prefix, dataLocations);
+        List<Path> stateFiles = findStateFilesByGeneration(maxStateId, dataLocations);


If maxStateId > 0 and stateFiles.isEmpty() I think we should throw an exception here as it clearly indicates something went horribly wrong (e.g. concurrent writes or deletes). That same condition was previously covered by distinguishing between pathAndStateIds and files.

ywelsch · 2018-10-02T07:05:52Z

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

+        for (Path dataLocation : locations) {
+            final Path stateFilePath = dataLocation.resolve(STATE_DIR_NAME).resolve(fileName);
+            if (Files.exists(stateFilePath)) {
+                files.add(stateFilePath);


Let's add trace logging to this (like it was before):

logger.trace("found state file: {}", stateFilePath);

ywelsch · 2018-10-02T07:10:15Z

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

+    }
+
+    public void testFailRandomlyAndReadAnyState() throws IOException {
+        Path paths[] = new Path[randomIntBetween(2, 5)];


let's also consider the typical use case of a single data path :)

andrershov · 2018-10-15T18:34:50Z

@ywelsch thank you for another round, I've made changes in this commit 0f33a39

ywelsch

LGTM. You'll have to merge in the latest zen2 branch into your branch to get CI green here.

andrershov · 2018-10-16T15:28:09Z

run gradle build tests please

andrershov · 2018-10-17T07:11:12Z

run gradle build tests please

andrershov · 2018-10-17T09:02:29Z

run gradle build tests again

Switch MetaDataStateFormat Lucene directory abstraction

76956de

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

andrershov requested a review from ywelsch September 24, 2018 10:38

ywelsch suggested changes Sep 24, 2018

View reviewed changes

Code review fixes

afa9a3b

andrershov force-pushed the zen2_storage branch from cd55bca to afa9a3b Compare September 26, 2018 18:54

ywelsch changed the title ~~Switch MetaDataStateFormat Lucene directory abstraction~~ Switch MetaDataStateFormat to Lucene directory abstraction Sep 27, 2018

ywelsch suggested changes Sep 27, 2018

View reviewed changes

Andrey Ershov added 3 commits September 27, 2018 20:26

Fix logging issues

c400868

Refactor Format, add random test, fix bug with tmp file, fix formatting

71ca04d

Refactor loadLatestState

a1ed7b3

Delete tmp file if exists before writing the state

5b5ef4d

ywelsch suggested changes Oct 2, 2018

View reviewed changes

Fix code review issues

0f33a39

ywelsch approved these changes Oct 16, 2018

View reviewed changes

andrershov mentioned this pull request Oct 17, 2018

Zen2 ClusterState storage #33958

Closed

6 tasks

Andrey Ershov added 2 commits October 17, 2018 15:09

Fix JavaDoc and IllegalStateException message

0d8b842

Merge branch 'zen2' into zen2_storage_fix

554537e

andrershov force-pushed the zen2_storage branch from 6ceaa43 to 554537e Compare October 17, 2018 13:13

andrershov merged commit 51f38dd into elastic:zen2 Oct 17, 2018

Switch MetaDataStateFormat to Lucene directory abstraction #33989

Switch MetaDataStateFormat to Lucene directory abstraction #33989

Uh oh!

Conversation

andrershov commented Sep 24, 2018

Uh oh!

elasticmachine commented Sep 24, 2018

Uh oh!

ywelsch 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!

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 Sep 26, 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!

Uh oh!

andrershov commented Sep 26, 2018

Uh oh!

ywelsch 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!

andrershov commented Oct 1, 2018

Uh oh!

ywelsch 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!

andrershov commented Oct 15, 2018

Uh oh!

ywelsch left a comment

Choose a reason for hiding this comment

Uh oh!

andrershov commented Oct 16, 2018

Uh oh!

andrershov Sep 26, 2018 •

edited

Loading