Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Stats rework (1/3): Interfaces and implementations for tiers #24

Open
wants to merge 34 commits into
base: ehcache-disk-integ-base
Choose a base branch
from
Open

Stats rework (1/3): Interfaces and implementations for tiers #24

wants to merge 34 commits into from

Conversation

peteralfonsi
Copy link
Owner

Description

Changes stats for ICache to be tracked by ICache implementations themselves, using the CacheStats object. ICache<K, V> implementations now take in ICacheKey, which contains a K as well as a list of CacheStatsDimension objects. This contains information that can be used to aggregate cache stats. For example, you might want to aggregate by shard ID value. CacheStats can return values either in total or using a list of dimensions.

Also modifies EhcacheDiskCache and OpenSearchOnHeapCache to use the changes to ICache, and to use the new SingleDimensionCacheStats objects to track their stats.

Related Issues

Part of tiered caching (stats rework)

Check List

  • New functionality includes testing.
    • All tests pass
  • New functionality has been documented.
    • New functionality has javadoc added
  • Failing checks are inspected and point to the corresponding known issue(s) (See: Troubleshooting Failing Builds)
  • 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.

Peter Alfonsi added 17 commits February 8, 2024 11:36
CacheStats interface and SingleDimensionCacheStats impl
641abf1 
Signed-off-by: Peter Alfonsi <[email protected]>
First attempt to integrate new stats with EhcacheDiskCache
937ca51 
Signed-off-by: Peter Alfonsi <[email protected]>
Adds Serializer interface and impls for Key and BytesReference
21b1078 
Signed-off-by: Peter Alfonsi <[email protected]>
Adds ICacheKey serializer impl
a460acd 
Signed-off-by: Peter Alfonsi <[email protected]>
Added more icachekey serializer tests
31ee9ae 
Signed-off-by: Peter Alfonsi <[email protected]>
Attempts to fix ehcache hits
9069077 
Signed-off-by: Peter Alfonsi <[email protected]>
Fixed unexpected misses by implementing hashCode for ICacheKey and Ca…
18ddb23 
…cheStatsDimension

Signed-off-by: Peter Alfonsi <[email protected]>
Adds memory tracking to disk tier
8618458 
Signed-off-by: Peter Alfonsi <[email protected]>
Updated other tests
46b3e49 
Signed-off-by: Peter Alfonsi <[email protected]>
Added partial memory tests
51ea583 
Signed-off-by: Peter Alfonsi <[email protected]>
Changed cache stats API to get values by dimension list
17cf26a 
Signed-off-by: Peter Alfonsi <[email protected]>
Cleanup
be5eece 
Signed-off-by: Peter Alfonsi <[email protected]>
Redid memory tracking to use already implemented weigher fn
a6b0899 
Signed-off-by: Peter Alfonsi <[email protected]>
Split CacheStats into CacheStats and CacheStatsBase, which can't upda…
9b1e66c 
…te stats

Signed-off-by: Peter Alfonsi <[email protected]>
Readded BytesReferenceSerializer impl as ICacheKeySerializerTests dep…
01e04a8 
…ended on it

Signed-off-by: Peter Alfonsi <[email protected]>
Changed SingleDimensionCacheStats to use ConcurrentMap
3777e3f 
Signed-off-by: Peter Alfonsi <[email protected]>
Made SingleDimensionCacheStats also take in tier dimensions
6059680 
Signed-off-by: Peter Alfonsi <[email protected]>
sgup432
sgup432 reviewed Feb 26, 2024
View reviewed changes
server/src/main/java/org/opensearch/common/cache/stats/CacheStatsDimension.java Outdated
Comment on lines 21 to 22
public static final String TIER_DIMENSION_VALUE_ON_HEAP = "on_heap";
public static final String TIER_DIMENSION_VALUE_DISK = "disk";
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should not be declared here. Should be present in respective caches.

server/src/main/java/org/opensearch/common/cache/store/OpenSearchOnHeapCache.java Outdated
.setMaximumWeight(builder.getMaxWeightInBytes())
.weigher(builder.getWeigher())
.removalListener(this);
if (builder.getExpireAfterAcess() != null) {
cacheBuilder.setExpireAfterAccess(builder.getExpireAfterAcess());
}
cache = cacheBuilder.build();
this.eventListener = builder.getEventListener();
String dimensionName = Objects.requireNonNull(builder.shardIdDimensionName, "Shard id dimension name can't be null");
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This cache shouldn't be limited to a specific dimension. It is used by IRC this way but we need to make it generic(in terms of dimension) going forward. Same with other caches.

server/src/main/java/org/opensearch/common/cache/store/OpenSearchOnHeapCache.java Outdated
.setMaximumWeight(builder.getMaxWeightInBytes())
.weigher(builder.getWeigher())
.removalListener(this);
if (builder.getExpireAfterAcess() != null) {
cacheBuilder.setExpireAfterAccess(builder.getExpireAfterAcess());
}
cache = cacheBuilder.build();
this.eventListener = builder.getEventListener();
String dimensionName = Objects.requireNonNull(builder.shardIdDimensionName, "Shard id dimension name can't be null");
this.stats = new SingleDimensionCacheStats(dimensionName, CacheStatsDimension.TIER_DIMENSION_VALUE_ON_HEAP);
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This doesn't look right. Why are we limiting this to a single dimension?

server/src/main/java/org/opensearch/common/cache/stats/SingleDimensionCacheStats.java Outdated
* A CacheStats implementation for caches that aggregate over a single dimension, as well as holding a tier dimension.
* For example, caches in the IndicesRequestCache only aggregate over ShardId value.
*/
public class SingleDimensionCacheStats implements CacheStats {
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Lets not do this. In case I want to have multiple dimension, I would have to implement another class?
EhcacheDiskCache or onHeap cache should have the capability to support multiple dimensions from the start and keep it generic.

You can implement this support multiple dimensions and it can be used to have a single dimension if needed. That makes it extensible.

plugins/cache-ehcache/src/main/java/org/opensearch/cache/store/disk/EhcacheDiskCache.java Outdated
this.cache = buildCache(Duration.ofMillis(expireAfterAccess.getMillis()), builder);
this.shardIdDimensionName = Objects.requireNonNull(builder.shardIdDimensionName, "Dimension name can't be null");
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is not right. We shouldn't be limiting this to a single dimension that too hardcoding it to be shardId specific.

Peter Alfonsi and others added 8 commits February 26, 2024 12:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sgup432 sgup432 sgup432 left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@peteralfonsi @sgup432