[k-NN] Add Clear Cache API #740

naveentatikonda · 2023-01-24T01:44:13Z

Description

Add API to flush given k-NN indices out of cache memory.

Issues Resolved

Check List

New functionality includes testing.
- All tests pass
- New functionality has javadoc added
Commits are signed as per the DCO using --signoff

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.

codecov-commenter · 2023-01-24T02:02:25Z

Codecov Report

Merging #740 (7d10109) into main (74b87e8) will decrease coverage by 0.06%.
The diff coverage is 85.71%.

@@             Coverage Diff              @@
##               main     #740      +/-   ##
============================================
- Coverage     85.03%   84.97%   -0.06%     
- Complexity     1144     1167      +23     
============================================
  Files           154      159       +5     
  Lines          4692     4760      +68     
  Branches        425      427       +2     
============================================
+ Hits           3990     4045      +55     
- Misses          509      522      +13     
  Partials        193      193

Files Changed	Coverage Δ
...n/java/org/opensearch/knn/common/KNNConstants.java	`93.75% <ø> (ø)`
...search/knn/plugin/transport/ClearCacheRequest.java	`50.00% <50.00%> (ø)`
...earch/knn/plugin/transport/ClearCacheResponse.java	`50.00% <50.00%> (ø)`
...nsearch/knn/plugin/transport/ClearCacheAction.java	`75.00% <75.00%> (ø)`
...n/java/org/opensearch/knn/index/KNNIndexShard.java	`90.32% <80.00%> (-3.56%)`	⬇️
...nn/plugin/transport/ClearCacheTransportAction.java	`86.66% <86.66%> (ø)`
...rch/knn/index/memory/NativeMemoryCacheManager.java	`95.48% <100.00%> (+0.28%)`	⬆️
...main/java/org/opensearch/knn/plugin/KNNPlugin.java	`100.00% <100.00%> (ø)`
...nsearch/knn/plugin/rest/RestClearCacheHandler.java	`100.00% <100.00%> (ø)`

... and 3 files with indirect coverage changes

src/main/java/org/opensearch/knn/index/KNNIndexShard.java

src/main/java/org/opensearch/knn/plugin/rest/RestClearCacheHandler.java

src/main/java/org/opensearch/knn/index/KNNIndexShard.java

navneet1v

@naveentatikonda Here are some of my thoughts:

Why do we believe doing a flush operation using rest handler is the best way to remove the cache? Can this be done periodically is customer wants?
What will happen to the search request which are currently running on that index and using that cached graphs?
Can any user of OpenSearch call this API to clear the cache or there are any restrictions around the usage of the API?

I would recommend creating an issue for this so that different approaches can be discussed around what should be the best approach here.

naveentatikonda · 2023-03-28T23:36:31Z

Can I get some suggestions on API name something like "Clear k-NN Indices from Memory" or "Evict k-NN Indices from Memory" if we don't want to use the name "Clear Cache API"

Also, for the API endpoint should we use /_plugins/_knn/memory/<warmup | clear>/indices. like @jmazanec15 suggested in design discussion?

Will make the refactoring changes once the API name and endpoint are finalized.

jmazanec15 · 2023-03-30T20:23:01Z

