-
Notifications
You must be signed in to change notification settings - Fork 24.7k
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
Add Verify Repository High Level REST API #30662
Conversation
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
Pinging @elastic/es-core-infra |
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.
thanks a lot @hub-cap I like the direction, I left a couple of comments.
@@ -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"; |
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.
randomize the name?
// end::verify-repository-execute | ||
|
||
// tag::verify-repository-response | ||
List<VerifyRepositoryRestResponse.NodeView> repositoryMetaDataResponse = response.nodes(); |
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.
could we show a bit more about what to do with the response?
|
||
public class VerifyRepositoryRestResponse implements ToXContentObject { | ||
|
||
public static class NodeView implements ToXContentObject { |
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.
given that it's just id and name, I wonder if we should returns node as a Map<String, String>
import java.util.List; | ||
import java.util.Objects; | ||
|
||
public class VerifyRepositoryRestResponse implements ToXContentObject { |
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 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?
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.
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.
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 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.
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.
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.
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.
Question: can we actually drop data from these? Like, how would backwards compatibility work?
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.
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?
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 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?
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.
++, 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.
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.
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>
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.
linking #30762 as I will make sure these changes are done independent of this PR.
} | ||
|
||
static final class Fields { | ||
static final String NODES = "nodes"; |
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 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.
import java.util.List; | ||
import java.util.Objects; | ||
|
||
public class VerifyRepositoryRestResponse implements ToXContentObject { |
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.
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.
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. |
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