Skip to content

Add NodeIndicesStats to NodeStatsTests#89798

Merged
kingherc merged 1 commit intoelastic:mainfrom
kingherc:enhancement/node-stats-test
Sep 7, 2022
Merged

Add NodeIndicesStats to NodeStatsTests#89798
kingherc merged 1 commit intoelastic:mainfrom
kingherc:enhancement/node-stats-test

Conversation

@kingherc
Copy link
Copy Markdown
Contributor

@kingherc kingherc commented Sep 5, 2022

Will lighten the load of upcoming PR of issue #86639.

@kingherc kingherc force-pushed the enhancement/node-stats-test branch from bbdc707 to 4400afd Compare September 5, 2022 10:36
@kingherc kingherc marked this pull request as ready for review September 5, 2022 11:38
@elasticsearchmachine elasticsearchmachine added the needs:triage Requires assignment of a team area label label Sep 5, 2022
@kingherc kingherc added :Distributed/Distributed A catch all label for anything in the Distributed Area. Please avoid if you can. and removed needs:triage Requires assignment of a team area label labels Sep 5, 2022
@elasticsearchmachine elasticsearchmachine added the Team:Distributed Meta label for distributed team. label Sep 5, 2022
@elasticsearchmachine
Copy link
Copy Markdown
Collaborator

Pinging @elastic/es-distributed (Team:Distributed)

Copy link
Copy Markdown
Contributor

@original-brownbear original-brownbear left a comment

Choose a reason for hiding this comment

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

As discussed async, lets try to turn these classes into records instead of adding so much code for manual equals and hashCode if we can.

Copy link
Copy Markdown
Contributor

@original-brownbear original-brownbear left a comment

Choose a reason for hiding this comment

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

LGTM, it's a little unfortunate that we have to be so verbose here, but I agree that this is better and easier to maintain than what we did for other stats in the tests where we manually test all the getters. It's unfortunate records don't work for us here :)

@kingherc
Copy link
Copy Markdown
Contributor Author

kingherc commented Sep 7, 2022

I agree it's a bit unfortunate. I was excited to test records until I realized that the stats need mutations (due to their add() functionality) across the code. Thanks!

@kingherc kingherc merged commit 1dcd8cc into elastic:main Sep 7, 2022
@kingherc kingherc deleted the enhancement/node-stats-test branch September 7, 2022 11:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

:Distributed/Distributed A catch all label for anything in the Distributed Area. Please avoid if you can. >non-issue Team:Distributed Meta label for distributed team. v8.5.0

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants