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