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.14] Fix negative requestStats memory_size issue (#13553) #13591

Merged

Conversation

sgup432
Copy link
Contributor

@sgup432 sgup432 commented May 7, 2024

Description

Backports #13553

Related Issues

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.

This solves the bug where RequestStats memory_size metric was going negative in certain scenarios
as reported in the issue. It turns out that the issue occurs when an indexShard is deleted and then
reallocated on the same node. So whenever stale entries from older shard are deleted, those are
accounted for the new shard which has the same shardId.

---------

Signed-off-by: Sagar Upadhyaya <sagar.upadhyaya.121@gmail.com>
@sgup432
Copy link
Contributor Author

sgup432 commented May 7, 2024

Seems like all the changelog entries were removed recently. Maybe add a skip changelog tag in that case?

@msfroh
Copy link
Collaborator

msfroh commented May 7, 2024

Seems like all the changelog entries were removed recently. Maybe add a skip changelog tag in that case?

The change log for a release branch is the release notes. You should add this to the 2.14 release notes instead. (Actually, it should be removed from the Unreleased 2.x sections in the 2.x and main branches, since it will be released in 2.14.)

Copy link
Contributor

github-actions bot commented May 7, 2024

❌ Gradle check result for 8c74e4a: 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?

@sgup432
Copy link
Contributor Author

sgup432 commented May 7, 2024

@msfroh Added in the release notes. I thought it was already picked, my bad.

Copy link
Contributor

github-actions bot commented May 7, 2024

❌ Gradle check result for 9d76468: 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?

@sgup432 sgup432 force-pushed the backport/backport-13553-to-2.14 branch from 9d76468 to b1a7551 Compare May 7, 2024 20:49
Copy link
Contributor

github-actions bot commented May 7, 2024

❌ Gradle check result for b1a7551: null

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: Sagar Upadhyaya <sagar.upadhyaya.121@gmail.com>
@sgup432 sgup432 force-pushed the backport/backport-13553-to-2.14 branch from b1a7551 to 67169b0 Compare May 7, 2024 21:14
Copy link
Contributor

github-actions bot commented May 7, 2024

❕ Gradle check result for 67169b0: UNSTABLE

  • TEST FAILURES:
      2 org.opensearch.cluster.MinimumClusterManagerNodesIT.testThreeNodesNoClusterManagerBlock

Please review all flaky tests that succeeded after retry and create an issue if one does not already exist to track the flaky failure.

Copy link

codecov bot commented May 7, 2024

Codecov Report

Attention: Patch coverage is 92.30769% with 2 lines in your changes are missing coverage. Please review.

❗ No coverage uploaded for pull request base (2.14@8d538fa). Click here to learn what that means.

Files Patch % Lines
...nsearch/index/cache/request/ShardRequestCache.java 87.50% 1 Missing ⚠️
...va/org/opensearch/indices/IndicesRequestCache.java 94.44% 0 Missing and 1 partial ⚠️
Additional details and impacted files
@@           Coverage Diff           @@
##             2.14   #13591   +/-   ##
=======================================
  Coverage        ?   71.32%           
  Complexity      ?    61212           
=======================================
  Files           ?     5027           
  Lines           ?   287733           
  Branches        ?    42049           
=======================================
  Hits            ?   205222           
  Misses          ?    65196           
  Partials        ?    17315           

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

@andrross andrross merged commit 0f1da07 into opensearch-project:2.14 May 7, 2024
31 of 32 checks passed
@andrross
Copy link
Member

andrross commented May 7, 2024

@sgup432 Can you fix up the changelog and release notes on the 2.x and main branches for this change now?

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.

4 participants