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

REST high-level client: add clear cache API #28866

Merged
merged 10 commits into from
Mar 20, 2018

Conversation

javanna
Copy link
Member

@javanna javanna commented Mar 1, 2018

Relates to #27205

Copy link
Contributor

@olcbean olcbean left a 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>
Copy link
Contributor

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 )

Copy link
Member Author

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

Copy link
Contributor

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.

Copy link
Member Author

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

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 ?

Copy link
Member Author

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

Copy link
Member Author

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

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 ?

Copy link
Contributor

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 )

Copy link
Member Author

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 ;)

Copy link
Contributor

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 ?

Copy link
Member Author

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.

Copy link
Member

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

Copy link
Contributor

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`
Copy link
Contributor

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/

Copy link
Member

Choose a reason for hiding this comment

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

fielddata is correct now!

@olcbean
Copy link
Contributor

olcbean commented Mar 8, 2018

@javanna I just opened #28943 to deprecate "back" field_data in favor of fielddata ( then this PR does not need to change the fielddata )

"recycler": {
"type" : "boolean",
"description" : "Clear the recycler cache"
},
Copy link
Contributor

Choose a reason for hiding this comment

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

Relates to #27158

Copy link
Member

@tlrx tlrx left a 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()));
Copy link
Member

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

Choose a reason for hiding this comment

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

fielddata is correct now!

@javanna javanna merged commit ff09c82 into elastic:master Mar 20, 2018
javanna added a commit that referenced this pull request Mar 20, 2018
Relates to #27205

Also Closes #26947 (rest-spec were outdated)
martijnvg added a commit that referenced this pull request Mar 21, 2018
* 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
  ...
martijnvg added a commit that referenced this pull request Mar 21, 2018
* 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)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants