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

Include file cache stats in node stats api response #6333

Merged
merged 1 commit into from
Feb 23, 2023
Merged

Include file cache stats in node stats api response #6333

merged 1 commit into from
Feb 23, 2023

Conversation

adnapibar
Copy link
Contributor

@adnapibar adnapibar commented Feb 16, 2023

Description

Follow up to the #5641. This change exposes the file cache stats as part the node stats API response. The file cache was added as part of searchable snapshot in #5641. In addition this also adds an index event listener to clean up cache entries when a remote store index is deleted.

Issues Resolved

Resolves #5982

Check List

  • New functionality includes testing.
    • All tests pass
  • New functionality has been documented.
    • New functionality has javadoc added
  • Commits are signed per the DCO using --signoff
  • Commit changes are listed out in CHANGELOG.md file (See: Changelog)

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.

@github-actions
Copy link
Contributor

Gradle Check (Jenkins) Run Completed with:

@github-actions
Copy link
Contributor

Gradle Check (Jenkins) Run Completed with:

  • RESULT: UNSTABLE ❕
  • TEST FAILURES:
      1 org.opensearch.snapshots.DedicatedClusterSnapshotRestoreIT.testIndexDeletionDuringSnapshotCreationInQueue

@codecov-commenter
Copy link

codecov-commenter commented Feb 17, 2023

Codecov Report

Merging #6333 (c594d2a) into main (d439244) will increase coverage by 0.03%.
The diff coverage is 55.43%.

📣 This organization is not using Codecov’s GitHub App Integration. We recommend you install it so Codecov can continue to function properly for your repositories. Learn more

@@             Coverage Diff              @@
##               main    #6333      +/-   ##
============================================
+ Coverage     70.71%   70.74%   +0.03%     
- Complexity    59011    59049      +38     
============================================
  Files          4800     4801       +1     
  Lines        282712   282798      +86     
  Branches      40763    40767       +4     
============================================
+ Hits         199907   200073     +166     
+ Misses        66427    66303     -124     
- Partials      16378    16422      +44     
Impacted Files Coverage Δ
...min/cluster/stats/TransportClusterStatsAction.java 69.56% <ø> (ø)
...src/main/java/org/opensearch/node/NodeService.java 69.84% <0.00%> (-1.13%) ⬇️
...search/cluster/MockInternalClusterInfoService.java 0.00% <0.00%> (ø)
.../java/org/opensearch/test/InternalTestCluster.java 57.72% <ø> (-0.81%) ⬇️
...in/java/org/opensearch/indices/IndicesService.java 60.52% <7.14%> (-3.04%) ⬇️
...rch/action/admin/cluster/node/stats/NodeStats.java 52.43% <33.33%> (-1.11%) ⬇️
server/src/main/java/org/opensearch/node/Node.java 83.64% <50.00%> (ø)
...h/index/store/remote/filecache/FileCacheStats.java 71.18% <71.18%> (ø)
...on/admin/cluster/node/stats/NodesStatsRequest.java 93.15% <100.00%> (+0.09%) ⬆️
.../cluster/node/stats/TransportNodesStatsAction.java 100.00% <100.00%> (ø)
... and 501 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

}

