[Bugfix] Fix TieredSpilloverCache stats not adding correctly when shards are closed #16560
Conversation
Signed-off-by: Peter Alfonsi <petealft@amazon.com>
Signed-off-by: Peter Alfonsi <petealft@amazon.com>
Signed-off-by: Peter Alfonsi <petealft@amazon.com>
Signed-off-by: Peter Alfonsi <petealft@amazon.com>
Signed-off-by: Peter Alfonsi <petealft@amazon.com>
peteralfonsi
requested review from
anasalkouz,
andrross,
ashking94,
Bukhtawar,
CEHENKLE,
dblock,
dbwiddis,
gbbafna,
jainankitk,
kotwanikunal,
linuxpi,
mch2,
msfroh,
nknize,
owaiskazi19,
reta,
Rishikesh1159,
sachinpkale,
saratvemulapalli,
shwetathareja,
sohami and
VachaShah
as code owners
|
❌ Gradle check result for c2a05d4: FAILURE Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change? |
...-common/src/main/java/org/opensearch/common/cache/stats/TieredSpilloverCacheStatsHolder.java
Outdated
Show resolved
Hide resolved
| this.diskCacheEnabled = diskCacheEnabled; | ||
| } | ||
|
|
||
| @Override | ||
| public void removeDimensions(List<String> dimensionValues) { |
There was a problem hiding this comment.
Can't we just reuse removeDimensions method from DefaultCacheStatsHolder? Both look exactly same.
There was a problem hiding this comment.
For the TSC we check for assert dimensionValues.size() == dimensionNames.size() - 1 rather than dimensionNames.size(), so that the base case is the node whose children are the individual tier nodes, but besides the check they're otherwise the same.
| @@ -6,7 +6,7 @@ | |||
| * compatible open source license. | |||
| */ | |||
|
|
|||
| package org.opensearch.cache.common.tier; | |||
| package org.opensearch.common.cache.tier; | |||
There was a problem hiding this comment.
Can we avoid renaming these packages? This has introduced a lot of changes.
Plus for cache-common module, it makes sense to keep package name prefix as org.opensearch.cache
There was a problem hiding this comment.
We can do this if we make a couple methods in DefaultCacheStatsHolder public. Otherwise I'm not sure the tests can work out. Will make these changes.
This reverts commit 3b15a7a. Signed-off-by: Peter Alfonsi <petealft@amazon.com>
peteralfonsi
force-pushed
the
tsc-stats-bug
branch
from
381ee19
to
39f5f82
Compare
Signed-off-by: Peter Alfonsi <petealft@amazon.com>
peteralfonsi
force-pushed
the
tsc-stats-bug
branch
from
39f5f82
to
f85b719
Compare
|
❌ Gradle check result for f85b719: FAILURE Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change? |
Description
Fixes a bug where the total stats for the TieredSpilloverCache are decremented incorrectly when shards were closed. Misses and evictions from both the heap and disk tier were subtracted from the total, but this is incorrect. When the disk tier is enabled, only disk-tier misses and evictions should count towards the cache total, so only they should be subtracted. Adds UTs and ITs around this.
Also adds UT coverage for TieredSpilloverCacheStatsHolder, which was missing before.
Related Issues
Resolves #16559
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.