----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/47972/#review136482 -----------------------------------------------------------
3rdparty/stout/include/stout/os/posix/rmdir.hpp (line 39) <https://reviews.apache.org/r/47972/#comment201524> Quote ``continueOnError``. 3rdparty/stout/include/stout/os/posix/rmdir.hpp (lines 82 - 83) <https://reviews.apache.org/r/47972/#comment201527> 1. Wrap this within 80 chars. 2. s/WARNING/ERROR/ here and elsewhere. Sorry I didn't suggest this earlier but it is an error after all so let's log it as an error. 3rdparty/stout/include/stout/os/posix/rmdir.hpp (line 83) <https://reviews.apache.org/r/47972/#comment201526> 1. We usually use a `:` after `with error` to separate the nested error message from the enclosing error message, or actually `:` can replace `with error` because it's already clear it's a nested error message. 2. Use `os::strerror()`. 3rdparty/stout/include/stout/os/posix/rmdir.hpp (line 101) <https://reviews.apache.org/r/47972/#comment201528> Wrap this within 80 chars. It's helpful to run `./support/mesos-style.py` to check. :) 3rdparty/stout/include/stout/os/posix/rmdir.hpp (line 102) <https://reviews.apache.org/r/47972/#comment201529> Ditto to my comment above on a similar line. 3rdparty/stout/include/stout/os/posix/rmdir.hpp (lines 117 - 118) <https://reviews.apache.org/r/47972/#comment201530> 1. Two space indentation. 2. Just to explain the situation: Add a comment such as // If 'continueOnError' is true, we have already logged individual errors. 3. s/Error occurred during rmdir/rmdir failed in 'continueOnError' mode/ 3rdparty/stout/include/stout/os/windows/rmdir.hpp (lines 115 - 116) <https://reviews.apache.org/r/47972/#comment201536> 1. s/Error occurred during rmdir/rmdir failed in 'continueOnError' mode/ 2. This fits in one line. 3rdparty/stout/include/stout/os/windows/rmdir.hpp (line 120) <https://reviews.apache.org/r/47972/#comment201537> This looks to be over 80 chars too, let's fix all of them by running mesos-style.py. 3rdparty/stout/tests/os/rmdir_tests.cpp (line 309) <https://reviews.apache.org/r/47972/#comment201539> We initially used chattr but then we realized that AUFS doesn't support it, hence Mesos CI which runs on Docker+AUFS fails. We could achieve the same with a busy mount point and not suffer from this problem. Can we rewrite it? 3rdparty/stout/tests/os/rmdir_tests.cpp (line 311) <https://reviews.apache.org/r/47972/#comment201542> 1. Option<string> mountPoint; 2. It's a simple class, but let's still follow standard encapsulation practices and put this member in the private section. 3rdparty/stout/tests/os/rmdir_tests.cpp (line 314) <https://reviews.apache.org/r/47972/#comment201543> Check `mountPoint.isSome()`. 3rdparty/stout/tests/os/rmdir_tests.cpp (lines 315 - 316) <https://reviews.apache.org/r/47972/#comment201678> We don't need a temp variable here. 3rdparty/stout/tests/os/rmdir_tests.cpp (line 318) <https://reviews.apache.org/r/47972/#comment201545> Use a blank line to separate `}` and the next statement. 3rdparty/stout/tests/os/rmdir_tests.cpp (line 323) <https://reviews.apache.org/r/47972/#comment201540> Pull this up above the test fixture. 3rdparty/stout/tests/os/rmdir_tests.cpp (line 327) <https://reviews.apache.org/r/47972/#comment201677> "because it doesn't work with docker." is vague but as I suggested above, let's use the alternative mechanism to avoid this issue. 3rdparty/stout/tests/os/rmdir_tests.cpp (line 330) <https://reviews.apache.org/r/47972/#comment201652> You don't need the current path or the absolution path. We can just use the relative paths everywhere to simplify things. 3rdparty/stout/tests/os/rmdir_tests.cpp (line 334) <https://reviews.apache.org/r/47972/#comment201651> 1. We don't need to declare temp variables when you can inline them. 2. There is actually significance in the names so we should call it out. e.g., ``` const string directory = "directory"; // The busy mount point goes before the regular file in `rmdir`'s // directory traversal due to their names. This makes sure that // an error occurs before all deletable files are deleted. const string mountPoint = path::join(directory, "mount.point"); const string regularFile = path::join(directory, "regular.file"); ``` 3rdparty/stout/tests/os/rmdir_tests.cpp (line 341) <https://reviews.apache.org/r/47972/#comment201659> Assign to the member variable `mountPoint` after you have successfully mounted it because otherwise you don't need the extra step in `TearDown` to clean it up. 3rdparty/stout/tests/os/rmdir_tests.cpp (lines 348 - 350) <https://reviews.apache.org/r/47972/#comment201658> Here they should all be `ASSERT_SOME` because we should abort the test when any of them fail. 3rdparty/stout/tests/os/rmdir_tests.cpp (line 359) <https://reviews.apache.org/r/47972/#comment201680> Add comment: ``` // Run rmdir with 'continueOnError = true'. ``` 3rdparty/stout/tests/os/rmdir_tests.cpp (lines 361 - 363) <https://reviews.apache.org/r/47972/#comment201679> These are `EXPECT_*` as we want to run through all of them. 3rdparty/stout/tests/os/rmdir_tests.cpp (line 365) <https://reviews.apache.org/r/47972/#comment201681> No blank line here. - Jiang Yan Xu On May 27, 2016, 5:01 p.m., Megha Sharma wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/47972/ > ----------------------------------------------------------- > > (Updated May 27, 2016, 5:01 p.m.) > > > Review request for mesos and Jiang Yan Xu. > > > Bugs: MESOS-5196 > https://issues.apache.org/jira/browse/MESOS-5196 > > > Repository: mesos > > > Description > ------- > > https://issues.apache.org/jira/browse/MESOS-5196 > Updates made to rmdir to prevent early exit in the event of error. > > > Diffs > ----- > > 3rdparty/stout/include/stout/os/posix/rmdir.hpp > a6d3b578762d5c570542b0497578c330820b821a > 3rdparty/stout/include/stout/os/windows/rmdir.hpp > 772b86dd111d28aefbeeba5f829ffa377fd12efb > 3rdparty/stout/tests/os/rmdir_tests.cpp > a11bfc9f9e6cbb05f3e9ce0ea48297b8f88fe53f > > Diff: https://reviews.apache.org/r/47972/diff/ > > > Testing > ------- > > make check. > > Tested with option --gtest_also_run_disabled_tests. > > > Thanks, > > Megha Sharma > >
