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.

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

[Backport 2.x] [Tiered Caching] Additional ITs for cache stats #13981

Merged

Conversation

peteralfonsi
Copy link
Contributor

Original PR: #13655

Description

Adds more ITs and UT coverage for cache stats in the tiered spillover cache. Also has 3 small bugfixes which were found during testing:

  • Cache clear API incorrectly wiped hits, misses, and eviction stats
  • Items evicted from the heap tier, but rejected from the disk tier due to policies, incorrectly weren't counted towards the total evictions for the cache
  • request_cache object in XContent response for the cache stats API was incorrectly at nodes.[node_id].request_cache instead of nodes.[node_id].caches.request_cache

Related Issues

Resolves #13455

Check List

  • New functionality includes testing.
    • All tests pass
  • New functionality has been documented.
    • New functionality has javadoc added
  • API changes companion pull request created.
  • 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.

…3655)

* Adds cache clear IT

Signed-off-by: Peter Alfonsi <[email protected]>

Cleaned up logic for cache stats ITs

Signed-off-by: Peter Alfonsi <[email protected]>

Adds more tests around tiered spillover cache

Signed-off-by: Peter Alfonsi <[email protected]>

Fixed cache stats behavior for overall /_nodes/stats call

Signed-off-by: Peter Alfonsi <[email protected]>

cleanup

Signed-off-by: Peter Alfonsi <[email protected]>

Fixed folder structure

Signed-off-by: Peter Alfonsi <[email protected]>

Addressed Sagar's comments

Signed-off-by: Peter Alfonsi <[email protected]>

Addressed Ankit's comments

Signed-off-by: Peter Alfonsi <[email protected]>

Break horrifyingly long test case into many shorter cases

Signed-off-by: Peter Alfonsi <[email protected]>

Added unsupported operation exception to TSC stats holder incrementEvictions()

Signed-off-by: Peter Alfonsi <[email protected]>

Addressed Sorabh's comments

Signed-off-by: Peter Alfonsi <[email protected]>

* rerun assemble

Signed-off-by: Peter Alfonsi <[email protected]>

* rerun gradle

Signed-off-by: Peter Alfonsi <[email protected]>

* rerun gradle

Signed-off-by: Peter Alfonsi <[email protected]>

* rerun gradle

Signed-off-by: Peter Alfonsi <[email protected]>

---------

Signed-off-by: Peter Alfonsi <[email protected]>
Co-authored-by: Peter Alfonsi <[email protected]>
(cherry picked from commit 39ae32a)
Copy link
Contributor

github-actions bot commented Jun 5, 2024

✅ Gradle check result for b3fc919: SUCCESS

Copy link

codecov bot commented Jun 5, 2024

Codecov Report

Attention: Patch coverage is 78.26087% with 5 lines in your changes missing coverage. Please review.

Project coverage is 71.28%. Comparing base (0dd892c) to head (3fc7716).
Report is 297 commits behind head on 2.x.

Files Patch % Lines
...pensearch/common/cache/service/NodeCacheStats.java 0.00% 3 Missing ⚠️
...e/common/tier/TieredSpilloverCacheStatsHolder.java 66.66% 1 Missing ⚠️
...va/org/opensearch/indices/IndicesRequestCache.java 90.90% 1 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff              @@
##                2.x   #13981      +/-   ##
============================================
- Coverage     71.28%   71.28%   -0.01%     
- Complexity    60145    61317    +1172     
============================================
  Files          4957     5039      +82     
  Lines        282799   288417    +5618     
  Branches      41409    42137     +728     
============================================
+ Hits         201591   205594    +4003     
- Misses        64189    65445    +1256     
- Partials      17019    17378     +359     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Signed-off-by: Peter Alfonsi <[email protected]>
Copy link
Contributor

github-actions bot commented Jun 5, 2024

❌ Gradle check result for 51beda5: ABORTED

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?

Signed-off-by: Peter Alfonsi <[email protected]>
Copy link
Contributor

github-actions bot commented Jun 5, 2024

✅ Gradle check result for 3fc7716: SUCCESS

@sohami sohami merged commit 8749fc3 into opensearch-project:2.x Jun 5, 2024
30 checks passed
kkewwei pushed a commit to kkewwei/OpenSearch that referenced this pull request Jul 24, 2024
…earch-project#13981)

* [Tiered Caching] Additional ITs for cache stats  (opensearch-project#13655)

* Adds cache clear IT

Signed-off-by: Peter Alfonsi <[email protected]>

Cleaned up logic for cache stats ITs

Signed-off-by: Peter Alfonsi <[email protected]>

Adds more tests around tiered spillover cache

Signed-off-by: Peter Alfonsi <[email protected]>

Fixed cache stats behavior for overall /_nodes/stats call

Signed-off-by: Peter Alfonsi <[email protected]>

cleanup

Signed-off-by: Peter Alfonsi <[email protected]>

Fixed folder structure

Signed-off-by: Peter Alfonsi <[email protected]>

Addressed Sagar's comments

Signed-off-by: Peter Alfonsi <[email protected]>

Addressed Ankit's comments

Signed-off-by: Peter Alfonsi <[email protected]>

Break horrifyingly long test case into many shorter cases

Signed-off-by: Peter Alfonsi <[email protected]>

Added unsupported operation exception to TSC stats holder incrementEvictions()

Signed-off-by: Peter Alfonsi <[email protected]>

Addressed Sorabh's comments

Signed-off-by: Peter Alfonsi <[email protected]>

* rerun assemble

Signed-off-by: Peter Alfonsi <[email protected]>

* rerun gradle

Signed-off-by: Peter Alfonsi <[email protected]>

* rerun gradle

Signed-off-by: Peter Alfonsi <[email protected]>

* rerun gradle

Signed-off-by: Peter Alfonsi <[email protected]>

---------

Signed-off-by: Peter Alfonsi <[email protected]>
Co-authored-by: Peter Alfonsi <[email protected]>
(cherry picked from commit 39ae32a)

* rerun assemble

Signed-off-by: Peter Alfonsi <[email protected]>

* rerun gradle

Signed-off-by: Peter Alfonsi <[email protected]>

---------

Signed-off-by: Peter Alfonsi <[email protected]>
Co-authored-by: Peter Alfonsi <[email protected]>
Signed-off-by: kkewwei <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants