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

Remove memory section #17278

Merged
merged 1 commit into from Mar 24, 2016

Conversation

Projects
None yet
4 participants
@javanna
Copy link
Member

commented Mar 23, 2016

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

@javanna

This comment has been minimized.

Copy link
Member Author

commented Mar 23, 2016

@tlrx can you have a look please?

@javanna javanna force-pushed the javanna:fix/remove_cluster_stats_os_mem branch 3 times, most recently Mar 23, 2016

@tlrx

This comment has been minimized.

Copy link
Member

commented Mar 23, 2016

I'm afraid it's not a left over but I broke it in #12049. We do have the information in NodeStats so I think we should sum it in ClusterStats rather than simply remove it, no?

@javanna javanna force-pushed the javanna:fix/remove_cluster_stats_os_mem branch Mar 23, 2016

@javanna

This comment has been minimized.

Copy link
Member Author

commented Mar 23, 2016

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?

@javanna

This comment has been minimized.

Copy link
Member Author

commented Mar 23, 2016

one more thing: there are still cases where getMem() returns null I believe, then the total is completely useless as it would only take into account only the output from nodes that provided their value.

@tlrx

This comment has been minimized.

Copy link
Member

commented Mar 23, 2016

one more thing: there are still cases where getMem() returns null I believe,

Do you know which cases? I think we have OsProbeTests that checks the value returned by getMem() and I don't remember it failed on the platform we test on.

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?

@javanna

This comment has been minimized.

Copy link
Member Author

commented Mar 23, 2016

Do you know which cases? I think we have OsProbeTests that checks the value returned by getMem() and I don't remember it failed on the platform we test on.

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).

@pickypg

This comment has been minimized.

Copy link
Member

commented Mar 23, 2016

did you ever use this field?

@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)?

@javanna

This comment has been minimized.

Copy link
Member Author

commented Mar 23, 2016

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 0 for any 2.x version. Worse than useless to me... I am all for removing it, just in case I didn't make myself clear yet :) I would be curious to hear of people actually using it...

@pickypg

This comment has been minimized.

Copy link
Member

commented Mar 23, 2016

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 :)

@javanna

This comment has been minimized.

Copy link
Member Author

commented Mar 23, 2016

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 :)

@javanna javanna changed the title Cluster Stats: mem section is always set to 0 Cluster Stats: remove mem section Mar 24, 2016

@javanna javanna added >enhancement >breaking and removed >bug labels Mar 24, 2016

@javanna

This comment has been minimized.

Copy link
Member Author

commented Mar 24, 2016

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.

@tlrx

This comment has been minimized.

Copy link
Member

commented Mar 24, 2016

LGTM

@pickypg

This comment has been minimized.

Copy link
Member

commented Mar 24, 2016

LGTM. Sorry for the extra hassle!

@pickypg pickypg removed the review label Mar 24, 2016

@javanna

This comment has been minimized.

Copy link
Member Author

commented Mar 24, 2016

no worries! I also updated the migrate guide, which I had forgotten about at first ;)

Cluster Stats: remove mem section
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.

@javanna javanna force-pushed the javanna:fix/remove_cluster_stats_os_mem branch to ce86fc5 Mar 24, 2016

@javanna javanna merged commit 609456a into elastic:master Mar 24, 2016

1 check passed

CLA Commit author is a member of Elasticsearch
Details

@clintongormley clintongormley changed the title Cluster Stats: remove mem section Remove memory section Apr 5, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.