> On Aug. 11, 2015, 12:19 a.m., Ben Mahler wrote: > > 3rdparty/libprocess/src/tests/http_tests.cpp, line 451 > > <https://reviews.apache.org/r/37240/diff/1/?file=1035018#file1035018line451> > > > > Why the semi colon?
oops. fixed. > On Aug. 11, 2015, 12:19 a.m., Ben Mahler wrote: > > 3rdparty/libprocess/src/tests/http_tests.cpp, line 455 > > <https://reviews.apache.org/r/37240/diff/1/?file=1035018#file1035018line455> > > > > Maybe this is a bit more explicit about which statuses you're expecting: > > > > ``` > > ASSERT_EQ(http::OK().status, ...); > > ``` > > > > Ditto for 202? i just did based on how it's done in other tests in the file. i'll add TODO to fix it for all tests. > On Aug. 11, 2015, 12:19 a.m., Ben Mahler wrote: > > 3rdparty/libprocess/src/process.cpp, lines 3065-3066 > > <https://reviews.apache.org/r/37240/diff/1/?file=1035017#file1035017line3065> > > > > Mind adding a newline for readability? done > On Aug. 11, 2015, 12:19 a.m., Ben Mahler wrote: > > 3rdparty/libprocess/src/process.cpp, line 3040 > > <https://reviews.apache.org/r/37240/diff/1/?file=1035017#file1035017line3040> > > > > Maybe pull this up outside of the loop? Not sure what value there is in > > logging each step, also VLOG(1) seems too verbose for this kind of thing? i just used it for debugging. will kill it. > On Aug. 11, 2015, 12:19 a.m., Ben Mahler wrote: > > 3rdparty/libprocess/src/process.cpp, lines 3029-3030 > > <https://reviews.apache.org/r/37240/diff/1/?file=1035017#file1035017line3029> > > > > Seems a little odd to have this comment up here and then a similar > > comment above the while loop, can you combine them into one? Either a > > bigger one here that describes how we look, or just keep the one above the > > loop. done. > On Aug. 11, 2015, 12:19 a.m., Ben Mahler wrote: > > 3rdparty/libprocess/src/process.cpp, line 3039 > > <https://reviews.apache.org/r/37240/diff/1/?file=1035017#file1035017line3039> > > > > The following seems a bit less hacky than hardcoding "" and "." to know > > when we're done? > > > > ``` > > while (Path(name).dirname() != name) { > > > > } > > ``` > > > > This also avoids relying on "/" being trimmed (since you're not > > checking for name == "/" we must have that trim code in place). sgtm. fixed. > On Aug. 11, 2015, 12:19 a.m., Ben Mahler wrote: > > 3rdparty/libprocess/include/process/http.hpp, line 112 > > <https://reviews.apache.org/r/37240/diff/1/?file=1035016#file1035016line112> > > > > Hm.. is the http path similar enough to POSIX paths? FWICT, yes. - Vinod ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/37240/#review94827 ----------------------------------------------------------- On Aug. 7, 2015, 11:39 p.m., Vinod Kone wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/37240/ > ----------------------------------------------------------- > > (Updated Aug. 7, 2015, 11:39 p.m.) > > > Review request for mesos, Anand Mazumdar, Benjamin Hindman, and Ben Mahler. > > > Bugs: MESOS-3237 > https://issues.apache.org/jira/browse/MESOS-3237 > > > Repository: mesos > > > Description > ------- > > Needed this for scheduler HTTP API to handle "/api/v1/scheduler" > > > Diffs > ----- > > 3rdparty/libprocess/include/process/http.hpp > f24ca24f4b2926d6d9651b90bdf4dd8156f71c9f > 3rdparty/libprocess/src/process.cpp > 2ce547bd3dc13841cc6ea2537df1398acdb1edef > 3rdparty/libprocess/src/tests/http_tests.cpp > ecbcbd552ac834659860627c82628ed38e6139b3 > > Diff: https://reviews.apache.org/r/37240/diff/ > > > Testing > ------- > > make check > > > Thanks, > > Vinod Kone > >