I think I like this approach (similar to ml-commons for models: https://opensearch.org/docs/latest/ml-commons-plugin/api/#load-model):

POST /_plugins/_knn/{index_names}/_clear // remove indices from memory - if no indices provided, clear everything

POST /_plugins/_knn/{index_names}/_warmup // warmup/load indices into the cache - if no indices provided, warmup everything

I think this centers the APIs around the index/indices resource. With this, I think we could move to deprecating current warmup path:

GET /_plugins/_knn/warmup/{index_names}?pretty

Would be interested to hear what the rest of the team thinks @navneet1v @vamshin @martin-gaievski

src/main/java/org/opensearch/knn/index/KNNIndexShard.java

martin-gaievski · 2023-05-24T19:43:00Z

I think I like this approach (similar to ml-commons for models: https://opensearch.org/docs/latest/ml-commons-plugin/api/#load-model):
POST /_plugins/_knn/{index_names}/_clear // remove indices from memory - if no indices provided, clear everything

POST /_plugins/_knn/{index_names}/_warmup // warmup/load indices into the cache - if no indices provided, warmup everything
I think this centers the APIs around the index/indices resource. With this, I think we could move to deprecating current warmup path:
GET /_plugins/_knn/warmup/{index_names}?pretty
Would be interested to hear what the rest of the team thinks @navneet1v @vamshin @martin-gaievski

sounds good to me, both API suggestions and putting existing API to deprecation

CHANGELOG.md

src/main/java/org/opensearch/knn/index/KNNIndexShard.java

src/main/java/org/opensearch/knn/plugin/rest/RestClearCacheHandler.java

navneet1v · 2023-08-10T16:14:51Z

src/main/java/org/opensearch/knn/plugin/transport/ClearCacheAction.java

+    public static final ClearCacheAction INSTANCE = new ClearCacheAction();
+    public static final String NAME = "cluster:admin/clear_cache_action";
+
+    private ClearCacheAction() {


shouldn't this be public?

no, it is private. We will be only using it in this class to create the INSTANCE object.

src/main/java/org/opensearch/knn/plugin/transport/ClearCacheTransportAction.java

navneet1v · 2023-08-10T16:19:13Z

@naveentatikonda can you fix the build issues happening for the mac and ubnutu.

Signed-off-by: Naveen Tatikonda <[email protected]>

martin-gaievski · 2023-08-23T18:56:08Z

src/main/java/org/opensearch/knn/index/memory/NativeMemoryCacheManager.java

+     * @param indexName name of OpenSearch index
+     * @return NativeMemoryAllocation associated with given index
+     */
+    public Optional<NativeMemoryAllocation> getIndexMemoryAllocation(String indexName) {


nit: I think our convention is to have params in public methods as final. not a critical though, just for future PRs

navneet1v · 2023-08-23T19:26:17Z

I think I like this approach (similar to ml-commons for models: https://opensearch.org/docs/latest/ml-commons-plugin/api/#load-model):
POST /_plugins/_knn/{index_names}/_clear // remove indices from memory - if no indices provided, clear everything

POST /_plugins/_knn/{index_names}/_warmup // warmup/load indices into the cache - if no indices provided, warmup everything
I think this centers the APIs around the index/indices resource. With this, I think we could move to deprecating current warmup path:
GET /_plugins/_knn/warmup/{index_names}?pretty
Would be interested to hear what the rest of the team thinks @navneet1v @vamshin @martin-gaievski

I think we can move towards more index based rest endpoints. But I think deprecation of the endpoints and adding new endpoints is out of scope of this PR.

* Add Clear Cache API Signed-off-by: Naveen Tatikonda <[email protected]> * Add Unit and Integration tests Signed-off-by: Naveen Tatikonda <[email protected]> * Add BWC Tests Signed-off-by: Naveen Tatikonda <[email protected]> * Add CHANGELOG Signed-off-by: Naveen Tatikonda <[email protected]> * Address Review Comments Signed-off-by: Naveen Tatikonda <[email protected]> --------- Signed-off-by: Naveen Tatikonda <[email protected]> (cherry picked from commit 12f4a51)

* Add Clear Cache API Signed-off-by: Naveen Tatikonda <[email protected]> * Add Unit and Integration tests Signed-off-by: Naveen Tatikonda <[email protected]> * Add BWC Tests Signed-off-by: Naveen Tatikonda <[email protected]> * Add CHANGELOG Signed-off-by: Naveen Tatikonda <[email protected]> * Address Review Comments Signed-off-by: Naveen Tatikonda <[email protected]> --------- Signed-off-by: Naveen Tatikonda <[email protected]> (cherry picked from commit 12f4a51) Signed-off-by: Naveen Tatikonda <[email protected]>

* [k-NN] Add Clear Cache API (#740) * Add Clear Cache API Signed-off-by: Naveen Tatikonda <[email protected]> * Add Unit and Integration tests Signed-off-by: Naveen Tatikonda <[email protected]> * Add BWC Tests Signed-off-by: Naveen Tatikonda <[email protected]> * Add CHANGELOG Signed-off-by: Naveen Tatikonda <[email protected]> * Address Review Comments Signed-off-by: Naveen Tatikonda <[email protected]> --------- Signed-off-by: Naveen Tatikonda <[email protected]> (cherry picked from commit 12f4a51) Signed-off-by: Naveen Tatikonda <[email protected]> * Fix Failing Clear Cache API Test (#1336) Signed-off-by: Naveen Tatikonda <[email protected]> --------- Signed-off-by: Naveen Tatikonda <[email protected]> Co-authored-by: Naveen Tatikonda <[email protected]>

naveentatikonda added Features Introduces a new unit of functionality that satisfies a requirement backport 2.x labels Jan 24, 2023

naveentatikonda self-assigned this Jan 24, 2023

naveentatikonda requested a review from a team January 24, 2023 01:44

martin-gaievski reviewed Jan 24, 2023

View reviewed changes

navneet1v requested changes Jan 24, 2023

View reviewed changes

naveentatikonda force-pushed the add_clear_cache_api branch from f373209 to 69b67a9 Compare March 28, 2023 23:20

naveentatikonda requested review from navneet1v and martin-gaievski March 28, 2023 23:36

martin-gaievski reviewed Apr 3, 2023

View reviewed changes

src/main/java/org/opensearch/knn/index/KNNIndexShard.java Outdated Show resolved Hide resolved

src/main/java/org/opensearch/knn/index/KNNIndexShard.java Outdated Show resolved Hide resolved

vamshin changed the title ~~Add Clear Cache API~~ [k-NN] Add Clear Cache API Jul 27, 2023

naveentatikonda force-pushed the add_clear_cache_api branch from f836fd2 to 22f36a3 Compare August 9, 2023 17:35

naveentatikonda requested review from heemin32, VijayanB, vamshin, jmazanec15 and junqiu-lei as code owners August 9, 2023 17:35

naveentatikonda force-pushed the add_clear_cache_api branch 2 times, most recently from 4f167d0 to 01bb6c1 Compare August 9, 2023 23:27

navneet1v reviewed Aug 10, 2023

View reviewed changes

naveentatikonda added 4 commits August 11, 2023 18:39

Add Clear Cache API

c2b568a

Signed-off-by: Naveen Tatikonda <[email protected]>

Add Unit and Integration tests

f340792

Signed-off-by: Naveen Tatikonda <[email protected]>

Add BWC Tests

ad0931c

Signed-off-by: Naveen Tatikonda <[email protected]>

Add CHANGELOG

d89b361

Signed-off-by: Naveen Tatikonda <[email protected]>

naveentatikonda force-pushed the add_clear_cache_api branch from 01bb6c1 to e3e54b1 Compare August 11, 2023 23:46

naveentatikonda requested a review from martin-gaievski August 11, 2023 23:55

Address Review Comments

7d10109

Signed-off-by: Naveen Tatikonda <[email protected]>

naveentatikonda force-pushed the add_clear_cache_api branch from e3e54b1 to 7d10109 Compare August 11, 2023 23:59

naveentatikonda requested a review from navneet1v August 16, 2023 16:52

navneet1v approved these changes Aug 23, 2023

View reviewed changes

martin-gaievski reviewed Aug 23, 2023

View reviewed changes

martin-gaievski approved these changes Aug 23, 2023

View reviewed changes

naveentatikonda merged commit 12f4a51 into opensearch-project:main Aug 23, 2023

opensearch-trigger-bot bot mentioned this pull request Aug 23, 2023

[Backport 2.x] [k-NN] Add Clear Cache API #1055

Merged

jmazanec15 mentioned this pull request Nov 28, 2023

Fix 2.10 Release Notes #1327

Merged

1 task

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[k-NN] Add Clear Cache API #740

[k-NN] Add Clear Cache API #740

naveentatikonda commented Jan 24, 2023 •

edited

Loading

codecov-commenter commented Jan 24, 2023 •

edited by codecov bot

Loading

navneet1v left a comment •

edited

Loading

naveentatikonda commented Mar 28, 2023

jmazanec15 commented Mar 30, 2023

martin-gaievski commented May 24, 2023

navneet1v Aug 10, 2023

naveentatikonda Aug 11, 2023

navneet1v commented Aug 10, 2023

martin-gaievski Aug 23, 2023

navneet1v commented Aug 23, 2023

[k-NN] Add Clear Cache API #740

[k-NN] Add Clear Cache API #740

Conversation

naveentatikonda commented Jan 24, 2023 • edited Loading

Description

Issues Resolved

Check List

codecov-commenter commented Jan 24, 2023 • edited by codecov bot Loading

Codecov Report

navneet1v left a comment • edited Loading

Choose a reason for hiding this comment

naveentatikonda commented Mar 28, 2023

jmazanec15 commented Mar 30, 2023

martin-gaievski commented May 24, 2023

navneet1v Aug 10, 2023

Choose a reason for hiding this comment

naveentatikonda Aug 11, 2023

Choose a reason for hiding this comment

navneet1v commented Aug 10, 2023

martin-gaievski Aug 23, 2023

Choose a reason for hiding this comment

navneet1v commented Aug 23, 2023

naveentatikonda commented Jan 24, 2023 •

edited

Loading

codecov-commenter commented Jan 24, 2023 •

edited by codecov bot

Loading

navneet1v left a comment •

edited

Loading