-
Notifications
You must be signed in to change notification settings - Fork 0
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 query parameters to delete request cache on heap and disk selectively #15
Add query parameters to delete request cache on heap and disk selectively #15
Conversation
.../src/main/java/org/opensearch/action/admin/indices/cache/clear/ClearIndicesCacheRequest.java
Outdated
Show resolved
Hide resolved
.../src/main/java/org/opensearch/action/admin/indices/cache/clear/ClearIndicesCacheRequest.java
Outdated
Show resolved
Hide resolved
|
||
package org.opensearch.action; | ||
|
||
public interface InputValidator { |
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 feel like something like this must already exist somewhere... seems like a general problem that many APIs would have to solve
|
||
public void validateInput() { | ||
if (requestCache && requestCacheOnDisk) { | ||
throw new IllegalArgumentException("Invalid parameters: cannot have both requestCache and requestCacheOnDisk set 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.
Would these exceptions actually reach whatever is returned to the user who made the poorly formatted API call?
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.
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.
btw i changed requestCache to request in the error because the flag we pass to clear all entire requestcache is request.
.../test/java/org/opensearch/action/admin/indices/cache/clear/ClearIndicesCacheRequestTest.java
Outdated
Show resolved
Hide resolved
server/src/test/java/org/opensearch/indices/IndicesRequestCacheTests.java
Show resolved
Hide resolved
server/src/test/java/org/opensearch/indices/IndicesRequestCacheTests.java
Outdated
Show resolved
Hide resolved
server/src/internalClusterTest/java/org/opensearch/indices/IndicesRequestCacheDiskTierIT.java
Outdated
Show resolved
Hide resolved
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 add more details in the description?
Like url exposed, testing done etc.
public void validateInput() { | ||
if (requestCache && requestCacheOnDisk) { | ||
throw new IllegalArgumentException("Invalid parameters: cannot have both request and requestCacheOnDisk set to true"); | ||
} | ||
if (requestCache && requestCacheOnHeap) { | ||
throw new IllegalArgumentException("Invalid parameters: cannot have both request and requestCacheOnHeap set 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.
Lets remove this for now and keep it consistent with rest of the actions. Move this logic to the actual function.
Doesn't look much useful to me.
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 validity of the ClearIndicesCacheRequest should be scoped to the ClearIndicesCacheRequest class right ?
the caller of this class i.e RestClearIndicesCacheAction should not be concerned about that.
.../src/main/java/org/opensearch/action/admin/indices/cache/clear/ClearIndicesCacheRequest.java
Show resolved
Hide resolved
Yep, I should have done it. |
Description
With Tiered caching[Add Link], we will now have multiple tiers of caches, for now it is heap and disk.
We are exposing a way to clear the cache selectively.
Earlier:
POST /my-index/_cache/clear?request=true
- would clear the entire request cache.Now:
POST /my-index/_cache/clear?request=true
- will clear the entire request cache (heap & disk, if disk is enabled)New parameters
POST /my-index/_cache/clear?requestCacheOnDisk=true
- will clear the request cache on disk tier onlyPOST /my-index/_cache/clear?requestCacheOnHeap=true
- will clear the request cache on heap tier onlyPOST /my-index/_cache/clear?requestCacheOnDisk=true&requestCacheOnHeap=true
- would clear the entire request cache (heap & disk, if disk is enabled)Valid calls:
POST /my-index/_cache/clear?requestCacheOnDisk=true
POST /my-index/_cache/clear?requestCacheOnHeap=true
POST /my-index/_cache/clear?request=true
POST /my-index/_cache/clear?requestCacheOnDisk=true&requestCacheOnHeap=true
Invalid calls:
POST /my-index/_cache/clear?requestCacheOnDisk=true&request=true
POST /my-index/_cache/clear?requestCacheOnHeap=true&request=true
Related Issues
Resolves #[Issue number to be closed when this PR is merged]
Check List
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.