From a8cea90e1084983b2d045edae84187fe8708399d Mon Sep 17 00:00:00 2001 From: Michael Basnight Date: Tue, 22 May 2018 14:55:20 -0500 Subject: [PATCH] Modify state of VerifyRepositoryResponse for bwc (#30762) The VerifyRepositoryResponse class holds a DiscoveryNode[], but the nodes themselves are not serialized to a REST API consumer. Since we do not want to put all of a DiscoveryNode over the wire, be it REST or Transport since its unused, this change introduces a BWC compatible change in ser/deser of the Response. Anything 6.4 and above will read/write a NodeView, and anything prior will read/write a DiscoveryNode. Further changes to 7.0 will be introduced to remove the BWC shim and only read/write NodeView, and hold a List as the VerifyRepositoryResponse internal state. --- .../verify/VerifyRepositoryResponse.java | 138 +++++++++++++++--- .../repositories/RepositoryBlocksIT.java | 2 +- .../cluster/snapshots/SnapshotBlocksIT.java | 2 +- .../snapshots/RepositoriesIT.java | 2 +- 4 files changed, 118 insertions(+), 26 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 27612a3dab2..c3fb2d58beb 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,112 @@ 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; /** - * Unregister repository response + * Verify repository response */ public class VerifyRepositoryResponse extends ActionResponse implements ToXContentObject { - private DiscoveryNode[] nodes; + static final String NODES = "nodes"; + static final String NAME = "name"; + + public static class NodeView implements Writeable, ToXContentObject { + private static final ObjectParser.NamedObjectParser PARSER; + static { + ObjectParser internalParser = new ObjectParser<>(NODES); + internalParser.declareString(NodeView::setName, new ParseField(NAME)); + PARSER = (p, v, name) -> internalParser.parse(p, new NodeView(name), null); + } + + final String nodeId; + String name; + + public NodeView(String nodeId) { this.nodeId = nodeId; } + + public NodeView(String nodeId, String name) { + this(nodeId); + this.name = name; + } + + public NodeView(StreamInput in) throws IOException { + this(in.readString(), in.readString()); + } + + @Override + public void writeTo(StreamOutput out) throws IOException { + out.writeString(nodeId); + out.writeString(name); + } + + void setName(String name) { this.name = name; } + + public String getName() { return name; } + + public String getNodeId() { return nodeId; } + + public XContentBuilder toXContent(XContentBuilder builder, Params params) throws IOException { + builder.startObject(nodeId); + { + builder.field(NAME, name); + } + builder.endObject(); + 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) { + return false; + } + if (getClass() != obj.getClass()) { + return false; + } + NodeView other = (NodeView) obj; + return Objects.equals(nodeId, other.nodeId) && + Objects.equals(name, other.name); + } + + @Override + public int hashCode() { + return Objects.hash(nodeId, name); + } + } + + private List nodes; private ClusterName clusterName; @@ -45,53 +134,56 @@ public class VerifyRepositoryResponse extends ActionResponse implements ToXConte public VerifyRepositoryResponse(ClusterName clusterName, DiscoveryNode[] nodes) { this.clusterName = clusterName; - this.nodes = nodes; + this.nodes = Arrays.asList(nodes); } @Override public void readFrom(StreamInput in) throws IOException { super.readFrom(in); - clusterName = new ClusterName(in); - nodes = new DiscoveryNode[in.readVInt()]; - for (int i=0; i n.convertToDiscoveryNode()).collect(Collectors.toList()); + } else { + clusterName = new ClusterName(in); + this.nodes = in.readList(DiscoveryNode::new); } } @Override public void writeTo(StreamOutput out) throws IOException { super.writeTo(out); - clusterName.writeTo(out); - out.writeVInt(nodes.length); - for (DiscoveryNode node : nodes) { - node.writeTo(out); + if (Version.CURRENT.onOrAfter(Version.V_7_0_0_alpha1)) { + out.writeList(getNodes()); + } else { + clusterName.writeTo(out); + out.writeList(nodes); } } - public DiscoveryNode[] getNodes() { - return nodes; + public List getNodes() { + return nodes.stream().map(dn -> new NodeView(dn.getId(), dn.getName())).collect(Collectors.toList()); } public ClusterName getClusterName() { return clusterName; } - static final class Fields { - static final String NODES = "nodes"; - static final String NAME = "name"; - } - @Override public XContentBuilder toXContent(XContentBuilder builder, Params params) throws IOException { builder.startObject(); - builder.startObject(Fields.NODES); - for (DiscoveryNode node : nodes) { - builder.startObject(node.getId()); - builder.field(Fields.NAME, node.getName()); + { + builder.startObject(NODES); + { + for (DiscoveryNode node : nodes) { + builder.startObject(node.getId()); + { + builder.field(NAME, node.getName()); + } + builder.endObject(); + } + } builder.endObject(); } builder.endObject(); - builder.endObject(); return builder; } 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 0aa5691dc67..43d94f56e5a 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 class RepositoryBlocksIT extends ESIntegTestCase { try { setClusterReadOnly(true); VerifyRepositoryResponse response = client().admin().cluster().prepareVerifyRepository("test-repo-blocks").execute().actionGet(); - assertThat(response.getNodes().length, 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 c66fa4b244f..5ca7cb1e506 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 @@ public class SnapshotBlocksIT extends ESIntegTestCase { logger.info("--> verify the repository"); VerifyRepositoryResponse verifyResponse = client().admin().cluster().prepareVerifyRepository(REPOSITORY_NAME).get(); - assertThat(verifyResponse.getNodes().length, 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 d9d06c26b7d..23cb579bfdc 100644 --- a/server/src/test/java/org/elasticsearch/snapshots/RepositoriesIT.java +++ b/server/src/test/java/org/elasticsearch/snapshots/RepositoriesIT.java @@ -61,7 +61,7 @@ public class RepositoriesIT extends AbstractSnapshotIntegTestCase { logger.info("--> verify the repository"); int numberOfFiles = FileSystemUtils.files(location).length; VerifyRepositoryResponse verifyRepositoryResponse = client.admin().cluster().prepareVerifyRepository("test-repo-1").get(); - assertThat(verifyRepositoryResponse.getNodes().length, 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));