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