----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/6739/#review11458 -----------------------------------------------------------
src/common/attributes.cpp <https://reviews.apache.org/r/6739/#comment24534> These are a great example of a place to use a switch in C/C++ ... what do you think? src/common/attributes.cpp <https://reviews.apache.org/r/6739/#comment24535> Saying "Unhandled" makes it sound like it's a future TODO, how about "Unexpected" instead? Also, s/Type/type/. Please do this throughout this review. src/common/resources.hpp <https://reviews.apache.org/r/6739/#comment24536> Thank you! src/local/local.hpp <https://reviews.apache.org/r/6739/#comment24537> In the future, let's consider overloading launch with a the version that takes disk, and having the version that doesn't take disk call the one which does with a default value ... src/local/local.cpp <https://reviews.apache.org/r/6739/#comment24538> This totally fits on one line! ;) Maybe you just thought it looked better this way? src/master/http.cpp <https://reviews.apache.org/r/6739/#comment24533> Space between foreach and ( please. src/master/http.cpp <https://reviews.apache.org/r/6739/#comment24532> Space between foreach and ( please. src/slave/slave.cpp <https://reviews.apache.org/r/6739/#comment24539> Thanks! src/webui/master/static/controllers.js <https://reviews.apache.org/r/6739/#comment24540> s/html/HTML/ src/webui/master/static/controllers.js <https://reviews.apache.org/r/6739/#comment24544> This comment is fantastic. I really like the use of 'column' and 'flip' to describe that happens when this function gets called (I can visualize a table, sorting it by some column, and flipping the sort based on that column). What doesn't follow for me is the code. In particular, the word column is never used once in the function body. And what is this predicates stuff? And how should I be reading $scope.reverses[table]? src/webui/master/static/controllers.js <https://reviews.apache.org/r/6739/#comment24542> How about just: "Adding bindings into scope so that they can be used from within AngularJS expressions." src/webui/master/static/controllers.js <https://reviews.apache.org/r/6739/#comment24546> I understand that the orderBy construct does indeed take a 'predicate' to decide how to do the actual ordering, but in our case we just do sorting by column "name", so the introduction of the name 'predicate', especially at the top-level of scope, seems hard to understand and very heavy-handed. Also, you use predicates here as an object, but in sortTable and other places you use it more like a map. Basically you're doing "reflection" other places except here. I'm no JS expert, but I'd say where ever we want to think of something as more of a map, we should consistently use the map like syntax (my guess is the map like syntax is harder to optimize by a JS virtual machine). Here are a few suggestions for making this stuff simpler to reason about: (1) Call the map orderingColumn or selectedColumn. Then orderBy:selectedColumn['active_tasks'] reads to me really cleanly: "order this by the selected column in the active_tasks table". In fact, it might even make sense to change 'sortTable' to 'selectColumn' (since the sorting is basically being done "under the covers"). (2) Create another "object" to describe a table, then do something like orderBy:tables['active_tasks'].selectedColumn. In this world, you could probably actually s/selectedColumn/predicate/ because you can put really good documentation whenever you set predicate (i.e., in sortTable). (In general, if you can get things to read cleanly with a singly named thing, e.g., predicate over selectedColumn, more power to you. Just not at the loss of information.) Finally, I think you might be able to eliminate 'reverses' all together by using '+' and '-' as a prefix to the column name. If you do (1), that might be nice because then you don't have two top-level maps that are actually very intertwined. If you do (2), then you can just put a 'reverse' on the "Table" object that you store, i.e., orderBy:tables['active_task'].predicate:tables['active_task'].reverse). src/webui/master/static/controllers.js <https://reviews.apache.org/r/6739/#comment24547> Is the magic here that it looks for a case-insensitive column "header" named 'id'? Would it make more sense to use the case-sensitive name so that it's easier to understand how that works (i.e., reading the HTML code below). - Benjamin Hindman 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 > >
