-
Notifications
You must be signed in to change notification settings - Fork 24.4k
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
Remove memory section #17278
Remove memory section #17278
Conversation
@tlrx can you have a look please? |
87bf0be
to
35e526c
Compare
I'm afraid it's not a left over but I broke it in #12049. We do have the information in |
35e526c
to
d761457
Compare
yes @tlrx that is an option, I just thought that this info is not useful in cluster stats given that it's broken since 2.0 beta 1 and nobody ever complained about it... I wonder what the purpose is of showing how much memory is available in total throughout all the nodes. You confirm we should make this work instead of removing it? |
one more thing: there are still cases where |
Do you know which cases? I think we have I like this info because it gives an estimation of the "size" of the cluster but other than that I'm not sure it is very useful. I'm throwing the ball to @pickypg : did you ever use this field? |
Happened on my machine (mac) while manually testing. (I had made the change you are suggesting at first, before going for removing the field completely). |
@tlrx I don't see any value in having the summed amount of memory across the cluster. It's kind of neat to "see it" but I agree that it does not add value to anyone. I can't really make any useful decision based on seeing "300 GB of memory" for a cluster. It also doesn't tell you if the nodes are heterogenous, which is dangerous. As long as it's in node stats, then that should be what matters. I don't necessarily think that we should introduce this change for existing version though since it's a non-BWC change (even if the value was useless)? |
that's debatable, the value is always set to |
I don't think anyone is using it properly, but if they're doing some kind of mass collection of stats, then this could be swooped up. I wouldn't want to break it over a useless value in a .z release :) |
That means we need to fix it in 2.x, I can't leave it as-is... but we agreed that this value is useless so we kinda fix it for nothing... I will need to sleep over this :) |
ok I updated the title, description and labels of this PR. This one is now targeted at 5.0 only and will remove the fields from the response, which is a breaking change. Another PR will follow-up to fix the bug in 2.x. |
LGTM |
LGTM. Sorry for the extra hassle! |
no worries! I also updated the migrate guide, which I had forgotten about at first ;) |
The available memory metric was always set to `0` since 2.0.beta1 (bug). was left behind but never set. Turns out the section wasn't that useful, as it would only output the total memory available throughout all nodes in the cluster. We decided to remove the section then.
6a121d9
to
ce86fc5
Compare
This seems to be a leftover of #12049, where some sigar specific stats were removed. The available memory metric was left behind but never set. This section can be removed as it is not useful.
Closes #16756