-
Notifications
You must be signed in to change notification settings - Fork 24.9k
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
REST high-level client: add clear cache API #28866
REST high-level client: add clear cache API #28866
Conversation
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.
LGTM
I just left a few minor nits.
* Clears the cache of one or more indices using the Clear Cache API | ||
* <p> | ||
* See <a href="https://www.elastic.co/guide/en/elasticsearch/reference/current/indices-clearcache.html"> | ||
* Clear Cache API on elastic.co</a> |
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.
nit : do we need this \t
here? ( we did not use it in the rest of the javadocs )
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.
we need it here otherwise the line is too long
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 @olcbean talks about the indent of the new line here (line 268), to remove it and align with other lines :) like in other comments.
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 right the tab :) that we don't need
@@ -234,6 +235,17 @@ static Request flush(FlushRequest flushRequest) { | |||
return new Request(HttpPost.METHOD_NAME, endpoint, parameters.getParams(), null); | |||
} | |||
|
|||
static Request clearCache(ClearIndicesCacheRequest clearIndicesCacheRequest) { | |||
String endpoint = endpoint(clearIndicesCacheRequest.indices(), "_cache", "clear"); |
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 this result a NPE if there are no indices specified ?
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.
great catch, this is true for every broadcast response
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 fixed this
Params parameters = Params.builder(); | ||
parameters.withIndicesOptions(clearIndicesCacheRequest.indicesOptions()); | ||
parameters.putParam("query", Boolean.toString(clearIndicesCacheRequest.queryCache())); | ||
parameters.putParam("fielddata", Boolean.toString(clearIndicesCacheRequest.fieldDataCache())); |
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.
nit : isn't field_data
the preferred name ?
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 you update the \rest-api-spec\src\main\resources\rest-api-spec\api\indices.clear_cache.json
as well? ( do we need to specify all supported params, or only the preferred ones. There is also a recycler
flag in the rest specs which I do not see in the code )
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 the fielddata
thing is a mistake in the way we use ParseField
to read parameters. Looking at the history, they are just synonyms, we never deprecated one in favour of the other. We use fielddata
in our docs though. You are welcome to send a PR to remove support for the other deprecated params against master 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.
Sorry, I only went with the code ( without going into details ).
However, I just called GET _cache/clear?fielddata
in Kibana and got
#! Deprecation: Deprecated field [fielddata] used, expected [field_data] instead
{
"_shards": {
"total": 2,
"successful": 1,
"failed": 0
}
}
The deprecation seems to be wrapped in the ParseField#match
We use
fielddata
in our docs though.
Good catch! Maybe it would be better to have a separate PR for the ref docs ( so that we can track when the preferred name has been changed and since when a deprecation msg is logged ) ( I can take this one over if you want )
You are welcome to send a PR to remove support for the other deprecated params against master though ;)
Which other params do you mean? And remove them from the the rest api specs and the ref docs ?
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.
Which other params do you mean? And remove them from the the rest api specs and the ref docs ?
I meant filter
, filter_cache
, and request_cache
. I think we can remove them now in master.
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.
fielddata
is the preferred name as of my merging #28943 today.
int failedShards = clearCacheResponse.getFailedShards(); // <3> | ||
DefaultShardOperationFailedException[] failures = clearCacheResponse.getShardFailures(); // <4> | ||
// end::clear-cache-response | ||
|
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.
maybe check the returned values ?
-------------------------------------------------- | ||
include-tagged::{doc-tests}/IndicesClientDocumentationIT.java[clear-cache-request-fielddata] | ||
-------------------------------------------------- | ||
<1> Set the `fielddata` flag to `true` |
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.
nit : s/fielddata
/field_data
/
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.
fielddata
is correct now!
"recycler": { | ||
"type" : "boolean", | ||
"description" : "Clear the recycler cache" | ||
}, |
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.
Relates to #27158
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.
LGTM, sorry for the delay it took me to review this.
Params parameters = Params.builder(); | ||
parameters.withIndicesOptions(clearIndicesCacheRequest.indicesOptions()); | ||
parameters.putParam("query", Boolean.toString(clearIndicesCacheRequest.queryCache())); | ||
parameters.putParam("fielddata", Boolean.toString(clearIndicesCacheRequest.fieldDataCache())); |
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.
fielddata
is the preferred name as of my merging #28943 today.
-------------------------------------------------- | ||
include-tagged::{doc-tests}/IndicesClientDocumentationIT.java[clear-cache-request-fielddata] | ||
-------------------------------------------------- | ||
<1> Set the `fielddata` flag to `true` |
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.
fielddata
is correct now!
* es/master: (50 commits) Reject updates to the `_default_` mapping. (#29165) Improve similarity docs. (#29089) [Docs] Update api.asciidoc (#29166) Docs: Add note about missing mapping for doc values field (#29036) Fix BWC issue for PreSyncedFlushResponse Remove BytesArray and BytesReference usage from XContentFactory (#29151) Add pluggable XContentBuilder writers and human readable writers (#29120) Add unreleased version 6.2.4 (#29171) Add unreleased version 6.1.5 (#29168) Add a note about using the `retry_failed` flag before accepting data loss (#29160) Fix typo in percolate-query.asciidoc (#29155) Require HTTP::Tiny 0.070 for release notes script Set Java 9 checkstyle to depend on checkstyle conf (#28383) REST high-level client: add clear cache API (#28866) Docs: Add example of resetting index setting (#29048) Plugins: Fix module name conflict check for meta plugins (#29146) Build: Fix meta plugin bundled plugin names (#29147) Build: Simplify rest spec hack configuration (#29149) Build: Fix meta modules to not install as plugin in tests (#29150) Fix javadoc warning in Strings for missing parameter description ...
* es/6.x: (46 commits) Docs: Add note about missing mapping for doc values field (#29036) [DOCS] Removed 6.1.4, 6.2.2, and 6.2.3 coming tags Remove BytesArray and BytesReference usage from XContentFactory (#29151) Fix BWC issue for PreSyncedFlushResponse Add pluggable XContentBuilder writers and human readable writers (#29120) Add unreleased version 6.2.4 (#29171) Add unreleased version 6.1.5 (#29168) Add a note about using the `retry_failed` flag before accepting data loss (#29160) Fix typo in percolate-query.asciidoc (#29155) Add release notes for 6.1.4 and 6.2.3 Require HTTP::Tiny 0.070 for release notes script REST high-level client: add clear cache API (#28866) Relax remote check for bwc project checkouts (#28666) Set Java 9 checkstyle to depend on checkstyle conf (#28383) Docs: Add example of resetting index setting (#29048) Plugins: Fix module name conflict check for meta plugins (#29146) Build: Fix meta plugin bundled plugin names (#29147) Build: Simplify rest spec hack configuration (#29149) CLI: Close subcommands in MultiCommand (#28954) Build: Fix meta modules to not install as plugin in tests (#29150) ...
Relates to #27205