> On Sept. 13, 2012, 2:45 p.m., Benjamin Hindman wrote: > > > > Benjamin Hindman wrote: > Also, if you added an fs.hpp, it doesn't appear to be in this review!
thanks! I think the table sorting JS should be much clearer now > On Sept. 13, 2012, 2:45 p.m., Benjamin Hindman wrote: > > src/common/attributes.cpp, line 40 > > <https://reviews.apache.org/r/6739/diff/4/?file=152353#file152353line40> > > > > These are a great example of a place to use a switch in C/C++ ... what > > do you think? great! I ended up asking vinod if you disliked switches when I was working on this code ;) done here and elsewhere! > On Sept. 13, 2012, 2:45 p.m., Benjamin Hindman wrote: > > src/local/local.cpp, lines 83-85 > > <https://reviews.apache.org/r/6739/diff/4/?file=152359#file152359line83> > > > > This totally fits on one line! ;) Maybe you just thought it looked > > better this way? I did ;) one-lined now > On Sept. 13, 2012, 2:45 p.m., Benjamin Hindman wrote: > > src/local/local.hpp, line 39 > > <https://reviews.apache.org/r/6739/diff/4/?file=152358#file152358line39> > > > > 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 ... sure.. by "in the future", did you want a TODO or..? I'll leave as is for now unless you want that in this CL? > On Sept. 13, 2012, 2:45 p.m., Benjamin Hindman wrote: > > src/master/http.cpp, line 70 > > <https://reviews.apache.org/r/6739/diff/4/?file=152360#file152360line70> > > > > Space between foreach and ( please. done! not sure why my brain decided this was the foreach style > On Sept. 13, 2012, 2:45 p.m., Benjamin Hindman wrote: > > src/master/http.cpp, line 93 > > <https://reviews.apache.org/r/6739/diff/4/?file=152360#file152360line93> > > > > Space between foreach and ( please. I also fixed mine from files.cpp and one in launcher.cpp, sorry about that > On Sept. 13, 2012, 2:45 p.m., Benjamin Hindman wrote: > > src/webui/master/static/controllers.js, line 14 > > <https://reviews.apache.org/r/6739/diff/4/?file=152366#file152366line14> > > > > s/html/HTML/ on the same note as the function below, I've heavily clarified this function IMO, let me know if how it reads > On Sept. 13, 2012, 2:45 p.m., Benjamin Hindman wrote: > > src/webui/master/static/controllers.js, lines 33-35 > > <https://reviews.apache.org/r/6739/diff/4/?file=152366#file152366line33> > > > > 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]? Hahah, you don't have to sugar coat it for me :) It should be much clearer now, PTAL > On Sept. 13, 2012, 2:45 p.m., Benjamin Hindman wrote: > > src/webui/master/static/controllers.js, line 256 > > <https://reviews.apache.org/r/6739/diff/4/?file=152366#file152366line256> > > > > 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). Nope, it's really a predicate that is getting applied on the javascript object for the table row. For example, sorting cpus will have a predicate of 'resources.cpus'. Although, effectively it's still like a column name, in that we don't really do special js code, so I'm leaving it named as column now. > On Sept. 13, 2012, 2:45 p.m., Benjamin Hindman wrote: > > src/webui/master/static/controllers.js, line 254 > > <https://reviews.apache.org/r/6739/diff/4/?file=152366#file152366line254> > > > > 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). First, note that it is not quite the column name here, it's actually a predicate to get at the data, so for example when CPUs is the selected column, the predicate will be 'resources.cpus', that being said I do like cleaning this up. Good call on sticking with map like usage for consistency. (1): Dropping for 2 (2): I'll stick with this, although with snake_case for consitency. Finally: note that '+' '-' does not work for strings. I also made a 'constructor' for Table, please see this article if you're wondering why the constructor body looks as it does: http://ejohn.org/blog/simple-class-instantiation/ - Ben ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/6739/#review11458 ----------------------------------------------------------- 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 > >
