mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Jiang Yan Xu <...@jxu.me>
Subject Re: Review Request 47972: Updated rmdir to continue deletion on error.
Date Wed, 08 Jun 2016 05:22:47 GMT

-----------------------------------------------------------
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
> 
>


Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message