Add Verify Repository High Level REST API #30662

hub-cap · 2018-05-16T21:23:48Z

This commit adds Verify Repository, the associated docs and tests for
the high level REST API client. One thing to note is the addition of a
class to handle deserializing the XContent into something that is usable
by the REST client. Prior to this, the VerifyRepositoryResponse handled
both transport and REST actions. This did not work because the transport
client was using an array of DiscoveryNode objects, whereas the REST API
was only seeing a representation of these, the node id and node
name. Instead of returning an object that the REST client could not
use, a new VerifyRepositoryRestResponse was created, so that it could
return an actual class representation of the data returned, as well as
could have a reasonable ObjectParser instead of custom fromXContent.

Relates #27205

This commit adds Verify Repository, the associated docs and tests for the high level REST API client. One thing to note is the addition of a class to handle deserializing the XContent into something that is usable by the REST client. Prior to this, the VerifyRepositoryResponse handled both transport and REST actions. This did not work because the transport client was using an array of DiscoveryNode objects, whereas the REST API was only seeing a representation of these, the node id and node name. Instead of returning an object that the REST client could not use, a new VerifyRepositoryRestResponse was created, so that it could return an actual class representation of the data returned, as well as could have a reasonable ObjectParser instead of custom fromXContent. Relates elastic#27205

elasticmachine · 2018-05-16T21:23:56Z

Pinging @elastic/es-core-infra

javanna

thanks a lot @hub-cap I like the direction, I left a couple of comments.

javanna · 2018-05-17T09:12:16Z

client/rest-high-level/src/test/java/org/elasticsearch/client/RequestConvertersTests.java

@@ -1566,6 +1567,21 @@ public void testCreateRepository() throws IOException {
        assertToXContentBody(putRepositoryRequest, request.getEntity());
    }

