From 6181534418b7894adc61317e239a84b668c77c1f Mon Sep 17 00:00:00 2001 From: Michael Basnight Date: Mon, 21 May 2018 13:36:40 -0500 Subject: [PATCH 1/4] Modify state of VerifyRepositoryResponse for bwc 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 | 114 ++++++++++++++++-- .../repositories/RepositoryBlocksIT.java | 2 +- .../cluster/snapshots/SnapshotBlocksIT.java | 2 +- .../snapshots/RepositoriesIT.java | 2 +- 4 files changed, 104 insertions(+), 16 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 27612a3dab24b..d490ae1452d84 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,109 @@ 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 org.elasticsearch.common.xcontent.XContentParser; +import org.elasticsearch.node.Node; import java.io.IOException; +import java.util.ArrayList; +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; + public static class NodeView implements Writeable, ToXContentObject { + private static final ObjectParser.NamedObjectParser PARSER; + static { + ObjectParser parser = new ObjectParser<>("nodes"); + parser.declareString(NodeView::setName, new ParseField(Fields.NAME)); + PARSER = (XContentParser p, Void v, String name )-> parser.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()); + this.name = in.readString(); + } + + 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(Fields.NAME, name); + builder.endObject(); + return builder; + } + + /** + * Temporary method that allows turning a {@link NodeView} into a {@link DiscoveryNode}. This will never be used in practice, but + * will be used to represent a the former as the latter in the {@link VerifyRepositoryResponse}. 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); + } + + @Override + public void writeTo(StreamOutput out) throws IOException { + out.writeString(nodeId); + out.writeString(name); + } + } + + private List nodes; private ClusterName clusterName; @@ -45,31 +131,33 @@ 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); + 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_6_4_0)) { + 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() { 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 0aa5691dc67ad..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.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 c66fa4b244f18..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.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 d9d06c26b7dcf..23cb579bfdc92 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 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().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)); From 7761c267cdd01a3e7cff6a8fc24fb4269a3b8953 Mon Sep 17 00:00:00 2001 From: Michael Basnight Date: Mon, 21 May 2018 15:04:33 -0500 Subject: [PATCH 2/4] Fixups as per review --- .../verify/VerifyRepositoryResponse.java | 26 ++++++++++--------- 1 file changed, 14 insertions(+), 12 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 d490ae1452d84..ac034c286b275 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 @@ -53,7 +53,7 @@ public static class NodeView implements Writeable, ToXContentObject { static { ObjectParser parser = new ObjectParser<>("nodes"); parser.declareString(NodeView::setName, new ParseField(Fields.NAME)); - PARSER = (XContentParser p, Void v, String name )-> parser.parse(p, new NodeView(name), null); + PARSER = (p, v, name) -> parser.parse(p, new NodeView(name), null); } final String nodeId; @@ -71,6 +71,12 @@ public NodeView(StreamInput in) throws IOException { this.name = 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; } @@ -85,11 +91,13 @@ public XContentBuilder toXContent(XContentBuilder builder, Params params) throws } /** - * Temporary method that allows turning a {@link NodeView} into a {@link DiscoveryNode}. This will never be used in practice, but - * will be used to represent a the former as the latter in the {@link VerifyRepositoryResponse}. 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}. + * 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), @@ -113,12 +121,6 @@ public boolean equals(Object obj) { public int hashCode() { return Objects.hash(nodeId, name); } - - @Override - public void writeTo(StreamOutput out) throws IOException { - out.writeString(nodeId); - out.writeString(name); - } } private List nodes; From 679050962db05da8c59526739a234ffc5b308446 Mon Sep 17 00:00:00 2001 From: Michael Basnight Date: Mon, 21 May 2018 16:05:17 -0500 Subject: [PATCH 3/4] Fix the assignment on nodes --- .../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 ac034c286b275..2af4dead581a7 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 @@ -143,7 +143,7 @@ public void readFrom(StreamInput in) throws IOException { this.nodes = in.readList(NodeView::new).stream().map(n -> n.convertToDiscoveryNode()).collect(Collectors.toList()); } else { clusterName = new ClusterName(in); - in.readList(DiscoveryNode::new); + this.nodes = in.readList(DiscoveryNode::new); } } From 75c2b732e9515cd4485c188782905e713e212a73 Mon Sep 17 00:00:00 2001 From: Michael Basnight Date: Mon, 21 May 2018 20:51:41 -0500 Subject: [PATCH 4/4] Changes as per review --- .../verify/VerifyRepositoryResponse.java | 46 ++++++++++--------- 1 file changed, 24 insertions(+), 22 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 2af4dead581a7..c3fb2d58bebf3 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 @@ -32,11 +32,8 @@ import org.elasticsearch.common.xcontent.ObjectParser; import org.elasticsearch.common.xcontent.ToXContentObject; import org.elasticsearch.common.xcontent.XContentBuilder; -import org.elasticsearch.common.xcontent.XContentParser; -import org.elasticsearch.node.Node; import java.io.IOException; -import java.util.ArrayList; import java.util.Arrays; import java.util.Collections; import java.util.List; @@ -44,16 +41,19 @@ import java.util.stream.Collectors; /** - * verify repository response + * Verify repository response */ public class VerifyRepositoryResponse extends ActionResponse implements ToXContentObject { + 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 parser = new ObjectParser<>("nodes"); - parser.declareString(NodeView::setName, new ParseField(Fields.NAME)); - PARSER = (p, v, name) -> parser.parse(p, new NodeView(name), null); + 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; @@ -67,8 +67,7 @@ public NodeView(String nodeId, String name) { } public NodeView(StreamInput in) throws IOException { - this(in.readString()); - this.name = in.readString(); + this(in.readString(), in.readString()); } @Override @@ -85,7 +84,9 @@ public void writeTo(StreamOutput out) throws IOException { public XContentBuilder toXContent(XContentBuilder builder, Params params) throws IOException { builder.startObject(nodeId); - builder.field(Fields.NAME, name); + { + builder.field(NAME, name); + } builder.endObject(); return builder; } @@ -139,7 +140,7 @@ public VerifyRepositoryResponse(ClusterName clusterName, DiscoveryNode[] nodes) @Override public void readFrom(StreamInput in) throws IOException { super.readFrom(in); - if (in.getVersion().onOrAfter(Version.V_6_4_0)) { + if (in.getVersion().onOrAfter(Version.V_7_0_0_alpha1)) { this.nodes = in.readList(NodeView::new).stream().map(n -> n.convertToDiscoveryNode()).collect(Collectors.toList()); } else { clusterName = new ClusterName(in); @@ -150,7 +151,7 @@ public void readFrom(StreamInput in) throws IOException { @Override public void writeTo(StreamOutput out) throws IOException { super.writeTo(out); - if (Version.CURRENT.onOrAfter(Version.V_6_4_0)) { + if (Version.CURRENT.onOrAfter(Version.V_7_0_0_alpha1)) { out.writeList(getNodes()); } else { clusterName.writeTo(out); @@ -166,22 +167,23 @@ 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; }