-
Notifications
You must be signed in to change notification settings - Fork 24.8k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Modify state of VerifyRepositoryResponse for bwc #30762
Changes from 4 commits
6181534
7761c26
9b0967d
6790509
75c2b73
2d59e05
6d7dd78
383d761
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -19,23 +19,111 @@ | |
|
||
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<NodeView, Void> PARSER; | ||
static { | ||
ObjectParser<NodeView, Void> parser = new ObjectParser<>("nodes"); | ||
parser.declareString(NodeView::setName, new ParseField(Fields.NAME)); | ||
PARSER = (p, v, 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()); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why not There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. heh cuz i added the 2 string constructor after i added the StreamInput one, good catch :) |
||
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; } | ||
|
||
public String getNodeId() { return nodeId; } | ||
|
||
public XContentBuilder toXContent(XContentBuilder builder, Params params) throws IOException { | ||
builder.startObject(nodeId); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. heh, i had actually added this locally after our last convo about it, but forgot to pull it out of a stash when i redid this as a separate PR :rip: |
||
builder.field(Fields.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,31 +133,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<nodes.length; i++){ | ||
nodes[i] = new DiscoveryNode(in); | ||
if (in.getVersion().onOrAfter(Version.V_6_4_0)) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Typically when we make a change like this in master we would set the version to 7.0.0 as otherwise it will be impossible to have a green CI build here since 6.x (6.4) does not have this code yet. Then when you backport you will flip the version to 6.4.0 and push a commit to master to change the version to 6.4.0. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. oh i wonder if this is why my bwc tests are failing! There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Almost surely. 😇 |
||
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_6_4_0)) { | ||
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() { | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it's a little odd that we call this
parser
here and it's not the same type asPARSER
. A standard construct in class initializers is something like:So we use the same name (in lowercase) to build up the immutable object that we are initializing. Since we are not using that pattern here, and
parser
andPARSER
are of different types, I think that they should have different names.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
++