[Backport 2.x] [Bugfix] Fix TieredSpilloverCache stats not adding correctly when sha… #16790
Conversation
…rds are closed (opensearch-project#16560) * added draft tests for tsc stats holder Signed-off-by: Peter Alfonsi <petealft@amazon.com> * first draft tsc stats bugfix Signed-off-by: Peter Alfonsi <petealft@amazon.com> * Complete tests Signed-off-by: Peter Alfonsi <petealft@amazon.com> * Cleanup Signed-off-by: Peter Alfonsi <petealft@amazon.com> * Integrate fix with TSC Signed-off-by: Peter Alfonsi <petealft@amazon.com> * Add IT Signed-off-by: Peter Alfonsi <petealft@amazon.com> * Refactor cache package names in TSC module to match with server Signed-off-by: Peter Alfonsi <petealft@amazon.com> * changelog Signed-off-by: Peter Alfonsi <petealft@amazon.com> * Revert "Refactor cache package names in TSC module to match with server" This reverts commit 3b15a7a. Signed-off-by: Peter Alfonsi <petealft@amazon.com> * Addressed Sagar's comments Signed-off-by: Peter Alfonsi <petealft@amazon.com> * More package fixes Signed-off-by: Peter Alfonsi <petealft@amazon.com> * Addressed andross's comments Signed-off-by: Peter Alfonsi <petealft@amazon.com> --------- Signed-off-by: Peter Alfonsi <petealft@amazon.com> Signed-off-by: Peter Alfonsi <peter.alfonsi@gmail.com> Co-authored-by: Peter Alfonsi <petealft@amazon.com> (cherry picked from commit c82cd2e)
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
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## 2.x #16790 +/- ##
============================================
- Coverage 71.88% 71.82% -0.07%
+ Complexity 65506 65435 -71
============================================
Files 5314 5314
Lines 305364 305369 +5
Branches 44501 44501
============================================
- Hits 219522 219320 -202
- Misses 67535 67744 +209
+ Partials 18307 18305 -2 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Thanks @peteralfonsi for raising this backport PR manually. Overall looks good, except for some of the seemingly unnecessary access modifiers. Can you fix that please, for main as well before merging this PR?
| return statsRoot; | ||
| } | ||
|
|
||
| /** | ||
| * Nodes that make up the tree in the stats holder. | ||
| */ | ||
| protected static class Node { | ||
| public static class Node { | ||
| private final String dimensionValue; | ||
| // Map from dimensionValue to the DimensionNode for that dimension value. | ||
| final Map<String, Node> children; |
There was a problem hiding this comment.
nit: Should this be private now since the getter is public? I am assuming we don't directly initialize children outside of the constructor?
There was a problem hiding this comment.
It can't be private since ImmutableCacheStatsHolder uses it during construction. But I can make it protected again. After reworking the TSC stats holder tests, it doesn't need to be public.
| Node getStatsRoot() { | ||
| public Node getStatsRoot() { | ||
| return statsRoot; | ||
| } |
There was a problem hiding this comment.
nit: Why does this need to be public? I don't any other usage throughout this PR
Signed-off-by: Peter Alfonsi <petealft@amazon.com>
|
@jainankitk The access changes were made because the TSC stats holder and original stats holder are in different packages, and testing was a problem without them. But, I was able to rework the tests to use the ImmutableCacheStatsHolder to read any necessary values, so I've reverted the access modifiers to the way they were before this PR. I'll raise the corresponding PR for main once we're happy with this version of the changes. |
Backport c82cd2e from #16560.
Opened a manual backport as the earlier one was getting stuck and I couldn't fix merge conficts without write permissions. I've closed the other one.