-
Notifications
You must be signed in to change notification settings - Fork 24.6k
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
Refactor _cluster/stats .nodes.fs deduplication #94780
Refactor _cluster/stats .nodes.fs deduplication #94780
Conversation
Pinging @elastic/es-data-management (Team:Data Management) |
deduplicator.add(address2, newFsInfo(path2)); | ||
FsInfo.Path total = deduplicator.getTotal(); | ||
|
||
// wait a second, this is the super-special case -- you can't actually have two nodes doing this unless something |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What very interesting thing causes this? It seems to me like it would be a bug that we even got here, right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not that I'm arguing that we ought to be asserting the opposite of what you're saing -- I'm just wondering if behavior in this case ought to even be defined.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's a pretty common case in practice, even if it's not at all common in theory. I'll catch up with you offline.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Related to #94744 and to #24472
Refactors the
ClusterStatsNodes
fs deduplication logic in order to make it easier to test, then adds four test scenarios (two of which currently pass, the two that fail are bugs that should be fixed -- I'll be fixing them in other PRs).In the response from
GET _cluster/stats
, this section:is currently governed by some logic that de-duplicates by ip address, this PR twiddles the code of that logic around a little bit without actually changing the logic.