> On Sept. 8, 2012, 4:45 p.m., Benjamin Hindman wrote: > >
thanks for the thorough review! > On Sept. 8, 2012, 4:45 p.m., Benjamin Hindman wrote: > > src/files/files.hpp, line 49 > > <https://reviews.apache.org/r/6617/diff/9/?file=151409#file151409line49> > > > > How come you made this virtual? Do you plan on extending Files? reverted > On Sept. 8, 2012, 4:45 p.m., Benjamin Hindman wrote: > > src/files/files.hpp, lines 60-62 > > <https://reviews.apache.org/r/6617/diff/9/?file=151409#file151409line60> > > > > 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. in this case I used PID<> since I don't want to expose FilesProcess, just double checking that's what you wanted? > On Sept. 8, 2012, 4:45 p.m., Benjamin Hindman wrote: > > src/files/files.hpp, line 69 > > <https://reviews.apache.org/r/6617/diff/9/?file=151409#file151409line69> > > > > 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? changed to jsonFileInfo > On Sept. 8, 2012, 4:45 p.m., Benjamin Hindman wrote: > > src/files/files.cpp, line 59 > > <https://reviews.apache.org/r/6617/diff/9/?file=151410#file151410line59> > > > > What's your thinking for making the HTTP endpoint functions public? artifact of pre http::get code, reverted > On Sept. 8, 2012, 4:45 p.m., Benjamin Hindman wrote: > > src/files/files.hpp, line 76 > > <https://reviews.apache.org/r/6617/diff/9/?file=151409#file151409line76> > > > > 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? Did some renaming around name to instead use: path -> parameter resolvedPath -> actual fs path > On Sept. 8, 2012, 4:45 p.m., Benjamin Hindman wrote: > > src/files/files.cpp, line 100 > > <https://reviews.apache.org/r/6617/diff/9/?file=151410#file151410line100> > > > > 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); > On Sept. 8, 2012, 4:45 p.m., Benjamin Hindman wrote: > > src/files/files.cpp, line 129 > > <https://reviews.apache.org/r/6617/diff/9/?file=151410#file151410line129> > > > > I'd prefer to keep the variable names equal to the actual query key, so > > s/name/path/. so along with some other changes, I removed name in favor of path (and resolvedPath for the actual fs path) > On Sept. 8, 2012, 4:45 p.m., Benjamin Hindman wrote: > > src/files/files.cpp, line 136 > > <https://reviews.apache.org/r/6617/diff/9/?file=151410#file151410line136> > > > > Then this can be s/path/resolvedPath/ or something of the sort. heh, already done! > On Sept. 8, 2012, 4:45 p.m., Benjamin Hindman wrote: > > src/files/files.cpp, line 146 > > <https://reviews.apache.org/r/6617/diff/9/?file=151410#file151410line146> > > > > 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? the latter so this initially used just the array, but when I went and tested on CentOS, the output ordering was different, so the sort is for my tests unfortunately we don't have string -> JSON or JSON equality so this was the best I could do atm, I had made a ticket for this: https://issues.apache.org/jira/browse/MESOS-269 > On Sept. 8, 2012, 4:45 p.m., Benjamin Hindman wrote: > > src/files/files.cpp, line 152 > > <https://reviews.apache.org/r/6617/diff/9/?file=151410#file151410line152> > > > > 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). already done! > On Sept. 8, 2012, 4:45 p.m., Benjamin Hindman wrote: > > src/files/files.cpp, line 156 > > <https://reviews.apache.org/r/6617/diff/9/?file=151410#file151410line156> > > > > 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). was originally doing that, so reverted :) > On Sept. 8, 2012, 4:45 p.m., Benjamin Hindman wrote: > > src/files/files.cpp, line 186 > > <https://reviews.apache.org/r/6617/diff/9/?file=151410#file151410line186> > > > > 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. neither am I is this the *only* reason format can fail: return Try<std::string>::error( "Failed to format '" + fmt + "' (possibly out of memory)"); if so, I don't see why we can't kill that, given we'd die from OOM anyway? > On Sept. 8, 2012, 4:45 p.m., Benjamin Hindman wrote: > > src/files/files.cpp, line 188 > > <https://reviews.apache.org/r/6617/diff/9/?file=151410#file151410line188> > > > > Yes, we should totally inlude the error, but this should probably be a > > BadRequest. wow I was all over the place with these! > On Sept. 8, 2012, 4:45 p.m., Benjamin Hindman wrote: > > src/files/files.cpp, line 200 > > <https://reviews.apache.org/r/6617/diff/9/?file=151410#file151410line200> > > > > 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. ;) much nicer! > On Sept. 8, 2012, 4:45 p.m., Benjamin Hindman wrote: > > src/files/files.cpp, line 327 > > <https://reviews.apache.org/r/6617/diff/9/?file=151410#file151410line327> > > > > 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. Devil's advocate: Oh come on, "index" isn't that much harder to type than i! ;) k and v are like i and j IMO (I bet we disagree here), but changed to name and path since I like that better! > On Sept. 8, 2012, 4:45 p.m., Benjamin Hindman wrote: > > src/files/files.cpp, line 335 > > <https://reviews.apache.org/r/6617/diff/9/?file=151410#file151410line335> > > > > Can you include a comment explaining what a "some" versus "none" versus > > "error" result means? done, added above where it's prototype is > On Sept. 8, 2012, 4:45 p.m., Benjamin Hindman wrote: > > src/master/master.cpp, line 533 > > <https://reviews.apache.org/r/6617/diff/9/?file=151419#file151419line533> > > > > You can actually pull this to the top of the function as a > > CHECK(!result.isDiscarded); ok I didn't really know the semantics of being discarded, and didn't expect that to be CHECK worthy (killing the program) > On Sept. 8, 2012, 4:45 p.m., Benjamin Hindman wrote: > > src/master/slaves_manager.cpp, line 816 > > <https://reviews.apache.org/r/6617/diff/9/?file=151420#file151420line816> > > > > 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 ... I'm gonna punt on even more refactoring in this CL, added a TODO > On Sept. 8, 2012, 4:45 p.m., Benjamin Hindman wrote: > > third_party/libprocess/include/stout/os.hpp, line 89 > > <https://reviews.apache.org/r/6617/diff/9/?file=151442#file151442line89> > > > > See comment at call site. fixed > On Sept. 8, 2012, 4:45 p.m., Benjamin Hindman wrote: > > third_party/libprocess/src/process.cpp, lines 3197-3198 > > <https://reviews.apache.org/r/6617/diff/9/?file=151448#file151448line3197> > > > > If the function + args fits on _newline_, please keep them together. I might be guilty of this in other places in this review, aware of this now :) > On Sept. 8, 2012, 4:45 p.m., Benjamin Hindman wrote: > > third_party/libprocess/src/process.cpp, line 3144 > > <https://reviews.apache.org/r/6617/diff/9/?file=151448#file151448line3144> > > > > Please add the following at the end of this line: > > > > // TODO(benh): Remove 'const &' after fixing libprocess. done, lost me on this one..? > On Sept. 8, 2012, 4:45 p.m., Benjamin Hindman wrote: > > src/tests/files_tests.cpp, line 76 > > <https://reviews.apache.org/r/6617/diff/9/?file=151429#file151429line76> > > > > 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! done, but I made them functions rather than macros, since I operate on the future multiple times (and that doesn't play well with macros) - Ben ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/6617/#review11205 ----------------------------------------------------------- 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 > >
