Remove latestSettings cache from KNNSettings #727

jmazanec15 · 2023-01-13T17:44:04Z

Description

Removes the latestSettings cache from the KNNSettings class. latestSettings cache gets updated in a consumer when the particular settings are updated.

KNNSettings.getSettingValue would pull from this cache and then fallback to the default if it is not present. However, in the case when the settings are set via opensearch.yml config file, the settings get set before any functions can be registered. So the config values never get put in the cache. This leads to getSettingValue to always return the default instead of the value specified in the config file.

To fix this, this change refactors getSettingValue to pull from the cluster settings and removes the latestSetting cache. However, because the dynamicCacheSettings have consumers that rebuild the NativeMemoryCacheManager cache when they are changed, the logic for passing the parameters to rebuild this cache had to change as well. Initially, the NativeMemoryCacheManager gets its parameters from the settings. However, because we are switching "getSettingValue" to get the values from the ClusterSettings, the new cluster settings will not yet be committed to the cluster state when the settings update consumer is called (it gets called in this chain starting here). To workaround this, this change refactors the cache to accept parameters as args.

In addition to this, I added a couple tests units tests and did some refactoring to get tests to pass.

Issues Resolved

#585

Check List

New functionality includes testing.
- All tests pass
New functionality has been documented.
- 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-13T18:03:50Z

Codecov Report

Merging #727 (3a0277d) into main (519bf1b) will increase coverage by 0.16%.
The diff coverage is 97.72%.

@@             Coverage Diff              @@
##               main     #727      +/-   ##
============================================
+ Coverage     84.43%   84.60%   +0.16%     
+ Complexity     1072     1069       -3     
============================================
  Files           152      152              
  Lines          4356     4364       +8     
  Branches        389      390       +1     
============================================
+ Hits           3678     3692      +14     
+ Misses          498      492       -6     
  Partials        180      180

Impacted Files	Coverage Δ
...rch/knn/index/memory/NativeMemoryCacheManager.java	`95.20% <96.00%> (+0.70%)`	⬆️
...ain/java/org/opensearch/knn/index/KNNSettings.java	`84.37% <100.00%> (+3.49%)`	⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

martin-gaievski · 2023-01-14T00:54:38Z

src/test/java/org/opensearch/knn/index/KNNSettingsTests.java

+ *
+ * Modifications Copyright OpenSearch Contributors. See
+ * GitHub history for details.
+ */


please fix license header to 2-lines format

martin-gaievski · 2023-01-14T01:12:19Z

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

-                NativeMemoryCacheManager.getInstance().rebuildCache();
-            });
-        }
+            ByteSizeValue maxCacheWeight = getSettingValue(KNN_MEMORY_CIRCUIT_BREAKER_LIMIT);


nit: can we change maxCacheWeight type to long, this way it's gonna be consistent with expiryTimeInMinutes setting. now we do have both primitive object and getter of complex. seems maxCacheWeight it's not used anywhere else in this method so ByteSizeValue type is not required

Sure, will update.

