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

change memory usage display #12172

Merged
merged 1 commit into from
Apr 13, 2015
Merged

change memory usage display #12172

merged 1 commit into from
Apr 13, 2015

Conversation

wonderflow
Copy link
Contributor

Using standard unix postfixes, for example GiB will be GB .
Add unit test for display.
It's a UI fix, fixes #10824 .

Libcontainer change is here #506

cc @resouer

Signed-off-by: Sun Jianbo wonderflow@zju.edu.cn

@icecrime
Copy link
Contributor

icecrime commented Apr 8, 2015

Thanks, I'm +1 on this. @crosbymichael WDYT?

@jessfraz
Copy link
Contributor

jessfraz commented Apr 8, 2015

I'm +1 as well

@icecrime
Copy link
Contributor

icecrime commented Apr 8, 2015

Moving to code review.

"testing"
)

type TestWriter struct {
Copy link
Contributor

Choose a reason for hiding this comment

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

I think you could use a bytes.Buffer instead of a custom type (you can use String() to query its content after it's been written to).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll change it. Thanks!

@wonderflow wonderflow force-pushed the change_stats branch 3 times, most recently from 0c31097 to f6f6ddc Compare April 9, 2015 02:26
@thaJeztah
Copy link
Member

@icecrime @jfrazelle PR was updated, PTAL

@thaJeztah
Copy link
Member

Just so it's not forgotten; the docs may contain examples showing this output, those have to be checked / updated

@wonderflow
Copy link
Contributor Author

@thaJeztah Which doc do you mean?

@thaJeztah
Copy link
Member

@wonderflow this page: http://docs.docker.com/reference/commandline/cli/#stats. The source for that is located here in the repo; docs/sources/reference/commandline/cli.md#stats.

Haven't checked if other locations mention it as well (don't remember, but it's possible)

@wonderflow
Copy link
Contributor Author

@thaJeztah Thanks. I'll check all the doc and push another commit.

@cpuguy83
Copy link
Member

cpuguy83 commented Apr 9, 2015

LGTM

@jessfraz
Copy link
Contributor

jessfraz commented Apr 9, 2015

LGTM ping @crosbymichael :)

@wonderflow
Copy link
Contributor Author

I find two places of all docs that should be changed.

@thaJeztah
Copy link
Member

Thanks, @wonderflow unfortunately, I see your pull-request now contains a merge-commit (which shouldn't be in there), also, the two commits should be "squashed". To solve this, you'll have to rebase and squash you commits. You can find some instructions for that in Work on an issue - pull and rebase frequently.

If you need help with that, feel free to ask for assistance in the #docker-dev IRC channel; there are many experienced developers there that should be able to help you out!

using standard unix postfixes add unit test for display
also change doc for memory usage display change
for example GiB will be GB

Signed-off-by: Sun Jianbo <wonderflow@zju.edu.cn>
@wonderflow
Copy link
Contributor Author

Thanks! @thaJeztah
I read the instructions and fixed my commit.
Hope it's OK now.

@thaJeztah
Copy link
Member

@wonderflow the commit looks ok now, great! (tests are running)

@wonderflow
Copy link
Contributor Author

@thaJeztah
I feel the test in windows is a little tricky. I just change some words in documents that do nothing with any code. But it report error last time. Do you know why?
Every time I face this case, I can only push it again.

@thaJeztah
Copy link
Member

@wonderflow I know some tests are sometimes randomly failing; don't worry about that! The maintainers can manually trigger the tests to be started again in those cases.

I see the tests are all green, so good!

@crosbymichael
Copy link
Contributor

LGTM

crosbymichael added a commit that referenced this pull request Apr 13, 2015
@crosbymichael crosbymichael merged commit 6fa2a1e into moby:master Apr 13, 2015
@thaJeztah
Copy link
Member

Yay, merged! Thanks for your contribution, @wonderflow!

@wonderflow wonderflow deleted the change_stats branch April 14, 2015 01:47
@wonderflow
Copy link
Contributor Author

@thaJeztah Thank you !

@@ -24,5 +24,5 @@ Run **docker stats** with multiple containers.
$ docker stats redis1 redis2
CONTAINER CPU % MEM USAGE/LIMIT MEM % NET I/O
redis1 0.07% 796 KiB/64 MiB 1.21% 788 B/648 B

Choose a reason for hiding this comment

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

looks like this line was missed

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Docker stats memory usage is misleading
8 participants