----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/6617/#review11205 -----------------------------------------------------------
src/files/files.hpp <https://reviews.apache.org/r/6617/#comment24002> How come you made this virtual? Do you plan on extending Files? src/files/files.hpp <https://reviews.apache.org/r/6617/#comment24003> Can you move nothing.hpp into stout please? src/files/files.hpp <https://reviews.apache.org/r/6617/#comment24005> s/upid/pid/g In the future, we'll eliminate UPID and just use PID<> when we don't know the type and PID<T> when we do. src/files/files.hpp <https://reviews.apache.org/r/6617/#comment24014> s/the/our/ s/Repre/repre/ Also, this is more the "file status" representation (i.e., it doesn't include the actual file content/bytes), so perhaps a more descriptive name? src/files/files.hpp <https://reviews.apache.org/r/6617/#comment24015> Is this really necessary? Can't one just get this via: basename(path)? In which case, you can simplify this function to just take one 'string path' argument instead of 'string dir' and 'string name'. Plus, 'name' is starting to get confusing to me ... i.e., is this the mapped 'name' from Files::attach? src/files/files.cpp <https://reviews.apache.org/r/6617/#comment24016> What's your thinking for making the HTTP endpoint functions public? src/files/files.cpp <https://reviews.apache.org/r/6617/#comment24017> Exposing R_OK, W_OK, and X_OK might be preferred since then you know at the call site what's being checked (without having to go look at the declaration of os::access), e.g.: os::access(result.get(), R_OK); src/files/files.cpp <https://reviews.apache.org/r/6617/#comment24018> I generally really like using the 'verb' name (or some variant of it) as the variable name, e.g., access from os::access (or accessible from os::access). Makes for really readable code! Since we stick all of our functions behind namespaces (like 'os'), this is usually always possible. Yeah! src/files/files.cpp <https://reviews.apache.org/r/6617/#comment24020> I'd prefer to keep the variable names equal to the actual query key, so s/name/path/. src/files/files.cpp <https://reviews.apache.org/r/6617/#comment24019> s/directory/path/ Also, this comment is pretty redundant given the readability of the next line. src/files/files.cpp <https://reviews.apache.org/r/6617/#comment24021> Then this can be s/path/resolvedPath/ or something of the sort. src/files/files.cpp <https://reviews.apache.org/r/6617/#comment24022> Any reason not to use a JSON::Array to start? Are you concerned about duplicates? I don't think you can get duplicates. Or are you using the map to get sort ordering? Why is the sort ordering important? src/files/files.cpp <https://reviews.apache.org/r/6617/#comment24012> Since PLOG will print the "reason" at the end, let's embed the file name in the message, i.e., PLOG(WARNING) << "Found " << fullPath << " in ls but stat failed"; Also, let's use full path (more info the better for log lines). src/files/files.cpp <https://reviews.apache.org/r/6617/#comment24023> I think having os::isdir is great, but you might as well not do the extra stat call here and use S_ISDIR (everywhere else it makes sense to just do os::isdir though). src/files/files.cpp <https://reviews.apache.org/r/6617/#comment24024> Ditto as comment above. src/files/files.cpp <https://reviews.apache.org/r/6617/#comment24025> Ditto above. src/files/files.cpp <https://reviews.apache.org/r/6617/#comment24026> BadRequest? src/files/files.cpp <https://reviews.apache.org/r/6617/#comment24027> One of the nice things about exposing the 'query' hashmap like you did means you can write stuff like this: if (request.query.get("offset").isSome()) { Try<off_t> result = numify<off_t>(request.query.get("offset").get()); ...; offset = result.get(); } I tend to like that style better because then you don't have to introduce as many temporary variables. src/files/files.cpp <https://reviews.apache.org/r/6617/#comment24029> Don't need to invoke .c_str()! Also, love to hear your thoughts on eliminating the Try return type of strings::format ... read: I'm not a fan of calling .get() on a Try without checking first. src/files/files.cpp <https://reviews.apache.org/r/6617/#comment24028> Yes, we should totally inlude the error, but this should probably be a BadRequest. src/files/files.cpp <https://reviews.apache.org/r/6617/#comment24030> I think the numify error output is actually sufficient here, so you can just do (here and above): return BadRequest("Failed to numify 'length': " + result.error()); Or if you want to log it too: string error = "Failed to numify 'length': " + result.error(); LOG(ERROR) << error; return BadRequest(error); Also, I had put numify in quotes before, not sure if you want to do that or not (or use a different word like parse)? I thought numify was a word I made up. ;) src/files/files.cpp <https://reviews.apache.org/r/6617/#comment24031> Is this an internal server error or a bad request? src/files/files.cpp <https://reviews.apache.org/r/6617/#comment24034> Again, shouldn't need c_str. src/files/files.cpp <https://reviews.apache.org/r/6617/#comment24035> Add a TODO to do this via the async process::read please. src/files/files.cpp <https://reviews.apache.org/r/6617/#comment24036> Oh come on, 'key' and 'value' aren't that much harder to type than k and v! ;) That or 'name' and 'path' since that maps well to the attach nomenclature. src/files/files.cpp <https://reviews.apache.org/r/6617/#comment24037> Can you include a comment explaining what a "some" versus "none" versus "error" result means? src/files/files.cpp <https://reviews.apache.org/r/6617/#comment24038> I'd move this just above the while loop (it's kind of hidden now). src/files/files.cpp <https://reviews.apache.org/r/6617/#comment24041> See comment below about chroot, possibly changing wording here too. src/files/files.cpp <https://reviews.apache.org/r/6617/#comment24039> if (!strings::startsWith(result.get(), paths[prefix])) { src/files/files.cpp <https://reviews.apache.org/r/6617/#comment24040> Maybe "Resolved path is inaccessible" rather than "breaks out of chroot", since the latter is a bit more esoteric to understand in this circumstance. src/local/main.cpp <https://reviews.apache.org/r/6617/#comment24042> Do the other main.cpp files not have a newline in between these calls? src/logging/logging.cpp <https://reviews.apache.org/r/6617/#comment24043> Brilliant! src/master/http.cpp <https://reviews.apache.org/r/6617/#comment24044> Any reason not to jsonp it? src/master/master.hpp <https://reviews.apache.org/r/6617/#comment24045> const & src/master/master.cpp <https://reviews.apache.org/r/6617/#comment24046> You can actually pull this to the top of the function as a CHECK(!result.isDiscarded); src/master/slaves_manager.cpp <https://reviews.apache.org/r/6617/#comment24047> If we had a version of numify like so: ----------------- template <typename T> Result<T> numify(const Option<std::string>& option) { if (option.isSome()) { return numify(option.get()); // Assumes we add a Result(const Try& t) constructor. } return Result<T>::none(); } ----------------- Then we could make this code even simpler: ----------------- Option<string> hostname = request.query.get("hostname"); Result<uint16_t> port = numify<uint16_t>(request.query.get("port")); if (hostname.isNone()) { ...; } if (port.isNone()) { ...; } else if (port.isError()) { ...; } ... hostname.get() ... port.get() ...; ----------------- You don't have to worry about this now if you don't want ... src/slave/http.cpp <https://reviews.apache.org/r/6617/#comment24048> Pass jsonp? src/slave/slave.hpp <https://reviews.apache.org/r/6617/#comment24049> const & src/slave/slave.cpp <https://reviews.apache.org/r/6617/#comment24050> Same comment about CHECK as in master.cpp. src/tests/files_tests.cpp <https://reviews.apache.org/r/6617/#comment24051> Please add to json.hpp an operator << (ostream, ...) overload and then use stringify everywhere you call jstonToString. Thanks! src/tests/files_tests.cpp <https://reviews.apache.org/r/6617/#comment24052> const & src/tests/files_tests.cpp <https://reviews.apache.org/r/6617/#comment24053> Ditto. src/tests/files_tests.cpp <https://reviews.apache.org/r/6617/#comment24054> Ditto. src/tests/files_tests.cpp <https://reviews.apache.org/r/6617/#comment24055> Okay, these last three functions can be generally useful as macros. For the first one: ASSERT_FUTURE_WILL_SUCCEED EXPECT_FUTURE_WILL_SUCCEED Note that I use "will" to capture that this is doing an await as well. You could also imagine: *_FUTURE_WILL_READY Or: *_FUTURE_AWAIT_READY Or something similar. For the second one: ASSERT_FUTURE_WILL_FAIL EXPECT_FUTURE_WILL_FAIL For the third one: ASSERT_FUTURE_WILL_EQ EXPECT_FUTURE_WILL_EQ Then for any that are specific to a type (e.g., checkStatus), I still think it would be great to see shareable macros along the lines of: ASSERT_RESPONSE_STATUS_WILL_EQ(OK().status, process::http::get(...)); You can stick these in src/tests/utils.hpp. There are lots of other tests that will be able to take advantage of these. That will be awesome! src/tests/files_tests.cpp <https://reviews.apache.org/r/6617/#comment24056> s/upid/pid/ src/tests/files_tests.cpp <https://reviews.apache.org/r/6617/#comment24057> s/upid/pid/ third_party/libprocess/include/process/http.hpp <https://reviews.apache.org/r/6617/#comment24058> Stick this inside the "query" namespace and just call the function "parse" please. third_party/libprocess/include/process/http.hpp <https://reviews.apache.org/r/6617/#comment24059> After doing above, this will probably all fit on one line. third_party/libprocess/include/process/http.hpp <https://reviews.apache.org/r/6617/#comment24061> s/upid/pid/ third_party/libprocess/include/process/http.hpp <https://reviews.apache.org/r/6617/#comment24071> & third_party/libprocess/include/process/http.hpp <https://reviews.apache.org/r/6617/#comment24060> '{' on newline please. third_party/libprocess/include/stout/os.hpp <https://reviews.apache.org/r/6617/#comment24062> See comment at call site. third_party/libprocess/src/decoder.hpp <https://reviews.apache.org/r/6617/#comment24063> Please add TODO: Make DataDecoder abstract (pure-virtual) class and make RequestDecoder be a concrete subclass. third_party/libprocess/src/process.cpp <https://reviews.apache.org/r/6617/#comment24064> YES! third_party/libprocess/src/process.cpp <https://reviews.apache.org/r/6617/#comment24065> Awesome! third_party/libprocess/src/process.cpp <https://reviews.apache.org/r/6617/#comment24067> Please add the following at the end of this line: // TODO(benh): Remove 'const &' after fixing libprocess. third_party/libprocess/src/process.cpp <https://reviews.apache.org/r/6617/#comment24068> If the function + args fits on _newline_, please keep them together. third_party/libprocess/src/process.cpp <https://reviews.apache.org/r/6617/#comment24069> s/copy/response/ third_party/libprocess/src/process.cpp <https://reviews.apache.org/r/6617/#comment24070> s/upid/pid/ and const & on string parameters. third_party/libprocess/src/process.cpp <https://reviews.apache.org/r/6617/#comment24072> ;) third_party/libprocess/src/process.cpp <https://reviews.apache.org/r/6617/#comment24073> Unused? third_party/libprocess/src/statistics.cpp <https://reviews.apache.org/r/6617/#comment24074> Another place to use numify(option)! - Benjamin Hindman On Sept. 7, 2012, 10:49 p.m., Ben Mahler wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/6617/ > ----------------------------------------------------------- > > (Updated Sept. 7, 2012, 10:49 p.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 19bd20d > src/common/attributes.cpp 66d0b70 > src/common/resources.cpp 2cee743 > src/common/values.cpp ec341be > src/configurator/configuration.hpp e2cd1b5 > src/configurator/configurator.cpp 3916427 > src/files/files.hpp d0cab91 > src/files/files.cpp d4080d4 > src/launcher/main.cpp 06597e6 > src/linux/cgroups.cpp 9cedea5 > src/local/local.cpp 9c1c5b6 > src/local/main.cpp ecab6b8 > src/logging/logging.cpp 6909b0b > src/master/http.cpp c480bc6 > src/master/main.cpp 48859f1 > src/master/master.hpp 866f3ef > src/master/master.cpp ab516ec > src/master/slaves_manager.cpp e25efd0 > src/slave/http.cpp a1f7926 > src/slave/main.cpp f1aade2 > src/slave/slave.hpp b7ab2ab > src/slave/slave.cpp ced232d > src/tests/allocator_tests.cpp 80ba39d > src/tests/allocator_zookeeper_tests.cpp c4956dc > src/tests/configurator_tests.cpp c2f5aa0 > src/tests/fault_tolerance_tests.cpp 0e88701 > src/tests/files_tests.cpp PRE-CREATION > src/tests/gc_tests.cpp 66bdbb2 > src/tests/master_detector_tests.cpp 5b2ab4f > src/tests/master_tests.cpp c77a4ed > src/tests/resource_offers_tests.cpp 1d7d9ed > src/tests/stout_tests.cpp 0bc60a9 > src/tests/utils.hpp 54da799 > src/webui/master/static/controllers.js 1606e64 > third_party/libprocess/Makefile.am f898469 > third_party/libprocess/include/process/http.hpp 8424ca6 > third_party/libprocess/include/process/io.hpp 6a40b18 > third_party/libprocess/include/stout/hashmap.hpp 51bdea0 > third_party/libprocess/include/stout/json.hpp 25dbcf4 > third_party/libprocess/include/stout/os.hpp df0f7ff > third_party/libprocess/include/stout/path.hpp 80d9bc6 > third_party/libprocess/include/stout/stringify.hpp ad2f2fa > third_party/libprocess/include/stout/strings.hpp 0646bf9 > third_party/libprocess/src/decoder.hpp 105fe5d > third_party/libprocess/src/encoder.hpp 55b5d50 > third_party/libprocess/src/process.cpp 2b2d521 > third_party/libprocess/src/statistics.cpp d05b327 > third_party/libprocess/src/tokenize.hpp f886186 > third_party/libprocess/src/tokenize.cpp 759ce5f > > Diff: https://reviews.apache.org/r/6617/diff/ > > > Testing > ------- > > Added files_tests.cpp > make check > > > Thanks, > > Ben Mahler > >