martin-gaievski · 2023-01-14T01:17:33Z

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

        executor.execute(() -> {
-            cache.invalidateAll();
-            initialize();
+            if (cache != null) {


curious did we have an NPE with cache var or it's more of being on a safe side?

I dont think this can be reached. The only time the cache will be null will be if rebuildCache is called before the constructor, but I dont think this can happen. I think Ill remove this.

martin-gaievski · 2023-01-14T01:21:36Z

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

+        );
+    }
+
+    private void initialize(boolean isWeightLimited, long maxWeight, boolean isExpirationLimited, long expiryTimeInMin) {


nit: I suggest we use DTO instead of 4 arguments. It's will increase maintainability for future changes if we need to add/remove settings, especially as we can re-use DTO in multiple places, e.g. rebuildCache, tests etc.

Sure, will update

martin-gaievski · 2023-01-14T01:25:50Z

src/test/java/org/opensearch/knn/index/KNNSettingsTests.java

+public class KNNSettingsTests extends KNNTestCase {
+
+    @SneakyThrows
+    public void testGetSettingValueFromConfig() {


I assume that would fail without changes in rebuild cache? do we need to include other settings as well?

Not sure I understand what you mean by "that would fail without changes in rebuild cache". Could you elaborate?

I mean is this test, if it exists before your PR, fail without you code change? And second question is about settings, we're checking here only for KNNSettings.KNN_MEMORY_CIRCUIT_BREAKER_LIMIT, but we have other settings like KNNSettings.KNN_CACHE_ITEM_EXPIRY_ENABLED and KNNSettings.KNN_CACHE_ITEM_EXPIRY_TIME_MINUTES, do we need to have similar test for them as well?

I mean is this test, if it exists before your PR, fail without you code change?

Yes, this was the failure before the fix:

2> REPRODUCE WITH: ./gradlew ':test' --tests "org.opensearch.knn.index.KNNSettingsTests.testGetSettingValueFromConfig" -Dtests.seed=2B1E0A6CFDBD324B -Dtests.security.manager=false -Dtests.locale=fr -Dtests.timezone=America/Paramaribo -Druntime.java=11 2> java.lang.AssertionError: expected:<8126464> but was:<13> at __randomizedtesting.SeedInfo.seed([2B1E0A6CFDBD324B:4423BA039518D35]:0) at org.junit.Assert.fail(Assert.java:89) at org.junit.Assert.failNotEquals(Assert.java:835) at org.junit.Assert.assertEquals(Assert.java:647) at org.junit.Assert.assertEquals(Assert.java:633)

I think my actual and expected are mixed up, but Ill go ahead and fix this.

And second question is about settings, we're checking here only for KNNSettings.KNN_MEMORY_CIRCUIT_BREAKER_LIMIT, but we have other settings like KNNSettings.KNN_CACHE_ITEM_EXPIRY_ENABLED and KNNSettings.KNN_CACHE_ITEM_EXPIRY_TIME_MINUTES, do we need to have similar test for them as well?

I think it is probably okay to test just one. We group all of the dynamic cache settings here: https://github.com/opensearch-project/k-NN/blob/main/src/main/java/org/opensearch/knn/index/KNNSettings.java#L223 and then dont handle them uniquely from there, so I think we are okay.

navneet1v · 2023-01-15T05:44:50Z

However, in the case when the settings are set via opensearch.yml config file, the settings update consumers never get called, so the config values never get put in the cache.

The better explanation for this as per the deep-dive done was config(from .yml) gets set at the start, before any functions can be registered. Hence the .yml file config will not trigger the settings update, as these are updates.

Removes the latestSettings cache from the KNNSettings class. latestSettings cache gets updated in a consumer when the particular settings are updated. KNNSettings.getSettingValue would pull from this cache and then fallback to the default if it is not present. However, in the case when the settings are set via opensearch.yml config file, the settings update consumers never get called, so the config values never get put in the cache. This leads to getSettingValue to always return the default instead of the value specified in the config file. To fix this, this change refactors getSettingValue to pull from the cluster settings and removes the latestSetting cache. However, because the dynamicCacheSettings have consumers that rebuild the NativeMemoryCacheManager cache when they are changed, the logic for passing the parameters to rebuild this cache had to change as well. Signed-off-by: John Mazanec <[email protected]>

Addresses review comments. Switches configuration of cache manager to use DTO. Signed-off-by: John Mazanec <[email protected]>

Signed-off-by: John Mazanec <[email protected]>

jmazanec15 · 2023-01-18T23:52:34Z

@navneet1v true, I updated the PR description to reflect this.

navneet1v · 2023-01-13T22:24:59Z

src/test/java/org/opensearch/knn/KNNTestCase.java

@@ -30,6 +41,18 @@ public static void resetState() {
            knnCounter.set(0L);
        }

+        ClusterService clusterService = mock(ClusterService.class);


can we @mock annotation and mock this

Sure, will update.

navneet1v · 2023-01-19T01:23:56Z

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

+@Getter
+@Builder


[Minor]
if you use @value and @builder then you don't need add @Getter and you can remove final from every variable as @value will make class variables final.

Makes sense. Will add.

navneet1v · 2023-01-19T01:25:56Z

Minor comments overall code looks good to me.

Signed-off-by: John Mazanec <[email protected]>

martin-gaievski

looks good, thank you!

Removes the latestSettings cache from the KNNSettings class. latestSettings cache gets updated in a consumer when the particular settings are updated. KNNSettings.getSettingValue would pull from this cache and then fallback to the default if it is not present. However, in the case when the settings are set via opensearch.yml config file, the settings update consumers never get called, so the config values never get put in the cache. This leads to getSettingValue to always return the default instead of the value specified in the config file. To fix this, this change refactors getSettingValue to pull from the cluster settings and removes the latestSetting cache. However, because the dynamicCacheSettings have consumers that rebuild the NativeMemoryCacheManager cache when they are changed, the logic for passing the parameters to rebuild this cache had to change as well. Also, switches configuration of cache manager to use DTO. Signed-off-by: John Mazanec <[email protected]> (cherry picked from commit 8e2ad45)

jmazanec15 added the Bug Fixes Changes to a system or product designed to handle a programming bug/glitch label Jan 13, 2023

jmazanec15 requested a review from a team January 13, 2023 17:44

jmazanec15 requested a review from navneet1v January 13, 2023 19:03

jmazanec15 assigned vamshin and martin-gaievski and unassigned vamshin and martin-gaievski Jan 13, 2023

jmazanec15 requested review from martin-gaievski and vamshin January 13, 2023 19:37

jmazanec15 added the backport 2.x label Jan 13, 2023

martin-gaievski reviewed Jan 14, 2023

View reviewed changes

jmazanec15 force-pushed the issue-585 branch from fa57cfd to 728b2a8 Compare January 17, 2023 19:20

Address review comments

4769666

Addresses review comments. Switches configuration of cache manager to use DTO. Signed-off-by: John Mazanec <[email protected]>

jmazanec15 requested a review from martin-gaievski January 18, 2023 00:54

Minor test cleanup

896816c

Signed-off-by: John Mazanec <[email protected]>

navneet1v reviewed Jan 19, 2023

View reviewed changes

Address comments

3a0277d

Signed-off-by: John Mazanec <[email protected]>

jmazanec15 requested a review from navneet1v January 19, 2023 05:05

navneet1v approved these changes Jan 19, 2023

View reviewed changes

martin-gaievski approved these changes Jan 19, 2023

View reviewed changes

jmazanec15 merged commit 8e2ad45 into opensearch-project:main Jan 19, 2023

opensearch-trigger-bot bot mentioned this pull request Jan 19, 2023

[Backport 2.x] Remove latestSettings cache from KNNSettings #733

Merged

jmazanec15 mentioned this pull request Jan 19, 2023

Circuit breaker test case failure #734

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove latestSettings cache from KNNSettings #727

Remove latestSettings cache from KNNSettings #727

jmazanec15 commented Jan 13, 2023 •

edited

Loading

codecov-commenter commented Jan 13, 2023 •

edited

Loading

martin-gaievski Jan 14, 2023

martin-gaievski Jan 14, 2023

jmazanec15 Jan 17, 2023

martin-gaievski Jan 14, 2023

jmazanec15 Jan 17, 2023

martin-gaievski Jan 14, 2023

jmazanec15 Jan 18, 2023

martin-gaievski Jan 14, 2023

jmazanec15 Jan 17, 2023

martin-gaievski Jan 18, 2023

jmazanec15 Jan 18, 2023

navneet1v commented Jan 15, 2023

jmazanec15 commented Jan 18, 2023

navneet1v Jan 13, 2023

jmazanec15 Jan 19, 2023

navneet1v Jan 19, 2023

jmazanec15 Jan 19, 2023

navneet1v commented Jan 19, 2023

martin-gaievski left a comment

		@Getter
		@Builder

Remove latestSettings cache from KNNSettings #727

Remove latestSettings cache from KNNSettings #727

Conversation

jmazanec15 commented Jan 13, 2023 • edited Loading

Description

Issues Resolved

Check List

codecov-commenter commented Jan 13, 2023 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

navneet1v commented Jan 15, 2023

jmazanec15 commented Jan 18, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

navneet1v commented Jan 19, 2023

martin-gaievski left a comment

Choose a reason for hiding this comment

jmazanec15 commented Jan 13, 2023 •

edited

Loading

codecov-commenter commented Jan 13, 2023 •

edited

Loading