private Path getShardPath(ShardId shardId) throws IOException {
final Path[] paths = nodeEnvironment.availableShardPaths(shardId);
Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Moving this to another PR as per Andrew's comment below, will address in there.

@andrross
Copy link
Member

In addition this also adds an index event listener to clean up cache entries when a remote store index is deleted.

Is this related to the stats API at all? If not, can we split it out as a separate PR and commit? It'll be easier to review and make the commit history a bit clearer.

@andrross
Copy link
Member

We'll want to make sure that the new file_cache section of that stats API isn't present if the feature flag is not enabled.

@adnapibar
Copy link
Contributor Author

In addition this also adds an index event listener to clean up cache entries when a remote store index is deleted.

Is this related to the stats API at all? If not, can we split it out as a separate PR and commit? It'll be easier to review and make the commit history a bit clearer.

We can, in that case we have to merge the stats API code changes first before raising a PR for the cleaner as it depends on the former.

@kotwanikunal
Copy link
Member

We'll want to make sure that the new file_cache section of that stats API isn't present if the feature flag is not enabled.

+1 on this. LGTM otherwise.

@github-actions
Copy link
Contributor

Gradle Check (Jenkins) Run Completed with:

@@ -171,6 +176,11 @@ public NodeStats(StreamInput in) throws IOException {
} else {
weightedRoutingStats = null;
}
if (in.getVersion().onOrAfter(Version.V_3_0_0)) {
Copy link
Member

Choose a reason for hiding this comment

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

I think we'll want to gate these writes behind the feature flag as well. We want to be able to make backwards-incompatible changes to this feature (like maybe remove this or change the structure) and not impact any instances that have not enabled the experimental feature flag. The only way to do that (I think) is to make sure we don't write anything over the wire unless the feature flag is turned on.

This change exposes the file cache stats as part the node stats API response.

Signed-off-by: Rabi Panda <[email protected]>
@github-actions
Copy link
Contributor

Gradle Check (Jenkins) Run Completed with:

  • RESULT: UNSTABLE ❕
  • TEST FAILURES:
      1 org.opensearch.cluster.allocation.AwarenessAllocationIT.testThreeZoneOneReplicaWithForceZoneValueAndLoadAwareness

@andrross andrross added the backport 2.x Backport to 2.x branch label Feb 23, 2023
@andrross andrross merged commit 0f3b870 into opensearch-project:main Feb 23, 2023
@opensearch-trigger-bot
Copy link
Contributor

The backport to 2.x failed:

The process '/usr/bin/git' failed with exit code 128

To backport manually, run these commands in your terminal:

# Fetch latest updates from GitHub
git fetch
# Create a new working tree
git worktree add ../.worktrees/backport-2.x 2.x
# Navigate to the new working tree
pushd ../.worktrees/backport-2.x
# Create a new branch
git switch --create backport/backport-6333-to-2.x
# Cherry-pick the merged commit of this pull request and resolve the conflicts
git cherry-pick -x --mainline 1 0f3b8705a982ac8cce43d57e235cc4e13fb3ae6f
# Push it to GitHub
git push --set-upstream origin backport/backport-6333-to-2.x
# Go back to the original working tree
popd
# Delete the working tree
git worktree remove ../.worktrees/backport-2.x

Then, create a pull request where the base branch is 2.x and the compare/head branch is backport/backport-6333-to-2.x.

@kotwanikunal
Copy link
Member

The backport to 2.x failed:

The process '/usr/bin/git' failed with exit code 128

To backport manually, run these commands in your terminal:

# Fetch latest updates from GitHub
git fetch
# Create a new working tree
git worktree add ../.worktrees/backport-2.x 2.x
# Navigate to the new working tree
pushd ../.worktrees/backport-2.x
# Create a new branch
git switch --create backport/backport-6333-to-2.x
# Cherry-pick the merged commit of this pull request and resolve the conflicts
git cherry-pick -x --mainline 1 0f3b8705a982ac8cce43d57e235cc4e13fb3ae6f
# Push it to GitHub
git push --set-upstream origin backport/backport-6333-to-2.x
# Go back to the original working tree
popd
# Delete the working tree
git worktree remove ../.worktrees/backport-2.x

Then, create a pull request where the base branch is 2.x and the compare/head branch is backport/backport-6333-to-2.x.

@adnapibar Looks like it needs a manual backport 🙂

kotwanikunal pushed a commit to kotwanikunal/OpenSearch that referenced this pull request Feb 24, 2023
)

This change exposes the file cache stats as part the node stats API response.

Signed-off-by: Rabi Panda <[email protected]>
(cherry picked from commit 0f3b870)
@kotwanikunal
Copy link
Member

Backport: #6485

andrross pushed a commit that referenced this pull request Feb 25, 2023
* Include file cache stats in node stats response (#6333)

This change exposes the file cache stats as part the node stats API response.

Signed-off-by: Rabi Panda <[email protected]>
(cherry picked from commit 0f3b870)

* Add cache reservation logic (#6350)

Signed-off-by: Kunal Kotwani <[email protected]>
(cherry picked from commit eb78246)

---------

Co-authored-by: Rabi Panda <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport 2.x Backport to 2.x branch skip-changelog
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Searchable Snapshot] Expose local file system cache stats into node stats
4 participants