----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/6617/#review10321 -----------------------------------------------------------
src/files/files.hpp <https://reviews.apache.org/r/6617/#comment21882> Alphabetize please. src/files/files.hpp <https://reviews.apache.org/r/6617/#comment21883> Space in between 'process' and 'stout' headers please. src/files/files.hpp <https://reviews.apache.org/r/6617/#comment22154> I'd prefer not to expose these just for tests. Here are my suggestions: (1) Put the FilesProcess class declaration in files/process.hpp and have files/files.cpp include files/files.hpp and files/process.hpp and put all the implementations there. Then you can directly test a FilesProcess via dispatches. The downside here is a little less opportunity for the compiler to optimize and slightly more verbose tests (need to do spawn, dispatch, wait, terminate). (2) Add an abstraction for "submitting" an http::Request. Since a FilesProcess is currently always at a well known id (i.e., "files") you can easily create a Request for it (subsequent abstractions may require exposing their PID so that we can know where to send requests). This will be INCREDIBLY useful for testing lots of different current and future HTTP endpoints. src/files/files.cpp <https://reviews.apache.org/r/6617/#comment22155> We've been including C headers first, then C++, then our own (i.e., those with "" not <>). src/files/files.cpp <https://reviews.apache.org/r/6617/#comment22158> No longer need this! ;) src/files/files.cpp <https://reviews.apache.org/r/6617/#comment22156> I'm guessing this was for access to browse and read. If you decided to do (1) mentioned above just make browse and read be public. So kill this regardless. src/files/files.cpp <https://reviews.apache.org/r/6617/#comment22157> 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). src/files/files.cpp <https://reviews.apache.org/r/6617/#comment22159> 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. ;) src/files/files.cpp <https://reviews.apache.org/r/6617/#comment22160> This should be a BadRequest("Expecting 'dir=value' in query.\n"). src/files/files.cpp <https://reviews.apache.org/r/6617/#comment22162> Only return NotFound if the file actually wasn't found. Have resolve return a Result instead of a Try. src/files/files.cpp <https://reviews.apache.org/r/6617/#comment22163> Could just s/files/os::ls(path.get())/ src/files/files.cpp <https://reviews.apache.org/r/6617/#comment22165> 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. src/files/files.cpp <https://reviews.apache.org/r/6617/#comment22164> Some sort of log warning would be nice. src/files/files.cpp <https://reviews.apache.org/r/6617/#comment22166> Kill. src/files/files.cpp <https://reviews.apache.org/r/6617/#comment22167> file.values["dir"] = S_ISDIR(s.st_mode) ? JSON::True() : JSON::False(); src/files/files.cpp <https://reviews.apache.org/r/6617/#comment22168> 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=... src/files/files.cpp <https://reviews.apache.org/r/6617/#comment22169> BadRequest. src/files/files.cpp <https://reviews.apache.org/r/6617/#comment22161> You should return a Result here. That way you can actually distinguish between NotFound and InternalServerError. src/files/files.cpp <https://reviews.apache.org/r/6617/#comment22171> Try<string> result => Result<string> result ... how fitting. src/files/files.cpp <https://reviews.apache.org/r/6617/#comment22172> Yes, my bad. Please delete. src/files/files.cpp <https://reviews.apache.org/r/6617/#comment22173> It can not, because then we'd have to expose FilesProcess to/in files.hpp. src/tests/configurator_tests.cpp <https://reviews.apache.org/r/6617/#comment22174> 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. src/tests/configurator_tests.cpp <https://reviews.apache.org/r/6617/#comment22175> There is also a "write" in stout/os.hpp. Try<bool> result = os::write("conf2/mesos.conf", ....); result.isSome() || FAIL() << "" << result.error(); src/tests/files_tests.cpp <https://reviews.apache.org/r/6617/#comment22178> 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. src/tests/files_tests.cpp <https://reviews.apache.org/r/6617/#comment22176> Newline. src/tests/files_tests.cpp <https://reviews.apache.org/r/6617/#comment22177> Two newlines between tests please (here and rest of file). src/tests/files_tests.cpp <https://reviews.apache.org/r/6617/#comment22181> You could throw this in a header and use it in files.cpp as well as here. src/tests/files_tests.cpp <https://reviews.apache.org/r/6617/#comment22180> 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! third_party/libprocess/include/stout/path.hpp <https://reviews.apache.org/r/6617/#comment22182> & third_party/libprocess/include/stout/path.hpp <https://reviews.apache.org/r/6617/#comment22183> s/ret/result/ - Benjamin Hindman On Aug. 15, 2012, 2:48 a.m., Ben Mahler wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/6617/ > ----------------------------------------------------------- > > (Updated Aug. 15, 2012, 2:48 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/tests/configurator_tests.cpp c2f5aa0 > src/tests/files_tests.cpp PRE-CREATION > src/tests/utils.hpp caf5926 > third_party/libprocess/include/stout/json.hpp 25dbcf4 > third_party/libprocess/include/stout/path.hpp 80d9bc6 > > Diff: https://reviews.apache.org/r/6617/diff/ > > > Testing > ------- > > Added files_tests.cpp > make check > > > Thanks, > > Ben Mahler > >
