Clear-cache should accept `request` parameter

[clintongormley]

The indices-clear-cache action accepts filter (with filter_cache deprecated) but accepts request_cache (although it is documented to accept request

We should make request the preferred parameter and deprecate request_cache.

[StefanGor]

Hi there,
I am interested in picking this one up.
I am new to the project, but as far as I can tell, the only change needed is in RestClearIndicesCacheAction.java where REQUEST_CACHE would be renamed to REQUEST and
public static final ParseField REQUEST_CACHE = new ParseField("request_cache");
would become:
public static final ParseField REQUEST = new ParseField("request", "request_cache");
Does this seem correct?

[nik9000]

I think that sounds about right. Probably worth adding another rest test and you'll have to fix the rest tests that use the deprecated parameter so they don't fail.

…

On Fri, Mar 10, 2017, 9:43 PM Stefan Gorgiovski ***@***.***> wrote: Hi there, I am interested in picking this one up. I am new to the project, but as far as I can tell, the only change needed is in RestClearIndicesCacheAction.java <https://github.com/elastic/elasticsearch/blob/6265ef1c1ba1d308bcc28d00dccccac555e33b89/core/src/main/java/org/elasticsearch/rest/action/admin/indices/RestClearIndicesCacheAction.java> where REQUEST_CACHE would be renamed to REQUEST and public static final ParseField REQUEST_CACHE = new ParseField("request_cache"); would become: public static final ParseField REQUEST = new ParseField("request", "request_cache"); Does this seem correct? — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub <#22748 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AANLoneOeWqFSiIISDbu3vFTOz0SNfCCks5rkjSWgaJpZM4LrRNm> .

[nfsec]

Good to take care of this because the api calls to clear cache from python client don't work now.

[StefanGor]

I could use some guidance on creating a proper rest test for this. I can't find any that specifically test the cache (and didn't have to update any tests to not use the deprecated parameter).

I currently have this for my commit. While the test case I have written does fail on the current master branch and passes with this commit, I'm not sure that this is the way I should have done it.

[nik9000]

We usually use the rest tests written in yaml (most are here) to validate this kind of thing because it buys us some integration testing coverage as well. I'd see about implementing one of those instead of/in addition to the unit test you've written. I'd also have a look at adding one for the old name. That should pass without the change and fail with the change because it gets a deprecation warning. You can look up how to handle those deprecation warnings in other tests. Once you've got that going the test should pass again.

[StefanGor]

I've now added some yaml tests, is this sufficient or should I be trying to validate the output of the request as well?

[nik9000]

I've now added some yaml tests, is this sufficient or should I be trying to validate the output of the request as well?

I think it is pretty much right. You've made me realize we don't have a thing for deprecated parameters in those JSON files. I hadn't thought about that! But what you've showed me is a great start. If you open a PR I'll be glad to review it.

[colings86]

Looks like this is fixed by #23638, closing