+    public void testVerifyRepository() throws IOException {
+        Map<String, String> expectedParams = new HashMap<>();
+        String repository = "repo";


randomize the name?

javanna · 2018-05-17T09:29:21Z

...evel/src/test/java/org/elasticsearch/client/documentation/SnapshotClientDocumentationIT.java

+        // end::verify-repository-execute
+
+        // tag::verify-repository-response
+        List<VerifyRepositoryRestResponse.NodeView> repositoryMetaDataResponse = response.nodes();


could we show a bit more about what to do with the response?

javanna · 2018-05-17T09:37:45Z

...org/elasticsearch/action/admin/cluster/repositories/verify/VerifyRepositoryRestResponse.java

+
+public class VerifyRepositoryRestResponse implements ToXContentObject {
+
+    public static class NodeView implements ToXContentObject {


given that it's just id and name, I wonder if we should returns node as a Map<String, String>

javanna · 2018-05-17T09:40:31Z

...org/elasticsearch/action/admin/cluster/repositories/verify/VerifyRepositoryRestResponse.java

+import java.util.List;
+import java.util.Objects;
+
+public class VerifyRepositoryRestResponse implements ToXContentObject {


I agree that we need a different response. In this case I wonder if we should change the transport client response too and align it with REST. I do not see also why transport client returns the cluster name. In that case, we would introduce the needed changes to the existing response and break the transport client. Otherwise, I think the new response should be placed among the high-level client classes. @nik9000 which way would you go?

Yeah, I'd be tempted to modify the response that we use in the server and transport client to look more like the rest response rather than make a whole new response. When we talked we weren't clear on whether or not we all felt like that was ok to do though. I think it is fine in cases where we don't need the extra data. But if we're uncomfortable making that change I can understand it. If we don't want to change the response then, yeah, I'd make a new response object and pitch it into the high level rest client.

I think we need to go case by case and this is one of the cases where it should not make a difference to return something at transport that resembles more what we return at REST, so I tend to think that this API is a good example of where we should change the existing reponse, especially given how small the response is. It will be a breaking change for transport client users though.

Yeah. We've traditionally been fairly ok with breaking transport client users in minor releases if we have a good reason. To me this is a good reason.

Question: can we actually drop data from these? Like, how would backwards compatibility work?

oh that is a good point. the 6.x class will still read and write a discovery node depending on the version but it can live without it? When you look at how much stuff we serialize over the wire, you do see how this change is useful at transport too (unless we need all this info somewhere). Would that work?

I am going to remove the DiscoveryNode from the 7.0 class, read/write NodeView and the response.nodes() will return a List<NodeView>. in 6.x i will still read/write DiscoveryNode and response.nodes() will convert from DiscoveryNode[] to List<NodeView> on the fly, so there is only 1 backing object in each version of the code. Sound valid?

++, the only thing is I would even go with a Map<String,String> if that works. not sure what you and Nik think about that.

ok I chatted with @nik9000 and @jasontedor and I will do 2 PRs. The first one (this one) will keep the backing object a DiscoveryNode[], but will read/write the NodeView class to >=6.4 nodes, and write the DiscoveryNode otherwise. Then once thats backported, ill come back in on the master branch and fix it so that it knows zero about the DiscoveryNode, and instead just reads/writes and stores a List<NodeView>

linking #30762 as I will make sure these changes are done independent of this PR.

nik9000 · 2018-05-17T13:42:12Z

...org/elasticsearch/action/admin/cluster/repositories/verify/VerifyRepositoryRestResponse.java

+    }
+
+    static final class Fields {
+        static final String NODES = "nodes";


I believe we've fallen out of love with making these classes. Can you remove it? Personally I just use the string. Some folks make a string constant. Either way is fine though.

nik9000 · 2018-05-17T13:47:48Z

...org/elasticsearch/action/admin/cluster/repositories/verify/VerifyRepositoryRestResponse.java

+import java.util.List;
+import java.util.Objects;
+
+public class VerifyRepositoryRestResponse implements ToXContentObject {


Yeah, I'd be tempted to modify the response that we use in the server and transport client to look more like the rest response rather than make a whole new response. When we talked we weren't clear on whether or not we all felt like that was ok to do though. I think it is fine in cases where we don't need the extra data. But if we're uncomfortable making that change I can understand it. If we don't want to change the response then, yeah, I'd make a new response object and pitch it into the high level rest client.

hub-cap · 2018-05-25T15:31:08Z

Im going to close this because its super out of date. All existing comments have been addressed in other PRs, and I will be adding a new, much smaller PR for Verify, now that all the hard BWC stuff has been already taken care of.

hub-cap added >enhancement review v7.0.0 v6.4.0 labels May 16, 2018

javanna mentioned this pull request May 16, 2018

Java high-level REST client completeness #27205

Closed

80 tasks

javanna requested changes May 17, 2018

View reviewed changes

nik9000 reviewed May 17, 2018

View reviewed changes

hub-cap mentioned this pull request May 21, 2018

Modify state of VerifyRepositoryResponse for bwc #30762

Merged

hub-cap closed this May 25, 2018

lcawl added the >non-issue label Aug 21, 2018

colings86 added v7.0.0-beta1 and removed v7.0.0 labels Feb 7, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add Verify Repository High Level REST API #30662

Add Verify Repository High Level REST API #30662

hub-cap commented May 16, 2018

elasticmachine commented May 16, 2018

javanna left a comment

javanna May 17, 2018

javanna May 17, 2018

javanna May 17, 2018

javanna May 17, 2018

nik9000 May 17, 2018

javanna May 17, 2018

nik9000 May 17, 2018

nik9000 May 17, 2018

javanna May 17, 2018

hub-cap May 17, 2018

javanna May 17, 2018

hub-cap May 21, 2018

hub-cap May 21, 2018

nik9000 May 17, 2018

nik9000 May 17, 2018

hub-cap commented May 25, 2018


		public class VerifyRepositoryRestResponse implements ToXContentObject {

		public static class NodeView implements ToXContentObject {

Add Verify Repository High Level REST API #30662

Add Verify Repository High Level REST API #30662

Conversation

hub-cap commented May 16, 2018

elasticmachine commented May 16, 2018

javanna left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hub-cap commented May 25, 2018