> On Aug. 16, 2012, 4:04 p.m., Benjamin Hindman wrote: > > third_party/libprocess/include/stout/path.hpp, line 20 > > <https://reviews.apache.org/r/6617/diff/1/?file=140343#file140343line20> > > > > &
> On Aug. 16, 2012, 4:04 p.m., Benjamin Hindman wrote: > > src/tests/files_tests.cpp, lines 254-255 > > <https://reviews.apache.org/r/6617/diff/1/?file=140340#file140340line254> > > > > Same comment as in files.cpp, unless you can't do that because C++ > > expects both expressions in the ternary if to have exactly the same types. > > In which case, I retract my comment above! yeah that was the issue with ternary > On Aug. 16, 2012, 4:04 p.m., Benjamin Hindman wrote: > > src/files/files.cpp, line 130 > > <https://reviews.apache.org/r/6617/diff/1/?file=140338#file140338line130> > > > > s/val/value/ > > > > Also, make both types be const&. > > > > Finally, this is too useful to sit locked away in this file! Please add > > it to process/http.hpp as an additional constructor for OK. Then you'd do: > > > > return OK(value, jsonp); > > > > Alternatively, you could add a top-level function that adds JSON to the > > body of any Response object (more general, but we can always do that later > > and make it specific for OK responses now). Very clean, I like it! Added constructor to OK > On Aug. 16, 2012, 4:04 p.m., Benjamin Hindman wrote: > > src/files/files.cpp, line 190 > > <https://reviews.apache.org/r/6617/diff/1/?file=140338#file140338line190> > > > > Could just s/files/os::ls(path.get())/ done > On Aug. 16, 2012, 4:04 p.m., Benjamin Hindman wrote: > > src/files/files.cpp, line 194 > > <https://reviews.apache.org/r/6617/diff/1/?file=140338#file140338line194> > > > > Just an FYI: we try and construct C++ wrappers for C functions as much > > as possible. In your case you use multiple things returned from stat, so > > it's a bit harder. We've gotten away thus far with only needing one thing > > from stat at a time and thus have functions that wrap stat and just return > > that one thing. If you don't object to double stat'ing for each non-dir in the listing, I could add an os::fileSize or something? Otherwise, I'll leave as is unless you have alternatives? > On Aug. 16, 2012, 4:04 p.m., Benjamin Hindman wrote: > > src/files/files.cpp, line 195 > > <https://reviews.apache.org/r/6617/diff/1/?file=140338#file140338line195> > > > > Some sort of log warning would be nice. Added, but why? The file could have simply been deleted since we did the listing in which case we really wouldn't care. But I guess just in case something else went wrong > On Aug. 16, 2012, 4:04 p.m., Benjamin Hindman wrote: > > src/files/files.cpp, lines 204-205 > > <https://reviews.apache.org/r/6617/diff/1/?file=140338#file140338line204> > > > > file.values["dir"] = S_ISDIR(s.st_mode) ? JSON::True() : JSON::False(); Sadly this does not work due to the typing. > On Aug. 16, 2012, 4:04 p.m., Benjamin Hindman wrote: > > src/files/files.cpp, line 225 > > <https://reviews.apache.org/r/6617/diff/1/?file=140338#file140338line225> > > > > I believe this breaks the webui (master/slave logs), so it would be > > great if this commit included that fix. Also, I'd like us to have a > > consistent query "key" for both 'browse' and 'read' (having to remember one > > keyword will be much better in the long run). I had chosen "name" because > > that matches with what you do when you "attach". However, I'd be okay (and > > perhaps is better) to do "path" instead, i.e.: > > > > ip:port/files/browse?path=... > > ip:port/files/read?path=... I went for clarity (file vs dir) but I like the consistency of path, fixed! (Path also makes more sense from the client side, like the ui etc) Also thanks for catching the webui thing, please look the fixes over in controllers.js :) > On Aug. 16, 2012, 4:04 p.m., Benjamin Hindman wrote: > > src/files/files.cpp, line 440 > > <https://reviews.apache.org/r/6617/diff/1/?file=140338#file140338line440> > > > > It can not, because then we'd have to expose FilesProcess to/in > > files.hpp. k > On Aug. 16, 2012, 4:04 p.m., Benjamin Hindman wrote: > > src/tests/files_tests.cpp, line 245 > > <https://reviews.apache.org/r/6617/diff/1/?file=140340#file140340line245> > > > > You could throw this in a header and use it in files.cpp as well as > > here. done, put it in files.hpp but it feels a bit out of place.. > On Aug. 16, 2012, 4:04 p.m., Benjamin Hindman wrote: > > src/files/files.cpp, line 407 > > <https://reviews.apache.org/r/6617/diff/1/?file=140338#file140338line407> > > > > Try<string> result => Result<string> result ... how fitting. hm? os::realpath returns a Try<string> > On Aug. 16, 2012, 4:04 p.m., Benjamin Hindman wrote: > > src/tests/files_tests.cpp, line 45 > > <https://reviews.apache.org/r/6617/diff/1/?file=140340#file140340line45> > > > > s/val/value/ > > > > Also, this should really be from an implementation of operator << > > (ostream, ...). Then you can just do: 'stringify(value)' below and it will > > convert the JSON object to a string. I like that better. so looking at json.hpp the Value is not a struct, but rather a boost::variant typedef.. so I'm not so sure how to implement the << operator for that? Seems like this was the original reason for the recursive Renderer in the first place.. So.. I tried to implement a stringify template override for JSON::Value but couldn't get the compiler to like it > On Aug. 16, 2012, 4:04 p.m., Benjamin Hindman wrote: > > src/tests/configurator_tests.cpp, line 133 > > <https://reviews.apache.org/r/6617/diff/1/?file=140339#file140339line133> > > > > There is already a mkdir in stout. I'd prefer: > > > > mkdir("conf2") || FAIL() << "Failed to create directory 'conf2'"; > > > > Otherwise mkDir has a nasty side-effect of failing without you > > realizing. I noticed these, but sadly it doesn't fit on one line like that, hence why I made the test helpers (mkdir and writeFile) to fail the test on error under the hood. I agree it's clunky to have the helpers here, but it's also really clunky to error check every mkdir and write in the tests (especially since it requires an if) maybe if I do something like: if (mkdir("1", 0755) || mkdir("2", 0755) || mkdir("3", 0755) { FAIL() << "..."; // but this doesn't know which mkdir failed. } Thoughts? Right now I've renamed the test helpers to match the os::_ names but with cleaner signatures and they fail under the hood. > On Aug. 16, 2012, 4:04 p.m., Benjamin Hindman wrote: > > src/tests/configurator_tests.cpp, line 134 > > <https://reviews.apache.org/r/6617/diff/1/?file=140339#file140339line134> > > > > There is also a "write" in stout/os.hpp. > > > > Try<bool> result = os::write("conf2/mesos.conf", > > ....); > > > > result.isSome() || FAIL() << "" << result.error(); similarly to above, PTAL and let me know your thoughts > On Aug. 16, 2012, 4:04 p.m., Benjamin Hindman wrote: > > src/files/files.cpp, lines 157-158 > > <https://reviews.apache.org/r/6617/diff/1/?file=140338#file140338line157> > > > > Eventually it would be cool to have some abstractions for getting query > > key=value pairs out easily. Keep that in the back of your mind. ;) done!! The request.query is now a map of the query <k,v> pairs. Now this review is deleting a lot of code. Also, see the Option<V> get() method added to hashmap. - Ben ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/6617/#review10321 ----------------------------------------------------------- On Aug. 17, 2012, 4:15 a.m., Ben Mahler wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/6617/ > ----------------------------------------------------------- > > (Updated Aug. 17, 2012, 4:15 a.m.) > > > Review request for mesos, Benjamin Hindman, Vinod Kone, and Jie Yu. > > > Description > ------- > > Implementing the file abstraction and http endpoints for file reading / > browsing. > > > This addresses bug MESOS-255. > https://issues.apache.org/jira/browse/MESOS-255 > > > Diffs > ----- > > src/Makefile.am b0cb6cc > src/files/files.hpp d0cab91 > src/files/files.cpp d4080d4 > src/logging/logging.cpp 6909b0b > src/master/http.cpp e783ac3 > src/master/slaves_manager.cpp e25efd0 > src/slave/http.cpp a1f7926 > src/tests/configurator_tests.cpp c2f5aa0 > src/tests/files_tests.cpp PRE-CREATION > src/tests/utils.hpp caf5926 > src/webui/master/static/controllers.js 1606e64 > third_party/libprocess/include/process/http.hpp 8424ca6 > third_party/libprocess/include/stout/hashmap.hpp 51bdea0 > third_party/libprocess/include/stout/json.hpp 25dbcf4 > third_party/libprocess/include/stout/os.hpp b1eceb3 > third_party/libprocess/include/stout/path.hpp 80d9bc6 > third_party/libprocess/include/stout/stringify.hpp ad2f2fa > third_party/libprocess/src/decoder.hpp 105fe5d > third_party/libprocess/src/statistics.cpp d05b327 > > Diff: https://reviews.apache.org/r/6617/diff/ > > > Testing > ------- > > Added files_tests.cpp > make check > > > Thanks, > > Ben Mahler > >
