Apply feedback

elastic · Apr 27, 2018 · 46d4748 · 46d4748
1 parent ff72347
commit 46d4748
Show file tree

Hide file tree

Showing 4 changed files with 57 additions and 19 deletions.
diff --git a/docs/CHANGELOG.asciidoc b/docs/CHANGELOG.asciidoc
@@ -24,7 +24,8 @@
 
 === Bug Fixes
 
-Fix NullPointerException when creating or deleting a snapshot ({pull}30140[#30140])
+Fix NullPointerException when creating or deleting a snapshot on a repository that has been
+written to by an older Elasticsearch after writing to it with a newer Elasticsearch version. ({pull}30140[#30140])
 
 === Regressions
 

diff --git a/docs/reference/modules/snapshots.asciidoc b/docs/reference/modules/snapshots.asciidoc
@@ -45,11 +45,11 @@ one cluster should have write access to the repository. All other clusters
 connected to that repository should set the repository to `readonly` mode.
 
 IMPORTANT: The snapshot format can change across major versions, so if you have
-clusters on different major versions trying to write the same repository,
-snapshots written by one version may not be visible to the other. While
-setting the repository to `readonly` on all but one of the clusters should work
-with multiple clusters differing by one major version, it is not a supported
-configuration.
+clusters on different versions trying to write the same repository, snapshots
+written by one version may not be visible to the other and the repository could
+be corrupted. While setting the repository to `readonly` on all but one of the
+clusters should work with multiple clusters differing by one major version, it
+is not a supported configuration.
 
 [source,js]
 -----------------------------------

diff --git a/server/src/main/java/org/elasticsearch/repositories/RepositoryData.java b/server/src/main/java/org/elasticsearch/repositories/RepositoryData.java
@@ -423,17 +423,16 @@ public static RepositoryData snapshotsFromXContent(final XContentParser parser,
                                         // since we already have the name/uuid combo in the snapshots array
                                         uuid = parser.text();
                                     }
-                                    if (uuid != null) {
-                                        SnapshotId snapshotId = snapshots.get(uuid);
-                                        if (snapshotId != null) {
-                                            snapshotIds.add(snapshotId);
-                                        } else {
-                                            // A snapshotted index references a snapshot which does not exist in
-                                            // the list of snapshots. This can happen when multiple clusters in
-                                            // different versions create or delete snapshot in the same repository.
-                                            throw new ElasticsearchParseException("Unknown snapshot uuid [" + uuid
-                                                + "] for index " + indexId);
-                                        }
+
+                                    SnapshotId snapshotId = snapshots.get(uuid);
+                                    if (snapshotId != null) {
+                                        snapshotIds.add(snapshotId);
+                                    } else {
+                                        // A snapshotted index references a snapshot which does not exist in
+                                        // the list of snapshots. This can happen when multiple clusters in
+                                        // different versions create or delete snapshot in the same repository.
+                                        throw new ElasticsearchParseException("Detected a corrupted repository, index " + indexId
+                                            + " references an unknown snapshot uuid [" + uuid + "]");
                                     }
                                 }
                             }

diff --git a/server/src/test/java/org/elasticsearch/repositories/RepositoryDataTests.java b/server/src/test/java/org/elasticsearch/repositories/RepositoryDataTests.java
@@ -42,7 +42,10 @@
 import java.util.Map;
 import java.util.Set;
 
+import static java.util.Collections.emptySet;
+import static java.util.Collections.singleton;
 import static org.elasticsearch.repositories.RepositoryData.EMPTY_REPO_GEN;
+import static org.hamcrest.Matchers.empty;
 import static org.hamcrest.Matchers.equalTo;
 import static org.hamcrest.Matchers.greaterThan;
 
@@ -160,7 +163,7 @@ public void testGetSnapshotState() {
         assertNull(repositoryData.getSnapshotState(new SnapshotId(randomAlphaOfLength(8), UUIDs.randomBase64UUID())));
     }
 
-    public void testUnknownSnapshot() throws IOException {
+    public void testIndexThatReferencesAnUnknownSnapshot() throws IOException {
         final XContent xContent = randomFrom(XContentType.values()).xContent();
         final RepositoryData repositoryData = generateRandomRepoData();
 
@@ -197,7 +200,42 @@ public void testUnknownSnapshot() throws IOException {
 
         ElasticsearchParseException e = expectThrows(ElasticsearchParseException.class, () ->
             RepositoryData.snapshotsFromXContent(createParser(corruptedBuilder), corruptedRepositoryData.getGenId()));
-        assertThat(e.getMessage(), equalTo("Unknown snapshot uuid [_does_not_exist] for index " + corruptedIndexId));
+        assertThat(e.getMessage(), equalTo("Detected a corrupted repository, index " + corruptedIndexId + " references an unknown " +
+            "snapshot uuid [_does_not_exist]"));
+    }
+
+    public void testIndexThatReferenceANullSnapshot() throws IOException {
+        final XContentBuilder builder = XContentBuilder.builder(randomFrom(XContentType.JSON).xContent());
+        builder.startObject();
+        {
+            builder.startArray("snapshots");
+            builder.value(new SnapshotId("_name", "_uuid"));
+            builder.endArray();
+
+            builder.startObject("indices");
+            {
+                builder.startObject("docs");
+                {
+                    builder.field("id", "_id");
+                    builder.startArray("snapshots");
+                    {
+                        builder.startObject();
+                        if (randomBoolean()) {
+                            builder.field("name", "_name");
+                        }
+                        builder.endObject();
+                    }
+                    builder.endArray();
+                }
+                builder.endObject();
+            }
+            builder.endObject();
+        }
+        builder.endObject();
+
+        ElasticsearchParseException e = expectThrows(ElasticsearchParseException.class, () ->
+            RepositoryData.snapshotsFromXContent(createParser(builder), randomNonNegativeLong()));
+        assertThat(e.getMessage(), equalTo("Detected a corrupted repository, index [docs/_id] references an unknown snapshot uuid [null]"));
     }
 
     public static RepositoryData generateRandomRepoData() {