Delete Stale Keys from Disk AND RBM #13

kiranprakash154 · 2023-12-05T17:41:05Z

Description

[Describe what this change achieves]

Related Issues

Resolves #[Issue number to be closed when this PR is merged]

Check List

New functionality includes testing.
- All tests pass
New functionality has been documented.
- New functionality has javadoc added
Commits are signed per the DCO using --signoff
Commit changes are listed out in CHANGELOG.md file (See: Changelog)
Public documentation issue/PR created

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.

to delete stale keys from diskcache

…rsettings.java

… settings

Signed-off-by: Kiran Prakash <[email protected]>

server/src/main/java/org/opensearch/common/settings/ClusterSettings.java

server/src/main/java/org/opensearch/indices/IndicesRequestCache.java

peteralfonsi · 2023-12-13T00:03:38Z

server/src/main/java/org/opensearch/indices/IndicesRequestCache.java

+
+        diskCleanupKeyToCountMap
+            .computeIfAbsent(shardId, k -> ConcurrentCollections.newConcurrentMap())
+            .merge(cleanupKey.readerCacheKeyId, 1, Integer::sum);


Maybe add a comment explaining this since it's a bit hard to read? My understanding is add a new concurrent map as the value for shardId if it doesn't yet exist. Then take this inner map and set its value for readerCacheKeyId to 1 if it's empty, or increment it by 1 if it already exists.

your understanding is right.
I do have code comments already added on top of this method that I thought would be self explanatory.
let me know if you didn't see it or u think that was not clear enough.

peteralfonsi · 2023-12-13T00:08:54Z

server/src/main/java/org/opensearch/indices/IndicesRequestCache.java

