You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@mesos.apache.org by Benjamin Hindman <be...@berkeley.edu> on 2012/09/10 05:49:04 UTC
Re: Review Request: Additional webui stats and slave json data,
plus table sorting!
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/6739/#review11230
-----------------------------------------------------------
src/master/http.cpp
<https://reviews.apache.org/r/6739/#comment24168>
This is slightly convoluted (you really have to look at the code closely to figure out what is going). Two suggestions:
(1) Only do the pulling out of the value from the name=val pair if it's not a scalar (since resource.scalar().value() is already a double). And even then I think it might be more readable to do 'remove(resource.name() + "=", strings::PREFIX)'.
(2) Provide a operator << for Value (make operator << for Resource use that), and then use that here too (you'll still need to check each possible type, so make sure to put a CHECK to make sure we get a runtime check that we're getting all the values ... unless you can think of a way to do this statically).
src/master/http.cpp
<https://reviews.apache.org/r/6739/#comment24169>
See comments above.
src/slave/http.cpp
<https://reviews.apache.org/r/6739/#comment24170>
Again, same comments above.
src/slave/http.cpp
<https://reviews.apache.org/r/6739/#comment24171>
Ditto.
src/slave/slave.cpp
<https://reviews.apache.org/r/6739/#comment24172>
Not really sure, but what about os::disk instead of os::available? Or maybe it's time for a new namespace, something like 'fs' to represent filesystem operations ... then fs::available might read quite naturally. Thoughts?
src/slave/slave.cpp
<https://reviews.apache.org/r/6739/#comment24173>
Spaces around '*'.
src/slave/slave.cpp
<https://reviews.apache.org/r/6739/#comment24174>
Ditto, and parens if you don't mind.
src/slave/slave.cpp
<https://reviews.apache.org/r/6739/#comment24175>
Can all of the rhs fit on it's own newline?
src/webui/master/static/controllers.js
<https://reviews.apache.org/r/6739/#comment24179>
Rather than binding 'this', would it make more sense to inject $scope via currying? Also, using $ makes me think you are injecting some special object, but aren't these just strings?
src/webui/master/static/controllers.js
<https://reviews.apache.org/r/6739/#comment24180>
This stuff needs comments ... I have no idea what is going on.
src/webui/master/static/frameworks.html
<https://reviews.apache.org/r/6739/#comment24181>
What about "sortTable" instead of "tableSort"? The former sounds more like it does something than the latter.
third_party/libprocess/include/stout/os.hpp
<https://reviews.apache.org/r/6739/#comment24178>
It's really time for a Bytes/Kilobytes/Megabytes/Gigabytes struct. Unlike Duration, maybe it makes sense to just create a single Bytes class as the "base", or maybe call it Bits? Not sure if Size or Data are descriptive enough. Thoughts?
third_party/libprocess/include/stout/strings.hpp
<https://reviews.apache.org/r/6739/#comment24177>
const &
third_party/libprocess/include/stout/strings.hpp
<https://reviews.apache.org/r/6739/#comment24176>
const & and { on newline.
- Benjamin Hindman
On Aug. 23, 2012, 11:13 p.m., Ben Mahler wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/6739/
> -----------------------------------------------------------
>
> (Updated Aug. 23, 2012, 11:13 p.m.)
>
>
> Review request for mesos, Benjamin Hindman and Vinod Kone.
>
>
> Description
> -------
>
> See summary. Also attached a screenshot of the stats.
>
>
> This addresses bug MESOS-259.
> https://issues.apache.org/jira/browse/MESOS-259
>
>
> Diffs
> -----
>
> src/common/attributes.hpp c91e82e
> src/common/attributes.cpp 77be97c
> src/local/local.hpp 96a1206
> src/local/local.cpp 742bf0c
> src/master/http.cpp c480bc6
> src/slave/http.cpp a1f7926
> src/slave/slave.cpp 4efd41e
> src/tests/exception_tests.cpp d0d17c4
> src/tests/fault_tolerance_tests.cpp 3cbab2b
> src/tests/resource_offers_tests.cpp 6718fb1
> src/webui/master/static/controllers.js 1606e64
> src/webui/master/static/framework.html 958a3d8
> src/webui/master/static/frameworks.html 6e087fc
> src/webui/master/static/home.html 319fcb8
> src/webui/master/static/mesos.css 97a8901
> src/webui/master/static/slaves.html 735d60d
> third_party/libprocess/include/stout/os.hpp b1eceb3
> third_party/libprocess/include/stout/strings.hpp 5bf0489
>
> Diff: https://reviews.apache.org/r/6739/diff/
>
>
> Testing
> -------
>
> make check
> ./mesos-local.sh
>
>
> Thanks,
>
> Ben Mahler
>
>
Re: Review Request: Additional webui stats and slave json data,
plus table sorting!
Posted by Ben Mahler <be...@gmail.com>.
> On Sept. 10, 2012, 3:49 a.m., Benjamin Hindman wrote:
> > src/slave/slave.cpp, lines 156-157
> > <https://reviews.apache.org/r/6739/diff/3/?file=145236#file145236line156>
> >
> > Can all of the rhs fit on it's own newline?
nope, but I moved the string down as well now
> On Sept. 10, 2012, 3:49 a.m., Benjamin Hindman wrote:
> > src/slave/slave.cpp, line 116
> > <https://reviews.apache.org/r/6739/diff/3/?file=145236#file145236line116>
> >
> > Not really sure, but what about os::disk instead of os::available? Or maybe it's time for a new namespace, something like 'fs' to represent filesystem operations ... then fs::available might read quite naturally. Thoughts?
very clean! I like fs::available, fs::mkdir, fs::find, fs::open, etc..
Created fs namespace, added a TODO for migrating the appropriate functions
> On Sept. 10, 2012, 3:49 a.m., Benjamin Hindman wrote:
> > src/master/http.cpp, lines 72-80
> > <https://reviews.apache.org/r/6739/diff/3/?file=145234#file145234line72>
> >
> > This is slightly convoluted (you really have to look at the code closely to figure out what is going). Two suggestions:
> >
> > (1) Only do the pulling out of the value from the name=val pair if it's not a scalar (since resource.scalar().value() is already a double). And even then I think it might be more readable to do 'remove(resource.name() + "=", strings::PREFIX)'.
> >
> > (2) Provide a operator << for Value (make operator << for Resource use that), and then use that here too (you'll still need to check each possible type, so make sure to put a CHECK to make sure we get a runtime check that we're getting all the values ... unless you can think of a way to do this statically).
did number 2, plus a runtime LOG(FATAL) on unhandled types,
also fixed a bug in Resources::isValid()
> On Sept. 10, 2012, 3:49 a.m., Benjamin Hindman wrote:
> > src/webui/master/static/controllers.js, line 14
> > <https://reviews.apache.org/r/6739/diff/3/?file=145240#file145240line14>
> >
> > Rather than binding 'this', would it make more sense to inject $scope via currying? Also, using $ makes me think you are injecting some special object, but aren't these just strings?
yeah wasn't quite sure how to cleanly do currying in js, did some more research and fixed this to curry
killed the $'s on string vars
> On Sept. 10, 2012, 3:49 a.m., Benjamin Hindman wrote:
> > third_party/libprocess/include/stout/os.hpp, line 580
> > <https://reviews.apache.org/r/6739/diff/3/?file=145246#file145246line580>
> >
> > It's really time for a Bytes/Kilobytes/Megabytes/Gigabytes struct. Unlike Duration, maybe it makes sense to just create a single Bytes class as the "base", or maybe call it Bits? Not sure if Size or Data are descriptive enough. Thoughts?
Naming here is indeed trickier
Size: typically refers to number of things (list::size, vector::size, map::size, ...)
Data: typically refers to the data itself, rather than size (char* data, string data, ...)
Bytes: although works fine at the unit level, like Seconds, seems odd as the base class (makes me think of actual byte data, rather than # of bytes).
Duration worked nicely to avoid doing Amount<Time>, however in this case it's tricker to avoid Amount<Data> ( https://github.com/twitter/commons/tree/master/src/java/com/twitter/common/quantity )
I'd go with Bytes or Bits, preferably Bytes.
Added a ticket for this rather than TODO.
- Ben
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/6739/#review11230
-----------------------------------------------------------
On Sept. 10, 2012, 10:53 p.m., Ben Mahler wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/6739/
> -----------------------------------------------------------
>
> (Updated Sept. 10, 2012, 10:53 p.m.)
>
>
> Review request for mesos, Benjamin Hindman and Vinod Kone.
>
>
> Description
> -------
>
> See summary. Also attached a screenshot of the stats.
>
>
> This addresses bug MESOS-259.
> https://issues.apache.org/jira/browse/MESOS-259
>
>
> Diffs
> -----
>
> src/common/attributes.hpp c91e82e
> src/common/attributes.cpp 7de35bb
> src/common/resources.hpp 6e61791
> src/common/resources.cpp 6a36e28
> src/common/values.hpp c916e31
> src/common/values.cpp e1726c7
> src/local/local.hpp 96a1206
> src/local/local.cpp e077327
> src/master/http.cpp aebaf52
> src/slave/http.cpp 435ab8b
> src/slave/slave.cpp 4ea1db1
> src/tests/exception_tests.cpp bb3f2c2
> src/tests/fault_tolerance_tests.cpp 35cf7a8
> src/tests/resource_offers_tests.cpp 148fc8a
> src/webui/master/static/controllers.js 41cec18
> src/webui/master/static/framework.html 958a3d8
> src/webui/master/static/frameworks.html 6e087fc
> src/webui/master/static/home.html 319fcb8
> src/webui/master/static/mesos.css 97a8901
> src/webui/master/static/slaves.html 735d60d
> third_party/libprocess/Makefile.am eb45801
> third_party/libprocess/include/stout/os.hpp 602db1f
> third_party/libprocess/include/stout/strings.hpp aca0b02
>
> Diff: https://reviews.apache.org/r/6739/diff/
>
>
> Testing
> -------
>
> make check
> ./mesos-local.sh
>
>
> Thanks,
>
> Ben Mahler
>
>