From fa73c571b94a24c81f13754328f074b92577e441 Mon Sep 17 00:00:00 2001 From: Michael Basnight Date: Fri, 25 May 2018 11:11:13 -0500 Subject: [PATCH 1/3] Remove version read/write logic in Verify Response Since master will always communicate with a >=6.4 node, the logic for checking if the node is 6.4 and conditionally reading and writing based on that can be removed from master. This logic will stay in 6.x as it is the bridge to the cleaner response in master. This also unmutes the failing test due to this bwc change. Closes #30807 --- .../test/snapshot.get_repository/10_basic.yml | 3 - .../TransportVerifyRepositoryAction.java | 2 +- .../verify/VerifyRepositoryResponse.java | 58 +++---------------- .../repositories/RepositoryBlocksIT.java | 2 +- .../cluster/snapshots/SnapshotBlocksIT.java | 2 +- .../snapshots/RepositoriesIT.java | 3 +- 6 files changed, 13 insertions(+), 57 deletions(-) diff --git a/rest-api-spec/src/main/resources/rest-api-spec/test/snapshot.get_repository/10_basic.yml b/rest-api-spec/src/main/resources/rest-api-spec/test/snapshot.get_repository/10_basic.yml index c70942f0c9d91..b944fe43791e4 100644 --- a/rest-api-spec/src/main/resources/rest-api-spec/test/snapshot.get_repository/10_basic.yml +++ b/rest-api-spec/src/main/resources/rest-api-spec/test/snapshot.get_repository/10_basic.yml @@ -51,9 +51,6 @@ setup: --- "Verify created repository": - - skip: - version: "all" - reason: AwaitsFix for https://github.com/elastic/elasticsearch/issues/30807 - do: snapshot.verify_repository: repository: test_repo_get_2 diff --git a/server/src/main/java/org/elasticsearch/action/admin/cluster/repositories/verify/TransportVerifyRepositoryAction.java b/server/src/main/java/org/elasticsearch/action/admin/cluster/repositories/verify/TransportVerifyRepositoryAction.java index 3bd2bef12748c..4614085f26e2b 100644 --- a/server/src/main/java/org/elasticsearch/action/admin/cluster/repositories/verify/TransportVerifyRepositoryAction.java +++ b/server/src/main/java/org/elasticsearch/action/admin/cluster/repositories/verify/TransportVerifyRepositoryAction.java @@ -73,7 +73,7 @@ public void onResponse(RepositoriesService.VerifyResponse verifyResponse) { if (verifyResponse.failed()) { listener.onFailure(new RepositoryVerificationException(request.name(), verifyResponse.failureDescription())); } else { - listener.onResponse(new VerifyRepositoryResponse(clusterService.getClusterName(), verifyResponse.nodes())); + listener.onResponse(new VerifyRepositoryResponse(verifyResponse.nodes())); } } diff --git a/server/src/main/java/org/elasticsearch/action/admin/cluster/repositories/verify/VerifyRepositoryResponse.java b/server/src/main/java/org/elasticsearch/action/admin/cluster/repositories/verify/VerifyRepositoryResponse.java index d5cc0b6f95728..b4ee772b145b1 100644 --- a/server/src/main/java/org/elasticsearch/action/admin/cluster/repositories/verify/VerifyRepositoryResponse.java +++ b/server/src/main/java/org/elasticsearch/action/admin/cluster/repositories/verify/VerifyRepositoryResponse.java @@ -19,23 +19,19 @@ package org.elasticsearch.action.admin.cluster.repositories.verify; -import org.elasticsearch.Version; import org.elasticsearch.action.ActionResponse; -import org.elasticsearch.cluster.ClusterName; import org.elasticsearch.cluster.node.DiscoveryNode; import org.elasticsearch.common.ParseField; import org.elasticsearch.common.Strings; import org.elasticsearch.common.io.stream.StreamInput; import org.elasticsearch.common.io.stream.StreamOutput; import org.elasticsearch.common.io.stream.Writeable; -import org.elasticsearch.common.transport.TransportAddress; import org.elasticsearch.common.xcontent.ObjectParser; import org.elasticsearch.common.xcontent.ToXContentObject; import org.elasticsearch.common.xcontent.XContentBuilder; import java.io.IOException; import java.util.Arrays; -import java.util.Collections; import java.util.List; import java.util.Objects; import java.util.stream.Collectors; @@ -91,20 +87,6 @@ public XContentBuilder toXContent(XContentBuilder builder, Params params) throws return builder; } - /** - * Temporary method that allows turning a {@link NodeView} into a {@link DiscoveryNode}. This representation will never be used in - * practice, because in >= 6.4 a consumer of the response will only be able to retrieve a representation of {@link NodeView} - * objects. - * - * Effectively this will be used to hold the state of the object in 6.x so there is no need to have 2 backing objects that - * represent the state of the Response. In practice these will always be read by a consumer as a NodeView, but it eases the - * transition to master which will not contain any representation of a {@link DiscoveryNode}. - */ - DiscoveryNode convertToDiscoveryNode() { - return new DiscoveryNode(name, nodeId, "", "", "", new TransportAddress(TransportAddress.META_ADDRESS, 0), - Collections.emptyMap(), Collections.emptySet(), Version.CURRENT); - } - @Override public boolean equals(Object obj) { if (obj == null) { @@ -124,47 +106,29 @@ public int hashCode() { } } - private List nodes; - - private ClusterName clusterName; - + private List nodes; VerifyRepositoryResponse() { } - public VerifyRepositoryResponse(ClusterName clusterName, DiscoveryNode[] nodes) { - this.clusterName = clusterName; - this.nodes = Arrays.asList(nodes); + public VerifyRepositoryResponse(DiscoveryNode[] nodes) { + this.nodes = Arrays.stream(nodes).map(dn -> new NodeView(dn.getId(), dn.getName())).collect(Collectors.toList()); } @Override public void readFrom(StreamInput in) throws IOException { super.readFrom(in); - if (in.getVersion().onOrAfter(Version.V_6_4_0)) { - this.nodes = in.readList(NodeView::new).stream().map(n -> n.convertToDiscoveryNode()).collect(Collectors.toList()); - } else { - clusterName = new ClusterName(in); - this.nodes = in.readList(DiscoveryNode::new); - } + this.nodes = in.readList(NodeView::new); } @Override public void writeTo(StreamOutput out) throws IOException { super.writeTo(out); - if (out.getVersion().onOrAfter(Version.V_6_4_0)) { - out.writeList(getNodes()); - } else { - clusterName.writeTo(out); - out.writeList(nodes); - } - } - - public List getNodes() { - return nodes.stream().map(dn -> new NodeView(dn.getId(), dn.getName())).collect(Collectors.toList()); + out.writeList(nodes()); } - public ClusterName getClusterName() { - return clusterName; + public List nodes() { + return nodes; } @Override @@ -173,12 +137,8 @@ public XContentBuilder toXContent(XContentBuilder builder, Params params) throws { builder.startObject(NODES); { - for (DiscoveryNode node : nodes) { - builder.startObject(node.getId()); - { - builder.field(NAME, node.getName()); - } - builder.endObject(); + for (NodeView node : nodes) { + node.toXContent(builder, params); } } builder.endObject(); diff --git a/server/src/test/java/org/elasticsearch/action/admin/cluster/repositories/RepositoryBlocksIT.java b/server/src/test/java/org/elasticsearch/action/admin/cluster/repositories/RepositoryBlocksIT.java index 43d94f56e5af3..f7c34fecbfc02 100644 --- a/server/src/test/java/org/elasticsearch/action/admin/cluster/repositories/RepositoryBlocksIT.java +++ b/server/src/test/java/org/elasticsearch/action/admin/cluster/repositories/RepositoryBlocksIT.java @@ -67,7 +67,7 @@ public void testVerifyRepositoryWithBlocks() { try { setClusterReadOnly(true); VerifyRepositoryResponse response = client().admin().cluster().prepareVerifyRepository("test-repo-blocks").execute().actionGet(); - assertThat(response.getNodes().size(), equalTo(cluster().numDataAndMasterNodes())); + assertThat(response.nodes().size(), equalTo(cluster().numDataAndMasterNodes())); } finally { setClusterReadOnly(false); } diff --git a/server/src/test/java/org/elasticsearch/action/admin/cluster/snapshots/SnapshotBlocksIT.java b/server/src/test/java/org/elasticsearch/action/admin/cluster/snapshots/SnapshotBlocksIT.java index 5ca7cb1e5066d..30ae5fbe34b79 100644 --- a/server/src/test/java/org/elasticsearch/action/admin/cluster/snapshots/SnapshotBlocksIT.java +++ b/server/src/test/java/org/elasticsearch/action/admin/cluster/snapshots/SnapshotBlocksIT.java @@ -73,7 +73,7 @@ protected void setUpRepository() throws Exception { logger.info("--> verify the repository"); VerifyRepositoryResponse verifyResponse = client().admin().cluster().prepareVerifyRepository(REPOSITORY_NAME).get(); - assertThat(verifyResponse.getNodes().size(), equalTo(cluster().numDataAndMasterNodes())); + assertThat(verifyResponse.nodes().size(), equalTo(cluster().numDataAndMasterNodes())); logger.info("--> create a snapshot"); CreateSnapshotResponse snapshotResponse = client().admin().cluster().prepareCreateSnapshot(REPOSITORY_NAME, SNAPSHOT_NAME) diff --git a/server/src/test/java/org/elasticsearch/snapshots/RepositoriesIT.java b/server/src/test/java/org/elasticsearch/snapshots/RepositoriesIT.java index 23cb579bfdc92..317f5a5296c7c 100644 --- a/server/src/test/java/org/elasticsearch/snapshots/RepositoriesIT.java +++ b/server/src/test/java/org/elasticsearch/snapshots/RepositoriesIT.java @@ -40,7 +40,6 @@ import static org.elasticsearch.test.hamcrest.ElasticsearchAssertions.assertAcked; import static org.elasticsearch.test.hamcrest.ElasticsearchAssertions.assertThrows; import static org.hamcrest.Matchers.containsString; -import static org.hamcrest.Matchers.either; import static org.hamcrest.Matchers.equalTo; import static org.hamcrest.Matchers.notNullValue; @@ -61,7 +60,7 @@ public void testRepositoryCreation() throws Exception { logger.info("--> verify the repository"); int numberOfFiles = FileSystemUtils.files(location).length; VerifyRepositoryResponse verifyRepositoryResponse = client.admin().cluster().prepareVerifyRepository("test-repo-1").get(); - assertThat(verifyRepositoryResponse.getNodes().size(), equalTo(cluster().numDataAndMasterNodes())); + assertThat(verifyRepositoryResponse.nodes().size(), equalTo(cluster().numDataAndMasterNodes())); logger.info("--> verify that we didn't leave any files as a result of verification"); assertThat(FileSystemUtils.files(location).length, equalTo(numberOfFiles)); From 9eca4376891082782c355a8fb9458983eb7e0b3f Mon Sep 17 00:00:00 2001 From: Michael Basnight Date: Fri, 25 May 2018 16:09:30 -0500 Subject: [PATCH 2/3] Revert the removal of get in getNodes --- .../cluster/repositories/verify/VerifyRepositoryResponse.java | 4 ++-- .../action/admin/cluster/repositories/RepositoryBlocksIT.java | 2 +- .../action/admin/cluster/snapshots/SnapshotBlocksIT.java | 2 +- .../test/java/org/elasticsearch/snapshots/RepositoriesIT.java | 2 +- 4 files changed, 5 insertions(+), 5 deletions(-) diff --git a/server/src/main/java/org/elasticsearch/action/admin/cluster/repositories/verify/VerifyRepositoryResponse.java b/server/src/main/java/org/elasticsearch/action/admin/cluster/repositories/verify/VerifyRepositoryResponse.java index b4ee772b145b1..4a1e75791b67d 100644 --- a/server/src/main/java/org/elasticsearch/action/admin/cluster/repositories/verify/VerifyRepositoryResponse.java +++ b/server/src/main/java/org/elasticsearch/action/admin/cluster/repositories/verify/VerifyRepositoryResponse.java @@ -124,10 +124,10 @@ public void readFrom(StreamInput in) throws IOException { @Override public void writeTo(StreamOutput out) throws IOException { super.writeTo(out); - out.writeList(nodes()); + out.writeList(nodes); } - public List nodes() { + public List getNodes() { return nodes; } diff --git a/server/src/test/java/org/elasticsearch/action/admin/cluster/repositories/RepositoryBlocksIT.java b/server/src/test/java/org/elasticsearch/action/admin/cluster/repositories/RepositoryBlocksIT.java index f7c34fecbfc02..43d94f56e5af3 100644 --- a/server/src/test/java/org/elasticsearch/action/admin/cluster/repositories/RepositoryBlocksIT.java +++ b/server/src/test/java/org/elasticsearch/action/admin/cluster/repositories/RepositoryBlocksIT.java @@ -67,7 +67,7 @@ public void testVerifyRepositoryWithBlocks() { try { setClusterReadOnly(true); VerifyRepositoryResponse response = client().admin().cluster().prepareVerifyRepository("test-repo-blocks").execute().actionGet(); - assertThat(response.nodes().size(), equalTo(cluster().numDataAndMasterNodes())); + assertThat(response.getNodes().size(), equalTo(cluster().numDataAndMasterNodes())); } finally { setClusterReadOnly(false); } diff --git a/server/src/test/java/org/elasticsearch/action/admin/cluster/snapshots/SnapshotBlocksIT.java b/server/src/test/java/org/elasticsearch/action/admin/cluster/snapshots/SnapshotBlocksIT.java index 30ae5fbe34b79..5ca7cb1e5066d 100644 --- a/server/src/test/java/org/elasticsearch/action/admin/cluster/snapshots/SnapshotBlocksIT.java +++ b/server/src/test/java/org/elasticsearch/action/admin/cluster/snapshots/SnapshotBlocksIT.java @@ -73,7 +73,7 @@ protected void setUpRepository() throws Exception { logger.info("--> verify the repository"); VerifyRepositoryResponse verifyResponse = client().admin().cluster().prepareVerifyRepository(REPOSITORY_NAME).get(); - assertThat(verifyResponse.nodes().size(), equalTo(cluster().numDataAndMasterNodes())); + assertThat(verifyResponse.getNodes().size(), equalTo(cluster().numDataAndMasterNodes())); logger.info("--> create a snapshot"); CreateSnapshotResponse snapshotResponse = client().admin().cluster().prepareCreateSnapshot(REPOSITORY_NAME, SNAPSHOT_NAME) diff --git a/server/src/test/java/org/elasticsearch/snapshots/RepositoriesIT.java b/server/src/test/java/org/elasticsearch/snapshots/RepositoriesIT.java index 317f5a5296c7c..d39d33b9d3e5e 100644 --- a/server/src/test/java/org/elasticsearch/snapshots/RepositoriesIT.java +++ b/server/src/test/java/org/elasticsearch/snapshots/RepositoriesIT.java @@ -60,7 +60,7 @@ public void testRepositoryCreation() throws Exception { logger.info("--> verify the repository"); int numberOfFiles = FileSystemUtils.files(location).length; VerifyRepositoryResponse verifyRepositoryResponse = client.admin().cluster().prepareVerifyRepository("test-repo-1").get(); - assertThat(verifyRepositoryResponse.nodes().size(), equalTo(cluster().numDataAndMasterNodes())); + assertThat(verifyRepositoryResponse.getNodes().size(), equalTo(cluster().numDataAndMasterNodes())); logger.info("--> verify that we didn't leave any files as a result of verification"); assertThat(FileSystemUtils.files(location).length, equalTo(numberOfFiles)); From c1462896dd4170196662e845687531ea24eb6140 Mon Sep 17 00:00:00 2001 From: Michael Basnight Date: Wed, 30 May 2018 14:15:59 -0500 Subject: [PATCH 3/3] Fix compile error from merge --- .../cluster/repositories/verify/VerifyRepositoryResponse.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/server/src/main/java/org/elasticsearch/action/admin/cluster/repositories/verify/VerifyRepositoryResponse.java b/server/src/main/java/org/elasticsearch/action/admin/cluster/repositories/verify/VerifyRepositoryResponse.java index 5abf8b5335b27..41835d3e11255 100644 --- a/server/src/main/java/org/elasticsearch/action/admin/cluster/repositories/verify/VerifyRepositoryResponse.java +++ b/server/src/main/java/org/elasticsearch/action/admin/cluster/repositories/verify/VerifyRepositoryResponse.java @@ -139,7 +139,7 @@ public List getNodes() { } protected void setNodes(List nodes) { - this.nodes = nodes.stream().map(n -> n.convertToDiscoveryNode()).collect(Collectors.toList()); + this.nodes = nodes; } @Override