From 94b4b5110c3de6e2d26d65aaff1419ce35a0bc51 Mon Sep 17 00:00:00 2001 From: Nikola Grcevski Date: Fri, 1 Oct 2021 17:21:51 -0400 Subject: [PATCH 01/11] [TEST] More MetadataStateFormat tests Add test about partial failure on multi-paths. --- .../gateway/MetadataStateFormat.java | 4 +- .../gateway/MetadataStateFormatTests.java | 50 +++++++++++++++++++ 2 files changed, 52 insertions(+), 2 deletions(-) diff --git a/server/src/main/java/org/elasticsearch/gateway/MetadataStateFormat.java b/server/src/main/java/org/elasticsearch/gateway/MetadataStateFormat.java index cc37538eaf5d4..097e77c2c8ba4 100644 --- a/server/src/main/java/org/elasticsearch/gateway/MetadataStateFormat.java +++ b/server/src/main/java/org/elasticsearch/gateway/MetadataStateFormat.java @@ -343,7 +343,7 @@ public void cleanupOldFiles(final long currentGeneration, Path... locations) { * @return maximum id of state file or -1 if no such files are found * @throws IOException if IOException occurs */ - private long findMaxGenerationId(final String prefix, Path... locations) throws IOException { + protected long findMaxGenerationId(final String prefix, Path... locations) throws IOException { long maxId = -1; for (Path dataLocation : locations) { final Path resolve = dataLocation.resolve(STATE_DIR_NAME); @@ -362,7 +362,7 @@ private long findMaxGenerationId(final String prefix, Path... locations) throws return maxId; } - private List findStateFilesByGeneration(final long generation, Path... locations) { + protected List findStateFilesByGeneration(final long generation, Path... locations) { List files = new ArrayList<>(); if (generation == -1) { return files; diff --git a/server/src/test/java/org/elasticsearch/gateway/MetadataStateFormatTests.java b/server/src/test/java/org/elasticsearch/gateway/MetadataStateFormatTests.java index f28c53f1b9a6d..3655dd8b1a43a 100644 --- a/server/src/test/java/org/elasticsearch/gateway/MetadataStateFormatTests.java +++ b/server/src/test/java/org/elasticsearch/gateway/MetadataStateFormatTests.java @@ -40,6 +40,15 @@ import static org.hamcrest.Matchers.equalTo; import static org.hamcrest.Matchers.is; import static org.hamcrest.Matchers.notNullValue; +import static org.mockito.Matchers.any; +import static org.mockito.Matchers.anyString; +import static org.mockito.Matchers.eq; +import static org.mockito.Mockito.doCallRealMethod; +import static org.mockito.Mockito.doReturn; +import static org.mockito.Mockito.doThrow; +import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.spy; +import static org.mockito.Mockito.when; @LuceneTestCase.SuppressFileSystems("ExtrasFS") // TODO: fix test to work with ExtrasFS public class MetadataStateFormatTests extends ESTestCase { @@ -344,6 +353,47 @@ public void testFailRandomlyAndReadAnyState() throws IOException { writeAndReadStateSuccessfully(format, paths); } + private Path makeMockTempDirPathHelper() { + Path realPath = createTempDir(); + Path result = spy(realPath); + + return result; + } + + public void testInconsistentMultiPathState() throws IOException { + Path paths[] = new Path[3]; + for (int i = 0; i < paths.length; i++) { + paths[i] = makeMockTempDirPathHelper(); + } + Format format = new Format("foo-"); + + DummyState state = new DummyState(randomRealisticUnicodeOfCodepointLengthBetween(1, 100), randomInt(), randomLong(), + randomDouble(), randomBoolean()); + // Call write without clean-up to simulate multi-write transaction. + long genId = format.write(state, paths); + assertEquals(state, format.loadLatestState(logger, NamedXContentRegistry.EMPTY, paths)); + ensureOnlyOneStateFile(paths); + + for (Path path : paths) { + assertEquals(genId, format.findMaxGenerationId("foo-", path)); + } + assertEquals(0, format.findStateFilesByGeneration(-1, paths).size()); + assertEquals(paths.length, format.findStateFilesByGeneration(genId, paths).size()); + + Path badPath = paths[paths.length-1]; + Format mockFormat = spy(format); + Directory mockDirectory = spy(mockFormat.newDirectory(badPath)); + + doThrow(new IOException("Disk temporarily inaccessible")).when(mockDirectory).rename(anyString(), anyString()); + doCallRealMethod().doCallRealMethod().doReturn(mockDirectory).when(mockFormat).newDirectory(any()); + + expectThrows(WriteStateException.class, () -> mockFormat.write(state, paths)); + long firstPathId = format.findMaxGenerationId("foo-", paths[0]); + assertEquals(firstPathId, format.findMaxGenerationId("foo-", paths[1])); + assertEquals(firstPathId, format.findMaxGenerationId("foo-", paths[2])); + assertEquals(genId, firstPathId); + } + private static class Format extends MetadataStateFormat { private enum FailureMode { NO_FAILURES, From b1799df930443edef026eb8fe463f3575e8e3625 Mon Sep 17 00:00:00 2001 From: Nikola Grcevski Date: Fri, 1 Oct 2021 19:14:01 -0400 Subject: [PATCH 02/11] Fix style. --- .../org/elasticsearch/gateway/MetadataStateFormatTests.java | 4 ---- 1 file changed, 4 deletions(-) diff --git a/server/src/test/java/org/elasticsearch/gateway/MetadataStateFormatTests.java b/server/src/test/java/org/elasticsearch/gateway/MetadataStateFormatTests.java index 3655dd8b1a43a..f06c9cfd30f60 100644 --- a/server/src/test/java/org/elasticsearch/gateway/MetadataStateFormatTests.java +++ b/server/src/test/java/org/elasticsearch/gateway/MetadataStateFormatTests.java @@ -42,13 +42,9 @@ import static org.hamcrest.Matchers.notNullValue; import static org.mockito.Matchers.any; import static org.mockito.Matchers.anyString; -import static org.mockito.Matchers.eq; import static org.mockito.Mockito.doCallRealMethod; -import static org.mockito.Mockito.doReturn; import static org.mockito.Mockito.doThrow; -import static org.mockito.Mockito.mock; import static org.mockito.Mockito.spy; -import static org.mockito.Mockito.when; @LuceneTestCase.SuppressFileSystems("ExtrasFS") // TODO: fix test to work with ExtrasFS public class MetadataStateFormatTests extends ESTestCase { From 6da40e20da1cc1fe994a4fc24843de8c0a5d0f41 Mon Sep 17 00:00:00 2001 From: Nikola Grcevski Date: Mon, 4 Oct 2021 14:30:44 -0400 Subject: [PATCH 03/11] Improve tests, add delete tests. --- .../gateway/MetadataStateFormat.java | 3 + .../gateway/MetadataStateFormatTests.java | 111 ++++++++++++++---- 2 files changed, 92 insertions(+), 22 deletions(-) diff --git a/server/src/main/java/org/elasticsearch/gateway/MetadataStateFormat.java b/server/src/main/java/org/elasticsearch/gateway/MetadataStateFormat.java index 097e77c2c8ba4..17ba3d48f673f 100644 --- a/server/src/main/java/org/elasticsearch/gateway/MetadataStateFormat.java +++ b/server/src/main/java/org/elasticsearch/gateway/MetadataStateFormat.java @@ -430,6 +430,9 @@ public Tuple loadLatestStateWithGeneration(Logger logger, NamedXContent long generation = findMaxGenerationId(prefix, dataLocations); T state = loadGeneration(logger, namedXContentRegistry, generation, dataLocations); + // It may not be possible to get into this state, if there's a bad state file the above + // call will throw ElasticsearchException. If there are no state files, we won't find a + // generation. if (generation > -1 && state == null) { throw new IllegalStateException("unable to find state files with generation id " + generation + " returned by findMaxGenerationId function, in data folders [" + diff --git a/server/src/test/java/org/elasticsearch/gateway/MetadataStateFormatTests.java b/server/src/test/java/org/elasticsearch/gateway/MetadataStateFormatTests.java index f06c9cfd30f60..b62f927f35482 100644 --- a/server/src/test/java/org/elasticsearch/gateway/MetadataStateFormatTests.java +++ b/server/src/test/java/org/elasticsearch/gateway/MetadataStateFormatTests.java @@ -16,6 +16,7 @@ import org.apache.lucene.store.MockDirectoryWrapper; import org.apache.lucene.store.NIOFSDirectory; import org.apache.lucene.util.LuceneTestCase; +import org.elasticsearch.ElasticsearchException; import org.elasticsearch.cluster.ClusterModule; import org.elasticsearch.cluster.metadata.Metadata; import org.elasticsearch.common.xcontent.NamedXContentRegistry; @@ -34,17 +35,13 @@ import java.nio.file.Path; import java.nio.file.StandardOpenOption; import java.util.HashSet; +import java.util.List; import java.util.Set; import java.util.stream.StreamSupport; import static org.hamcrest.Matchers.equalTo; import static org.hamcrest.Matchers.is; import static org.hamcrest.Matchers.notNullValue; -import static org.mockito.Matchers.any; -import static org.mockito.Matchers.anyString; -import static org.mockito.Mockito.doCallRealMethod; -import static org.mockito.Mockito.doThrow; -import static org.mockito.Mockito.spy; @LuceneTestCase.SuppressFileSystems("ExtrasFS") // TODO: fix test to work with ExtrasFS public class MetadataStateFormatTests extends ESTestCase { @@ -349,17 +346,10 @@ public void testFailRandomlyAndReadAnyState() throws IOException { writeAndReadStateSuccessfully(format, paths); } - private Path makeMockTempDirPathHelper() { - Path realPath = createTempDir(); - Path result = spy(realPath); - - return result; - } - public void testInconsistentMultiPathState() throws IOException { Path paths[] = new Path[3]; for (int i = 0; i < paths.length; i++) { - paths[i] = makeMockTempDirPathHelper(); + paths[i] = createTempDir(); } Format format = new Format("foo-"); @@ -377,19 +367,78 @@ public void testInconsistentMultiPathState() throws IOException { assertEquals(paths.length, format.findStateFilesByGeneration(genId, paths).size()); Path badPath = paths[paths.length-1]; - Format mockFormat = spy(format); - Directory mockDirectory = spy(mockFormat.newDirectory(badPath)); - doThrow(new IOException("Disk temporarily inaccessible")).when(mockDirectory).rename(anyString(), anyString()); - doCallRealMethod().doCallRealMethod().doReturn(mockDirectory).when(mockFormat).newDirectory(any()); + format.failOnPaths(badPath.resolve(MetadataStateFormat.STATE_DIR_NAME)); + format.failOnMethods(Format.FAIL_RENAME_TMP_FILE); - expectThrows(WriteStateException.class, () -> mockFormat.write(state, paths)); + expectThrows(WriteStateException.class, () -> format.write(state, paths)); long firstPathId = format.findMaxGenerationId("foo-", paths[0]); assertEquals(firstPathId, format.findMaxGenerationId("foo-", paths[1])); assertEquals(firstPathId, format.findMaxGenerationId("foo-", paths[2])); assertEquals(genId, firstPathId); } + public void testDeleteMetaState() throws IOException { + Path paths[] = new Path[3]; + for (int i = 0; i < paths.length; i++) { + paths[i] = createTempDir(); + } + Format format = new Format("foo-"); + + DummyState state = new DummyState(randomRealisticUnicodeOfCodepointLengthBetween(1, 100), randomInt(), randomLong(), + randomDouble(), randomBoolean()); + long genId = format.write(state, paths); + assertEquals(state, format.loadLatestState(logger, NamedXContentRegistry.EMPTY, paths)); + ensureOnlyOneStateFile(paths); + + for (Path path : paths) { + assertEquals(genId, format.findMaxGenerationId("foo-", path)); + } + assertEquals(0, format.findStateFilesByGeneration(-1, paths).size()); + assertEquals(paths.length, format.findStateFilesByGeneration(genId, paths).size()); + + Format.deleteMetaState(paths); + + assertEquals(0, format.findStateFilesByGeneration(genId, paths).size()); + + for (Path path : paths) { + assertEquals(false, Files.exists(path.resolve(MetadataStateFormat.STATE_DIR_NAME))); + } + } + + public void testDeleteMetaStateWithErrorPath() throws IOException { + Path paths[] = new Path[3]; + for (int i = 0; i < paths.length; i++) { + paths[i] = createTempDir(); + } + Format format = new Format("foo-"); + + DummyState state = new DummyState(randomRealisticUnicodeOfCodepointLengthBetween(1, 100), randomInt(), randomLong(), + randomDouble(), randomBoolean()); + long genId = format.write(state, paths); + assertEquals(state, format.loadLatestState(logger, NamedXContentRegistry.EMPTY, paths)); + ensureOnlyOneStateFile(paths); + + for (Path path : paths) { + assertEquals(genId, format.findMaxGenerationId("foo-", path)); + } + assertEquals(0, format.findStateFilesByGeneration(-1, paths).size()); + assertEquals(paths.length, format.findStateFilesByGeneration(genId, paths).size()); + + List stateFiles = format.findStateFilesByGeneration(genId, paths); + + format.failOnPaths(paths[1].resolve(MetadataStateFormat.STATE_DIR_NAME)); + format.failOnMethods(Format.FAIL_DELETE_TMP_FILE); + + // Ensure clean-up old files doesn't fail with one bad dir + format.cleanupOldFiles(1, paths); + + corruptFile(stateFiles.get(1), logger); + + // Ensure we find the corrupted metadata without the leader first state path + expectThrows(ElasticsearchException.class, () -> format.loadLatestStateWithGeneration(logger, xContentRegistry(), paths)); + } + private static class Format extends MetadataStateFormat { private enum FailureMode { NO_FAILURES, @@ -399,6 +448,7 @@ private enum FailureMode { private FailureMode failureMode; private String[] failureMethods; + private Path[] failurePaths; static final String FAIL_CREATE_OUTPUT_FILE = "createOutput"; static final String FAIL_WRITE_TO_OUTPUT_FILE = "writeBytes"; @@ -407,6 +457,7 @@ private enum FailureMode { static final String FAIL_FSYNC_STATE_DIRECTORY = "syncMetaData"; static final String FAIL_DELETE_TMP_FILE = "deleteFile"; static final String FAIL_OPEN_STATE_FILE_WHEN_COPYING = "openInput"; + static final String FAIL_LIST_ALL = "listAll"; /** * Constructs a MetadataStateFormat object for storing/retrieving DummyState. @@ -438,10 +489,26 @@ public void failOnMethods(String... failureMethods) { this.failureMethods = failureMethods; } + public void failOnPaths(Path... paths) { + this.failurePaths = paths; + } + public void failRandomly() { this.failureMode = FailureMode.FAIL_RANDOMLY; } + private void throwDirectoryExceptionCheckPaths(Path dir) throws MockDirectoryWrapper.FakeIOException { + if (failurePaths != null && failurePaths.length > 0) { + for (Path p : failurePaths) { + if (p.equals(dir)) { + throw new MockDirectoryWrapper.FakeIOException(); + } + } + } else { + throw new MockDirectoryWrapper.FakeIOException(); + } + } + @Override protected Directory newDirectory(Path dir) { MockDirectoryWrapper mock = newMockFSDirectory(dir); @@ -449,10 +516,10 @@ protected Directory newDirectory(Path dir) { final String failMethod = randomFrom(failureMethods); MockDirectoryWrapper.Failure fail = new MockDirectoryWrapper.Failure() { @Override - public void eval(MockDirectoryWrapper dir) throws IOException { + public void eval(MockDirectoryWrapper directory) throws IOException { for (StackTraceElement e : Thread.currentThread().getStackTrace()) { if (failMethod.equals(e.getMethodName())) { - throw new MockDirectoryWrapper.FakeIOException(); + throwDirectoryExceptionCheckPaths(dir); } } } @@ -461,9 +528,9 @@ public void eval(MockDirectoryWrapper dir) throws IOException { } else if (failureMode == FailureMode.FAIL_RANDOMLY) { MockDirectoryWrapper.Failure fail = new MockDirectoryWrapper.Failure() { @Override - public void eval(MockDirectoryWrapper dir) throws IOException { + public void eval(MockDirectoryWrapper directory) throws IOException { if (randomIntBetween(0, 20) == 0) { - throw new MockDirectoryWrapper.FakeIOException(); + throwDirectoryExceptionCheckPaths(dir); } } }; From 1a1e801637bd6c72902cdc10b219898636c3b1a8 Mon Sep 17 00:00:00 2001 From: Nikola Grcevski Date: Wed, 6 Oct 2021 13:25:08 -0400 Subject: [PATCH 04/11] Update test based on expected behaviour. --- .../org/elasticsearch/gateway/MetadataStateFormatTests.java | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/server/src/test/java/org/elasticsearch/gateway/MetadataStateFormatTests.java b/server/src/test/java/org/elasticsearch/gateway/MetadataStateFormatTests.java index b62f927f35482..bbaff5b1b235d 100644 --- a/server/src/test/java/org/elasticsearch/gateway/MetadataStateFormatTests.java +++ b/server/src/test/java/org/elasticsearch/gateway/MetadataStateFormatTests.java @@ -374,8 +374,8 @@ public void testInconsistentMultiPathState() throws IOException { expectThrows(WriteStateException.class, () -> format.write(state, paths)); long firstPathId = format.findMaxGenerationId("foo-", paths[0]); assertEquals(firstPathId, format.findMaxGenerationId("foo-", paths[1])); - assertEquals(firstPathId, format.findMaxGenerationId("foo-", paths[2])); - assertEquals(genId, firstPathId); + assertEquals(genId, format.findMaxGenerationId("foo-", badPath)); + assertEquals(genId, firstPathId-1); } public void testDeleteMetaState() throws IOException { From 84ccc3bd8b9dec73f991323dc17da4141c33107b Mon Sep 17 00:00:00 2001 From: Nikola Grcevski Date: Thu, 7 Oct 2021 09:58:40 -0400 Subject: [PATCH 05/11] Implement PR suggestions. --- .../gateway/MetadataStateFormat.java | 4 ++-- .../gateway/MetadataStateFormatTests.java | 19 +++++++++++++++++-- 2 files changed, 19 insertions(+), 4 deletions(-) diff --git a/server/src/main/java/org/elasticsearch/gateway/MetadataStateFormat.java b/server/src/main/java/org/elasticsearch/gateway/MetadataStateFormat.java index 17ba3d48f673f..d17a7c02924fe 100644 --- a/server/src/main/java/org/elasticsearch/gateway/MetadataStateFormat.java +++ b/server/src/main/java/org/elasticsearch/gateway/MetadataStateFormat.java @@ -343,7 +343,7 @@ public void cleanupOldFiles(final long currentGeneration, Path... locations) { * @return maximum id of state file or -1 if no such files are found * @throws IOException if IOException occurs */ - protected long findMaxGenerationId(final String prefix, Path... locations) throws IOException { + long findMaxGenerationId(final String prefix, Path... locations) throws IOException { long maxId = -1; for (Path dataLocation : locations) { final Path resolve = dataLocation.resolve(STATE_DIR_NAME); @@ -362,7 +362,7 @@ protected long findMaxGenerationId(final String prefix, Path... locations) throw return maxId; } - protected List findStateFilesByGeneration(final long generation, Path... locations) { + List findStateFilesByGeneration(final long generation, Path... locations) { List files = new ArrayList<>(); if (generation == -1) { return files; diff --git a/server/src/test/java/org/elasticsearch/gateway/MetadataStateFormatTests.java b/server/src/test/java/org/elasticsearch/gateway/MetadataStateFormatTests.java index bbaff5b1b235d..80cd3bc4294b0 100644 --- a/server/src/test/java/org/elasticsearch/gateway/MetadataStateFormatTests.java +++ b/server/src/test/java/org/elasticsearch/gateway/MetadataStateFormatTests.java @@ -371,11 +371,22 @@ public void testInconsistentMultiPathState() throws IOException { format.failOnPaths(badPath.resolve(MetadataStateFormat.STATE_DIR_NAME)); format.failOnMethods(Format.FAIL_RENAME_TMP_FILE); - expectThrows(WriteStateException.class, () -> format.write(state, paths)); + DummyState newState = new DummyState(randomRealisticUnicodeOfCodepointLengthBetween(1, 100), randomInt(), randomLong(), + randomDouble(), randomBoolean()); + + expectThrows(WriteStateException.class, () -> format.write(newState, paths)); long firstPathId = format.findMaxGenerationId("foo-", paths[0]); assertEquals(firstPathId, format.findMaxGenerationId("foo-", paths[1])); assertEquals(genId, format.findMaxGenerationId("foo-", badPath)); assertEquals(genId, firstPathId-1); + + // Since at least one path has the latest generation, we should find the latest + // generation when we supply all paths. + long allPathsId = format.findMaxGenerationId("foo-", paths); + assertEquals(firstPathId, allPathsId); + + // Assert that we can find the new state since one path successfully wrote it. + assertEquals(newState, format.loadLatestState(logger, NamedXContentRegistry.EMPTY, paths)); } public void testDeleteMetaState() throws IOException { @@ -404,6 +415,10 @@ public void testDeleteMetaState() throws IOException { for (Path path : paths) { assertEquals(false, Files.exists(path.resolve(MetadataStateFormat.STATE_DIR_NAME))); } + + // We shouldn't find any state or state generations anymore + assertEquals(-1, format.findMaxGenerationId("foo-", paths)); + assertNull(format.loadLatestState(logger, NamedXContentRegistry.EMPTY, paths)); } public void testDeleteMetaStateWithErrorPath() throws IOException { @@ -498,7 +513,7 @@ public void failRandomly() { } private void throwDirectoryExceptionCheckPaths(Path dir) throws MockDirectoryWrapper.FakeIOException { - if (failurePaths != null && failurePaths.length > 0) { + if (failurePaths != null) { for (Path p : failurePaths) { if (p.equals(dir)) { throw new MockDirectoryWrapper.FakeIOException(); From 1a8e8b10580980bc5811ccdae2665ae1f740160f Mon Sep 17 00:00:00 2001 From: Nikola Grcevski <6207777+grcevski@users.noreply.github.com> Date: Thu, 7 Oct 2021 09:58:54 -0400 Subject: [PATCH 06/11] Update server/src/test/java/org/elasticsearch/gateway/MetadataStateFormatTests.java Co-authored-by: Henning Andersen <33268011+henningandersen@users.noreply.github.com> --- .../org/elasticsearch/gateway/MetadataStateFormatTests.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/server/src/test/java/org/elasticsearch/gateway/MetadataStateFormatTests.java b/server/src/test/java/org/elasticsearch/gateway/MetadataStateFormatTests.java index bbaff5b1b235d..1ca1494ebbf5e 100644 --- a/server/src/test/java/org/elasticsearch/gateway/MetadataStateFormatTests.java +++ b/server/src/test/java/org/elasticsearch/gateway/MetadataStateFormatTests.java @@ -406,7 +406,7 @@ public void testDeleteMetaState() throws IOException { } } - public void testDeleteMetaStateWithErrorPath() throws IOException { + public void testCleanupOldFilesWithErrorPath() throws IOException { Path paths[] = new Path[3]; for (int i = 0; i < paths.length; i++) { paths[i] = createTempDir(); From a3e1bb75280793f3f14faccaa7fe23321f70ef9d Mon Sep 17 00:00:00 2001 From: Nikola Grcevski <6207777+grcevski@users.noreply.github.com> Date: Thu, 7 Oct 2021 09:59:20 -0400 Subject: [PATCH 07/11] Update server/src/test/java/org/elasticsearch/gateway/MetadataStateFormatTests.java Co-authored-by: Henning Andersen <33268011+henningandersen@users.noreply.github.com> --- .../org/elasticsearch/gateway/MetadataStateFormatTests.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/server/src/test/java/org/elasticsearch/gateway/MetadataStateFormatTests.java b/server/src/test/java/org/elasticsearch/gateway/MetadataStateFormatTests.java index 1ca1494ebbf5e..94fa4adc9440f 100644 --- a/server/src/test/java/org/elasticsearch/gateway/MetadataStateFormatTests.java +++ b/server/src/test/java/org/elasticsearch/gateway/MetadataStateFormatTests.java @@ -431,7 +431,7 @@ public void testCleanupOldFilesWithErrorPath() throws IOException { format.failOnMethods(Format.FAIL_DELETE_TMP_FILE); // Ensure clean-up old files doesn't fail with one bad dir - format.cleanupOldFiles(1, paths); + format.cleanupOldFiles(genId + 1, paths); corruptFile(stateFiles.get(1), logger); From b3eeb4ecda3e4c34c272888fd4231add6377cb1e Mon Sep 17 00:00:00 2001 From: Nikola Grcevski Date: Thu, 7 Oct 2021 11:23:30 -0400 Subject: [PATCH 08/11] More PR suggestions implementation. --- .../gateway/MetadataStateFormatTests.java | 21 ++++++++++++++----- 1 file changed, 16 insertions(+), 5 deletions(-) diff --git a/server/src/test/java/org/elasticsearch/gateway/MetadataStateFormatTests.java b/server/src/test/java/org/elasticsearch/gateway/MetadataStateFormatTests.java index 94b35d56dec31..22b411b70486b 100644 --- a/server/src/test/java/org/elasticsearch/gateway/MetadataStateFormatTests.java +++ b/server/src/test/java/org/elasticsearch/gateway/MetadataStateFormatTests.java @@ -39,6 +39,7 @@ import java.util.Set; import java.util.stream.StreamSupport; +import static org.hamcrest.Matchers.containsString; import static org.hamcrest.Matchers.equalTo; import static org.hamcrest.Matchers.is; import static org.hamcrest.Matchers.notNullValue; @@ -442,16 +443,26 @@ public void testCleanupOldFilesWithErrorPath() throws IOException { List stateFiles = format.findStateFilesByGeneration(genId, paths); - format.failOnPaths(paths[1].resolve(MetadataStateFormat.STATE_DIR_NAME)); + final int badDirIndex = 1; + + format.failOnPaths(paths[badDirIndex].resolve(MetadataStateFormat.STATE_DIR_NAME)); format.failOnMethods(Format.FAIL_DELETE_TMP_FILE); - // Ensure clean-up old files doesn't fail with one bad dir + // Ensure clean-up old files doesn't fail with one bad dir. We pretend we want to + // keep a newer generation that doesn't exist (genId + 1). format.cleanupOldFiles(genId + 1, paths); - corruptFile(stateFiles.get(1), logger); + // We simulated failure on deleting one stale state file, there should be one that's remaining from the old state. + // We'll corrupt this remaining file and check to see if loading the state throws an exception. + // All other state files, including the first directory uncorrupted state files should be cleaned up. + corruptFile(stateFiles.get(badDirIndex), logger); - // Ensure we find the corrupted metadata without the leader first state path - expectThrows(ElasticsearchException.class, () -> format.loadLatestStateWithGeneration(logger, xContentRegistry(), paths)); + try { + format.loadLatestStateWithGeneration(logger, xContentRegistry(), paths); + fail("Reading corrupted state should fail"); + } catch (ElasticsearchException esException) { + assertThat(esException.getMessage(), is("java.io.IOException: failed to read " + stateFiles.get(badDirIndex))); + } } private static class Format extends MetadataStateFormat { From 9467926f2f29cf6060b3b54f5a4bbbde8690a787 Mon Sep 17 00:00:00 2001 From: Nikola Grcevski Date: Thu, 7 Oct 2021 11:27:59 -0400 Subject: [PATCH 09/11] Fix import. --- .../java/org/elasticsearch/gateway/MetadataStateFormatTests.java | 1 - 1 file changed, 1 deletion(-) diff --git a/server/src/test/java/org/elasticsearch/gateway/MetadataStateFormatTests.java b/server/src/test/java/org/elasticsearch/gateway/MetadataStateFormatTests.java index 22b411b70486b..6fca04d6aebe4 100644 --- a/server/src/test/java/org/elasticsearch/gateway/MetadataStateFormatTests.java +++ b/server/src/test/java/org/elasticsearch/gateway/MetadataStateFormatTests.java @@ -39,7 +39,6 @@ import java.util.Set; import java.util.stream.StreamSupport; -import static org.hamcrest.Matchers.containsString; import static org.hamcrest.Matchers.equalTo; import static org.hamcrest.Matchers.is; import static org.hamcrest.Matchers.notNullValue; From 2b37ee510f6fa14f36fdcaa7fc5ecda9ed9915d2 Mon Sep 17 00:00:00 2001 From: Nikola Grcevski <6207777+grcevski@users.noreply.github.com> Date: Thu, 7 Oct 2021 12:51:02 -0400 Subject: [PATCH 10/11] Update server/src/test/java/org/elasticsearch/gateway/MetadataStateFormatTests.java Co-authored-by: Henning Andersen <33268011+henningandersen@users.noreply.github.com> --- .../elasticsearch/gateway/MetadataStateFormatTests.java | 7 +------ 1 file changed, 1 insertion(+), 6 deletions(-) diff --git a/server/src/test/java/org/elasticsearch/gateway/MetadataStateFormatTests.java b/server/src/test/java/org/elasticsearch/gateway/MetadataStateFormatTests.java index 6fca04d6aebe4..9b3a107a27a1e 100644 --- a/server/src/test/java/org/elasticsearch/gateway/MetadataStateFormatTests.java +++ b/server/src/test/java/org/elasticsearch/gateway/MetadataStateFormatTests.java @@ -456,12 +456,7 @@ public void testCleanupOldFilesWithErrorPath() throws IOException { // All other state files, including the first directory uncorrupted state files should be cleaned up. corruptFile(stateFiles.get(badDirIndex), logger); - try { - format.loadLatestStateWithGeneration(logger, xContentRegistry(), paths); - fail("Reading corrupted state should fail"); - } catch (ElasticsearchException esException) { - assertThat(esException.getMessage(), is("java.io.IOException: failed to read " + stateFiles.get(badDirIndex))); - } + assertThat(expectThrows(ElasticsearchException.class, () -> format.loadLatestStateWithGeneration(logger, xContentRegistry(), paths)).getMessage(), equalTo("java.io.IOException: failed to read " + stateFiles.get(badDirIndex))); } private static class Format extends MetadataStateFormat { From 0b4b055e222891398a13d8cd677c32e6e364232a Mon Sep 17 00:00:00 2001 From: Nikola Grcevski Date: Thu, 7 Oct 2021 12:52:19 -0400 Subject: [PATCH 11/11] Fix line width. --- .../org/elasticsearch/gateway/MetadataStateFormatTests.java | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/server/src/test/java/org/elasticsearch/gateway/MetadataStateFormatTests.java b/server/src/test/java/org/elasticsearch/gateway/MetadataStateFormatTests.java index 9b3a107a27a1e..c40b4c4b8b3dc 100644 --- a/server/src/test/java/org/elasticsearch/gateway/MetadataStateFormatTests.java +++ b/server/src/test/java/org/elasticsearch/gateway/MetadataStateFormatTests.java @@ -456,7 +456,9 @@ public void testCleanupOldFilesWithErrorPath() throws IOException { // All other state files, including the first directory uncorrupted state files should be cleaned up. corruptFile(stateFiles.get(badDirIndex), logger); - assertThat(expectThrows(ElasticsearchException.class, () -> format.loadLatestStateWithGeneration(logger, xContentRegistry(), paths)).getMessage(), equalTo("java.io.IOException: failed to read " + stateFiles.get(badDirIndex))); + assertThat(expectThrows(ElasticsearchException.class, + () -> format.loadLatestStateWithGeneration(logger, xContentRegistry(), paths)).getMessage(), + equalTo("java.io.IOException: failed to read " + stateFiles.get(badDirIndex))); } private static class Format extends MetadataStateFormat {