+            return;
+        }
+
+        if (cleanupKey.readerCacheKeyId == null) {


When will the reader cache key ID be null? Is this when the shard is totally closed?

It will be null when the cache clear api is called which calls this clear() method.
https://github.com/opensearch-project/OpenSearch/blob/main/server/src/main/java/org/opensearch/indices/IndicesRequestCache.java#L132

When the shard is closed, we would call the onclose() method of the cleanupKey generated which will have the readerCacheKeyId.

so essentially.
Closing a shard, or moving to a new reader (on refresh) will always know what the current readerCacheKeyId is.
But in terms of clear api, its more like, i dont care what the current readerCacheKeyId is, cleanup everything.

server/src/main/java/org/opensearch/indices/IndicesRequestCache.java

peteralfonsi · 2023-12-13T00:48:13Z

server/src/main/java/org/opensearch/indices/IndicesService.java

    public static final Setting<Boolean> INDICES_ID_FIELD_DATA_ENABLED_SETTING = Setting.boolSetting(
        "indices.id_field_data.enabled",
        true,
        Property.Dynamic,
        Property.NodeScope
    );

+    public static final Setting<String> INDICES_REQUEST_CACHE_DISK_CLEAN_THRESHOLD_SETTING =


Could instead use Setting.DoubleSetting() from 0 to 100 (or 0.0 to 1.0), which has built in min value and max value arguments?

but that wouldn't accept 50% and instead only works for 0.5.
having string is more flexible. this is following the same pattern like CLUSTER_ROUTING_ALLOCATION_LOW_DISK_WATERMARK_SETTING --- link

server/src/internalClusterTest/java/org/opensearch/indices/IndicesRequestCacheDiskTierIT.java

peteralfonsi · 2023-12-14T00:32:46Z

server/src/internalClusterTest/java/org/opensearch/indices/IndicesRequestCacheDiskTierIT.java

+
+    // When entire disk tier is stale, test whether cache cleaner cleans up everything from disk
+    public void testDiskTierInvalidationByCacheCleanerEntireDiskTier() throws Exception {
+        int thresholdInMillis = 4_000;


Set lower so the test doesn't take so long to run? Or does it flake if you do that

it flakes, i'm setting the threshold low enough so that by the time the cleaner runs, we have inserted 2 entries.
and then put the tests to sleep until the cachecleaner runs the job. when test thread wakes up the entries would have been deleted.

peteralfonsi · 2023-12-14T00:40:51Z

server/src/internalClusterTest/java/org/opensearch/indices/IndicesRequestCacheDiskTierIT.java

+        client().prepareIndex("index").setId("1").setSource("k", "good bye");
+        ensureSearchable("index");
+
+        for (int i = 0; i < 6; i++) { // index 5 items with the new readerCacheKeyId


Isn't this 6 items?

Yep, i'm considering going back to kinder garden with my son

peteralfonsi · 2023-12-14T00:50:55Z

server/src/test/java/org/opensearch/indices/IndicesRequestCacheTests.java

+
+            double result = indicesRequestCache.diskCleanupKeysPercentage();
+
+            assertEquals(0, result, 0);


add small delta?

why ? we want this to be absolute zero since we are checking when disk cache is empty. That's why I didn't add any delta

peteralfonsi · 2023-12-14T19:55:42Z

server/src/main/java/org/opensearch/indices/IndicesService.java

@@ -215,18 +215,56 @@ public class IndicesService extends AbstractLifecycleComponent
    private static final Logger logger = LogManager.getLogger(IndicesService.class);

    public static final String INDICES_SHARDS_CLOSED_TIMEOUT = "indices.shards_closed_timeout";
+    public static final String INDICES_REQUEST_CACHE_DISK_CLEAN_THRESHOLD_SETTING_KEY = "cluster.request_cache.disk.cleanup_threshold_percentage";


Why does this one start with "cluster" instead of "indices" like the rest?

we want this to be a cluster level setting.

peteralfonsi · 2023-12-19T00:33:45Z

server/src/test/java/org/opensearch/indices/IndicesRequestCacheTests.java

+        }
+    }
+
+    public static class CleanDiskCacheTests {


These don't run as part of the test suite in intellij for me - it says bc it's part of the nested class, but it might also be bc of the junit stuff. Should prob refactor this, but I'm gonna go ahead and benchmark without those changes

I see that too, let me address this, thanks !

server/src/main/java/org/opensearch/indices/IndicesRequestCache.java

sgup432 · 2023-12-19T01:06:41Z

server/src/main/java/org/opensearch/indices/IndicesRequestCache.java

+    private final ConcurrentMap<ShardId, ConcurrentMap<String , Integer>> diskCleanupKeyToCountMap = ConcurrentCollections.newConcurrentMap();
+    private final AtomicInteger staleKeysInDiskCount = new AtomicInteger(0);


I think these variables should be extracted out and be moved to some other class. Or make it generic so that it can be extended to other tiers if needed. As otherwise it tightly couples this cache with disk properties irrespective of whether tiered caching(disk) is enabled or not.

Here diskCleanupKeyToCountMap serve a mechanism to calculate staleKeys which in itself is like a "stats" associated with disk tier. So it should be attached to disk tier itself rather than hardcoding here. In case disk tier is enabled, this stats would be automatically calculated and available, otherwise now.

server/src/main/java/org/opensearch/indices/IndicesRequestCache.java

sgup432 · 2023-12-19T15:28:16Z

server/src/main/java/org/opensearch/indices/IndicesRequestCache.java

-        keysToClean.add(new CleanupKey(entity, null));
+        CleanupKey cleanupKey = new CleanupKey(entity, null);
+        keysToClean.put(cleanupKey, new CleanupStatus());
+        updateStaleKeysInDiskCount(cleanupKey);


This shouldn't be called in case disk tier is turned off.

I'll wait addressing this until your framework changes are in, it will give me a better picture on how to move all of this into each cache layer. Ideally, yes i also want the cleanup logic of each cache to be in their own classes instead of indicesrequestcache.java, which will also address this.

sgup432 · 2023-12-19T15:46:11Z

server/src/main/java/org/opensearch/indices/IndicesRequestCache.java

+        we need to make sure we clean the disk cache as well
+        hence passing threshold as 0
+        */
+        cleanDiskCache(0);


Again call it explicitly doesn't make sense as disk tier might be turned off.

I'll wait addressing this until your framework changes are in, it will give me a better picture on how to move all of this into each cache layer. Ideally, yes i also want the cleanup logic of each cache to be in their own classes instead of indicesrequestcache.java, which will also address this.

…ath (#13…" (opensearch-project#13244) This reverts commit 3d1d5e7.

kiranprakash154 added 15 commits November 28, 2023 15:16

Update IndicesRequestCache.java

52b9dfd

to delete stale keys from diskcache

More updates to disk cache cleanup

5bab65d

register INDICES_REQUEST_CACHE_DISK_CLEAN_THRESHOLD_SETTING in cluste…

4500090

…rsettings.java

make INDICES_REQUEST_CACHE_DISK_CLEAN_INTERVAL_SETTING dynamic property

4199bc2

minor changes

6e05b9d

change the way we calculate disk key staleness

aa3c39c

fix breaking tests

83d70f7

Fix test to include IndexShard

caae291

UT for testing invalidate of DiskTier is called

9d0eab1

Introduce CleanupStatus to keysToClean

5c472ae

use that cleanupStatus and logic to update staleKeyEntries

be10a16

register INDICES_REQUEST_CACHE_DISK_CLEAN_INTERVAL_SETTING to cluster…

4c8b240

… settings

Add removal listener to update eh cache stats

eeb973e

re-organize imports

f22cdaf

Add IT tests

280cb07

kiranprakash154 self-assigned this Dec 5, 2023

Merge branch 'feature/tiered-caching' into kp/delete-stale-keys

27f122b

Signed-off-by: Kiran Prakash <[email protected]>

peteralfonsi reviewed Dec 12, 2023

View reviewed changes

server/src/main/java/org/opensearch/common/settings/ClusterSettings.java Show resolved Hide resolved

peteralfonsi reviewed Dec 12, 2023

View reviewed changes

server/src/main/java/org/opensearch/common/settings/ClusterSettings.java Show resolved Hide resolved

peteralfonsi reviewed Dec 12, 2023

View reviewed changes

server/src/main/java/org/opensearch/indices/IndicesRequestCache.java Show resolved Hide resolved

peteralfonsi reviewed Dec 12, 2023

View reviewed changes

server/src/main/java/org/opensearch/indices/IndicesRequestCache.java Show resolved Hide resolved

peteralfonsi reviewed Dec 13, 2023

View reviewed changes

server/src/main/java/org/opensearch/indices/IndicesRequestCache.java Outdated Show resolved Hide resolved

peteralfonsi reviewed Dec 13, 2023

View reviewed changes

peteralfonsi reviewed Dec 14, 2023

View reviewed changes

server/src/internalClusterTest/java/org/opensearch/indices/IndicesRequestCacheDiskTierIT.java Outdated Show resolved Hide resolved

peteralfonsi reviewed Dec 14, 2023

View reviewed changes

server/src/internalClusterTest/java/org/opensearch/indices/IndicesRequestCacheDiskTierIT.java Outdated Show resolved Hide resolved

peteralfonsi reviewed Dec 14, 2023

View reviewed changes

kiranprakash154 added 6 commits December 14, 2023 17:17

refactor cleanCache & cleanDiskCache to share methods

8f0b5bc

null checks for indexShard

8c996b3

some bugs i found

4bd1503

use assertNumCacheEntries

1911fa6

update print statement

ae0b471

edit sleep time

fce110d

kiranprakash154 changed the title ~~[Draft] Delete Stale Keys from Disk AND RBM~~ Delete Stale Keys from Disk AND RBM Dec 18, 2023

peteralfonsi reviewed Dec 19, 2023

View reviewed changes

peteralfonsi merged commit f375874 into feature/tiered-caching Dec 19, 2023
12 of 14 checks passed

sgup432 reviewed Dec 19, 2023

View reviewed changes

kiranprakash154 added the enhancement New feature or request label Jan 31, 2024

peteralfonsi pushed a commit that referenced this pull request Apr 18, 2024

Revert "Add faster scaling composite hash value encoding for remote p…

6e0ed65

…ath (#13…" (opensearch-project#13244) This reverts commit 3d1d5e7.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Delete Stale Keys from Disk AND RBM #13

Delete Stale Keys from Disk AND RBM #13

kiranprakash154 commented Dec 5, 2023

peteralfonsi Dec 13, 2023

kiranprakash154 Dec 14, 2023

peteralfonsi Dec 13, 2023

kiranprakash154 Dec 14, 2023

peteralfonsi Dec 13, 2023

kiranprakash154 Dec 14, 2023

peteralfonsi Dec 14, 2023

kiranprakash154 Dec 15, 2023

peteralfonsi Dec 14, 2023

kiranprakash154 Dec 15, 2023

peteralfonsi Dec 14, 2023

kiranprakash154 Dec 20, 2023

peteralfonsi Dec 14, 2023

kiranprakash154 Dec 20, 2023

peteralfonsi Dec 19, 2023

kiranprakash154 Dec 20, 2023

sgup432 Dec 19, 2023

sgup432 Dec 19, 2023

kiranprakash154 Dec 20, 2023

sgup432 Dec 19, 2023

kiranprakash154 Dec 20, 2023


		double result = indicesRequestCache.diskCleanupKeysPercentage();

		assertEquals(0, result, 0);

		private final ConcurrentMap<ShardId, ConcurrentMap<String , Integer>> diskCleanupKeyToCountMap = ConcurrentCollections.newConcurrentMap();
		private final AtomicInteger staleKeysInDiskCount = new AtomicInteger(0);

Delete Stale Keys from Disk AND RBM #13

Delete Stale Keys from Disk AND RBM #13

Conversation

kiranprakash154 commented Dec 5, 2023

Description

Related Issues

Check List

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

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