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<NodeView> as the
VerifyRepositoryResponse internal state.

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<NodeView, Void> PARSER;
+        static {
+            ObjectParser<NodeView, Void> 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<DiscoveryNode> 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<nodes.length; i++){
-            nodes[i] = new DiscoveryNode(in);
+        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);
+            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<NodeView> 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;
     }
 

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);
         }

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)

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));