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.

Sign up for GitHub

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

Jump to bottom

Set includeShardsStats = false in NodesStatsRequest where the caller does not use shards-level statistics #100938

Merged
merged 6 commits into from
Nov 23, 2023
Merged

Set includeShardsStats = false in NodesStatsRequest where the caller does not use shards-level statistics #100938

merged 6 commits into from
Nov 23, 2023

Conversation

NEUpanning
Copy link
Contributor

Relates #100466

@elasticsearchmachine elasticsearchmachine added needs:triage Requires assignment of a team area label external-contributor Pull request authored by a developer outside the Elasticsearch team v8.12.0 labels Oct 17, 2023
@NEUpanning NEUpanning mentioned this pull request Oct 17, 2023
Merged
@javanna javanna added :Data Management/Stats Statistics tracking and retrieval APIs and removed needs:triage Requires assignment of a team area label labels Oct 18, 2023
@elasticsearchmachine elasticsearchmachine added the Team:Data Management Meta label for data/management team label Oct 18, 2023
@elasticsearchmachine
Copy link
Collaborator

Pinging @elastic/es-data-management (Team:Data Management)

@gmarouli gmarouli self-assigned this Nov 22, 2023
@gmarouli gmarouli self-requested a review November 22, 2023 09:25
gmarouli
gmarouli reviewed Nov 22, 2023
View reviewed changes
docs/changelog/100938.yaml Outdated
@@ -0,0 +1,5 @@
pr: 100938
summary: "Set includeShardsStats = true in NodesStatsRequest where the caller does not use shards-level statistics"
Copy link
Contributor

Choose a reason for hiding this comment

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

Just to double check, do you mean here includeShardsStats = false?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, it's my mistake. Thanks for your check. It's fixed in f72732e and this PR title is also fixed.

@gmarouli
Copy link
Contributor

@elasticmachine ok to test

@gmarouli
Copy link
Contributor

@elasticmachine update branch

@gmarouli
Copy link
Contributor

buildkite test this please

@gmarouli
Copy link
Contributor

@NEUpanning thank you for leading this! This is such a great improvement. The code looks great. I just triggered the tests now, I see no blockers on moving forward with this. Let's see if the tests agree :)

@gmarouli
Copy link
Contributor

@NEUpanning if you need any help addressing the format failures, let me know

@NEUpanning NEUpanning changed the title Set includeShardsStats = true in NodesStatsRequest where the caller does not use shards-level statistics Set includeShardsStats = false in NodesStatsRequest where the caller does not use shards-level statistics Nov 23, 2023
@NEUpanning
Copy link
Contributor Author

I resolved the format failures. @gmarouli Could you trigger the tests please?

@gmarouli
Copy link
Contributor

buildkite test this please

gmarouli
gmarouli approved these changes Nov 23, 2023
View reviewed changes
Copy link
Contributor

@gmarouli gmarouli left a comment

Choose a reason for hiding this comment

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

LGTM 🚀 , thank you @NEUpanning for your contribution!

@gmarouli gmarouli added the auto-merge-without-approval Automatically merge pull request when CI checks pass (NB doesn't wait for reviews!) label Nov 23, 2023
@gmarouli
Copy link
Contributor

@elasticmachine update branch

@gmarouli
Copy link
Contributor

buildkite test this please

@NEUpanning
Copy link
Contributor Author

@gmarouli Thank you for reviewing my code.

@elasticsearchmachine elasticsearchmachine merged commit 6cf4c80 into elastic:main Nov 23, 2023
16 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@gmarouli gmarouli gmarouli approved these changes

Assignees

@gmarouli gmarouli

Labels
auto-merge-without-approval Automatically merge pull request when CI checks pass (NB doesn't wait for reviews!) :Data Management/Stats Statistics tracking and retrieval APIs >enhancement external-contributor Pull request authored by a developer outside the Elasticsearch team Team:Data Management Meta label for data/management team v8.12.0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@NEUpanning @elasticsearchmachine @gmarouli @javanna @elasticmachine