mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Benjamin Mahler <bmah...@apache.org>
Subject Re: Review Request 57190: Updated agent for hierarchical roles.
Date Tue, 28 Mar 2017 00:35:27 GMT

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/57190/#review170243
-----------------------------------------------------------



Sorry for dropping the ball on this one. Some minor feedback below.

Can you add jie to this review? Would like to get his thoughts on the use of spaces.


src/slave/paths.cpp
Line 451 (original), 451-452 (patched)
<https://reviews.apache.org/r/57190/#comment243037>

    It would be great to expand on why we do this. Also, on how we arrived on using spaces
(i.e. as it pertains to the allowed characters within roles, whether space is valid on multiple
filesystems / OS's, etc).



src/tests/role_tests.cpp
Lines 788-790 (patched)
<https://reviews.apache.org/r/57190/#comment243038>

    This seems a little obscure to me, how about clarifying the problem (i.e. related to the
directories and the use of slash, which is why we have the workaround with space characters).
That should give enough context for someone to understand why we would be testing this.



src/tests/role_tests.cpp
Lines 814-815 (patched)
<https://reviews.apache.org/r/57190/#comment243040>

    s/creating/and creates/ ?



src/tests/role_tests.cpp
Lines 814-817 (patched)
<https://reviews.apache.org/r/57190/#comment243045>

    Just a side thought, we could achieve a lot of cleanup in the tests if we were to have
some scheduler implementations we can leverage. One common need is for a scheduler that runs
a command and expects the task to succeed, like this one.
    
    This could be acheived with something like:
    
    ```
    Future<StatusUpdateStream> runTask(...);
    ```
    
    What's different here is we need a reservation and volume, but that could be done before
calling `runTask` using the endpoints.
    
    Just some food for thought to simplify the test code.



src/tests/role_tests.cpp
Lines 873 (patched)
<https://reviews.apache.org/r/57190/#comment243044>

    Can you add a comment describing what this command is doing?



src/tests/role_tests.cpp
Lines 901-902 (patched)
<https://reviews.apache.org/r/57190/#comment243042>

    How about returning the response up here and asserting below? I.e.
    
    ```
    auto destroyVolume = ... {
      ...
      
      return destroyVolumesResponse;
    };
    
    Future<http::Response> response = destroyVolume("a/b/c", "d");
    AWAIT_EXPECT_RESPONSE_CODE_EQ(Accepted().status, response);
    ```
    
    That way, it's just a function that tries to do something and returns the result, and
it's easier to tell which invocation of the lambda is failing because we'll have the line
number indicating which result failed.



src/tests/role_tests.cpp
Lines 943 (patched)
<https://reviews.apache.org/r/57190/#comment243041>

    We could avoid the future chaining and instead just have an expectation directly on the
result:
    
    ```
    AWAIT_EXPECT_RESPONSE_STATUS_EQ(Accepted().status, response);
    ```
    
    That would remove the need for the continuation.



src/tests/role_tests.cpp
Lines 946-949 (patched)
<https://reviews.apache.org/r/57190/#comment243043>

    Ah here is what I was looking for, this comment is helpful! Could we have a comment like
this at the top of the test to give the context?


- Benjamin Mahler


On March 9, 2017, 11:14 p.m., Benjamin Bannier wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/57190/
> -----------------------------------------------------------
> 
> (Updated March 9, 2017, 11:14 p.m.)
> 
> 
> Review request for mesos, Benjamin Mahler, Michael Park, and Neil Conway.
> 
> 
> Bugs: MESOS-7047
>     https://issues.apache.org/jira/browse/MESOS-7047
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> This commit adjusts the way persistent volumes are stored on the
> agent. Instead of interpreting the role of the volume as a literal
> path, we replace `/` with ` ` when creating the path. This prevents
> that subdirectories are created for volumes with hierarchical roles.
> Directly interpreting the role as a path is undesirable as it can lead
> to volumes overlapping (e.g., a volume with role `a/b/c/d` and id `id`
> would be visible as `id` in a volume with role `a/b/c` and id `d`).
> 
> 
> Diffs
> -----
> 
>   src/slave/paths.cpp 38ad1993aa36a627ec97a7865488677495ee4c5a 
>   src/tests/role_tests.cpp 77f3d46a544a51ba71476e2f0735bb32758dd9e1 
> 
> 
> Diff: https://reviews.apache.org/r/57190/diff/4/
> 
> 
> Testing
> -------
> 
> make check
> 
> 
> Thanks,
> 
> Benjamin Bannier
> 
>


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