----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/22722/#review46156 -----------------------------------------------------------
Ship it! Great! For this time, I will split this patch for you across stout and mesos. I left some notes below for some small cleanups I did before committing this. 3rdparty/libprocess/3rdparty/stout/tests/os/setns_tests.cpp <https://reviews.apache.org/r/22722/#comment81388> This should be an ASSERT because we want the test to stop if this condition is not satisfied. 3rdparty/libprocess/3rdparty/stout/tests/os_tests.cpp <https://reviews.apache.org/r/22722/#comment81384> This can be done in one line using EXPECT_SOME_EQ. 3rdparty/libprocess/3rdparty/stout/tests/os_tests.cpp <https://reviews.apache.org/r/22722/#comment81386> In general, let's avoid calling .get() on a Result without ensuring that it is SOME. src/cli/execute.cpp <https://reviews.apache.org/r/22722/#comment81389> No need for std:: src/sched/sched.cpp <https://reviews.apache.org/r/22722/#comment81390> We should have a CHECK_SOME here at least, that will print a CHECK failure with the error reason if things fail. src/scheduler/scheduler.cpp <https://reviews.apache.org/r/22722/#comment81391> Nice! I think for now we can keep the previous semantics and just use CHECK_SOME to ensure we can get the current user, as such a failure is quite serious. src/tests/environment.cpp <https://reviews.apache.org/r/22722/#comment81392> Nice! Let's store the Result<string> to avoid re-computing os::user() and potentially having .get() fail. src/tests/slave_tests.cpp <https://reviews.apache.org/r/22722/#comment81394> CHECK_SOME will print the error for you! :) - Ben Mahler On June 18, 2014, 10:56 a.m., Alexandra Sava wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/22722/ > ----------------------------------------------------------- > > (Updated June 18, 2014, 10:56 a.m.) > > > Review request for mesos and Ben Mahler. > > > Repository: mesos-git > > > Description > ------- > > Refactor os::user method: > * receive an optional uid argument for which the corresponding username will > be returned; > if no uid id provided, return the username of the current user > * change return type to Result<string> > * return None() and ErrnoError() on error cases rather then using LOG(FATAL) > which ends the application > > > Diffs > ----- > > 3rdparty/libprocess/3rdparty/stout/include/stout/os.hpp > 72f3e70ccc189d0a7dd613ccaf79537d3587c49c > 3rdparty/libprocess/3rdparty/stout/tests/os/setns_tests.cpp > f6d79b10b7cb0df3e510a3bf6e681726f5f019f3 > 3rdparty/libprocess/3rdparty/stout/tests/os_tests.cpp > 1a6ce0b9a70c0334946e456696b814961e5b4018 > src/cli/execute.cpp e881b64807f556b1c1774255beab95d60b6935d6 > src/sched/sched.cpp 6e14f1cf78b9518a04c35f856fa2358a3d72ddcf > src/scheduler/scheduler.cpp 4ae188e4eba67c630f57863b6c5116162c92a0a1 > src/tests/environment.cpp 005fc54759a4be1651f55249e4e8e1bafc2f73ce > src/tests/mesos.cpp 98a7c3895343b1bc84c13e54d12905de3b431338 > src/tests/script.cpp 9f1be63e2c6538a3ae3fd8f39ad0ea1f66ee88a4 > src/tests/slave_tests.cpp aaf509d1e0798928ab729704a42ec5aed18c11a2 > > Diff: https://reviews.apache.org/r/22722/diff/ > > > Testing > ------- > > > Thanks, > > Alexandra Sava > >
