-
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
HLREST: Add Clear Roles Cache API #34187
Conversation
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.
I left some small comments. It looks good and you beat me to getting my PR up with my own version of NodesResponseHeader
that is very similar (I did it for the clear realm cache api)
/** | ||
* The request used to clear the cache for native roles stored in an index. | ||
*/ | ||
public final class ClearRolesCacheRequest implements Validatable, Closeable { |
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 don't think this class should be closeable
*/ | ||
public final class ClearRolesCacheRequest implements Validatable, Closeable { | ||
|
||
final String[] 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.
make it private?
* A utility class to parse the Nodes Header returned by | ||
* {@link RestActions#buildNodesHeader(XContentBuilder, ToXContent.Params, BaseNodesResponse)}. | ||
*/ | ||
public class NodesResponseHeader { |
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.
make it final?
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.
Is this security related? or is it generic that APIs besides security would use it? If its only for security, pls drop it in security package, as I will be moving those clients into their respective packages soon.
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.
This would be used by security, watcher, and a few cluster APIs (cluster stats, nodes info, and nodes stats).
|
||
private final List<Node> nodes; | ||
private final NodesResponseHeader header; | ||
private ClusterName clusterName; |
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.
can we just make this a String
so we do not need the ClusterName
object from the server project? Also make it final?
/** | ||
* Get the {@link ClusterName} associated with all of the nodes. | ||
* | ||
* @return Never {@code null}. |
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 should enforce this in the constructor of the class with Objects.requireNonNull
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.
++, ive been telling ppl to remove the validation method and put all the validation into the constructor. the old school validation is a relic of the old actions.
// tag::clear-roles-cache-execute-async | ||
client.security().clearRolesCacheAsync(request, RequestOptions.DEFAULT, listener); // <1> | ||
// end::clear-roles-cache-execute-async | ||
} |
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.
add a check that the request completes like assertTrue(latch.await(30L, TimeUnit.SECONDS));
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.
+1 to jays comments, good work so far.
* A utility class to parse the Nodes Header returned by | ||
* {@link RestActions#buildNodesHeader(XContentBuilder, ToXContent.Params, BaseNodesResponse)}. | ||
*/ | ||
public class NodesResponseHeader { |
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.
Is this security related? or is it generic that APIs besides security would use it? If its only for security, pls drop it in security package, as I will be moving those clients into their respective packages soon.
/** | ||
* Get the {@link ClusterName} associated with all of the nodes. | ||
* | ||
* @return Never {@code null}. |
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.
++, ive been telling ppl to remove the validation method and put all the validation into the constructor. the old school validation is a relic of the old actions.
} | ||
|
||
@SuppressWarnings("unchecked") | ||
private static final ConstructingObjectParser<ClearRolesCacheResponse, Void> PARSER = |
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.
all this stuff is typically up top of the class
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 see updates to the Doc test, but i dont see any accompanying asciidoc changes, just making sure these are done properly. Other than that, looks good.
@jaymode sorry. I didn't realize you were blocked on this one. Thanks for picking it up. |
@hub-cap good catch on the docs. I just added them |
Adds support for the Clear Roles Cache API to the High Level Rest Client. As part of this a helper class, NodesResponseHeader, has been added that enables parsing the nodes header from responses that are node requests. Relates to #29827
Adds support for the Clear Roles Cache API to the High Level Rest Client. As part of this a helper class, NodesResponseHeader, has been added that enables parsing the nodes header from responses that are node requests. Relates to #29827
Add's support to the Clear Roles Cache API to the High Level Rest Client.
Reviews should note the addition of the NodesResponseHeader utility class (I'm not sure it's in the right place) and also the fact this specific API has a weird response structure:
I chose to the response object I added to have a
getHeader()
method to return the content of the_nodes
section and agetNodes()
method to return the node info (mimicking the response object in ES core). We can also choose to fold the header into the top level response class and expose total, successful, failed and failure directly (at the expense of the class not looking like the JSON)/Relates to #29827