Skip to content
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

Merged
merged 8 commits into from
May 22, 2018
Original file line number Diff line number Diff line change
Expand Up @@ -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<NodeView, Void> PARSER;
static {
ObjectParser<NodeView, Void> parser = new ObjectParser<>("nodes");
Copy link
Member

@jasontedor jasontedor May 22, 2018

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 as PARSER. A standard construct in class initializers is something like:

private static final Map<String, String> MAP;
static {
    final Map<String, String> map = new HashMap<>();
    map.put("foo", "bar");
    MAP = Collections.unmodifiableMap(map);
}

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 and PARSER are of different types, I think that they should have different names.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

++

parser.declareString(NodeView::setName, new ParseField(Fields.NAME));
PARSER = (XContentParser p, Void v, String name )-> parser.parse(p, new NodeView(name), null);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you replace ame )-> with ame) ->?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You could probably also get away with not having the types here too if you want.

}

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());
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not this(in.readString(), in.readString());?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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 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
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure I'd say it is never used in practice because it is used in 6.x. I'm certainly +1 on leaving a big chunk of javadoc explaining when and why we call the method.

* 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 {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you move this directly under the reading ctor? I like having them close together so you can see that they are the same more easily.

out.writeString(nodeId);
out.writeString(name);
}
}

private List<DiscoveryNode> nodes;

private ClusterName clusterName;

Expand All @@ -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<nodes.length; i++){
nodes[i] = new DiscoveryNode(in);
if (in.getVersion().onOrAfter(Version.V_6_4_0)) {
Copy link
Member

Choose a reason for hiding this comment

The 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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oh i wonder if this is why my bwc tests are failing!

Copy link
Member

Choose a reason for hiding this comment

The 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);
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() {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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);
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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));
Expand Down