mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Anand Mazumdar <an...@apache.org>
Subject Re: Review Request 52292: Updated nested container agent API to allow custom ContainerIDs.
Date Tue, 27 Sep 2016 01:20:06 GMT

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


Fix it, then Ship it!




LGTM, minus a few style nits + a query around not supporting multi level nesting.


src/slave/validation.cpp (line 17)
<https://reviews.apache.org/r/52292/#comment218484>

    Any reason for not using the cpp header `cctype`?



src/slave/validation.cpp (line 53)
<https://reviews.apache.org/r/52292/#comment218486>

    Use `os::PATH_SEPARATOR` for checking for directories instead?



src/slave/validation.cpp (line 158)
<https://reviews.apache.org/r/52292/#comment218488>

    Why did you remove this alias variable `launchNestedContainer`?



src/slave/validation.cpp (line 173)
<https://reviews.apache.org/r/52292/#comment218499>

    hmm, I thought we won't support more than one level of nesting only on the default executor.
But, custom executors should be free to use arbitrary level of nesting?



src/slave/validation.cpp (line 189)
<https://reviews.apache.org/r/52292/#comment218496>

    Might want to use a alias variable instead?



src/slave/validation.cpp (line 197)
<https://reviews.apache.org/r/52292/#comment218498>

    Should we also ensure that only one level of parent is set for the wait call similar to
the `LAUNCH` case? 
    
    Ditto for the `KILL` case.
    
    Provided we only want to support one level of nesting.



src/tests/slave_validation_tests.cpp (line 44)
<https://reviews.apache.org/r/52292/#comment218492>

    Can these sub tests be scoped into their own logic? Ditto for all the other similar tests
in this file. It's pretty easy for someone to modify something in these tests that can thereby
fail all the other assertions in this test.
    
    ```cpp
    {
      // No empty IDs.
    }
    
    {
      // No slashes.
    }
    ```



src/tests/slave_validation_tests.cpp (line 173)
<https://reviews.apache.org/r/52292/#comment218491>

    Not yours: As per my earlier comment, each of these should have been scoped into their
own block.


- Anand Mazumdar


On Sept. 27, 2016, 12:16 a.m., Benjamin Mahler wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52292/
> -----------------------------------------------------------
> 
> (Updated Sept. 27, 2016, 12:16 a.m.)
> 
> 
> Review request for mesos, Anand Mazumdar, Jie Yu, and Vinod Kone.
> 
> 
> Bugs: MESOS-6241
>     https://issues.apache.org/jira/browse/MESOS-6241
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> Previously, we required that ContainerID.value is a UUID. The
> intention here was that each container has the same format, and
> we could remove the need for ContainerID.parent since all
> ContainerIDs would be unique globally.
> 
> With this change, we intend to keep ContainerID.parent and allow
> users to specify custom IDs (e.g. <executor uuid>.redis.backup).
> This makes for more readable IDs, and we maintain globally unique
> ContainerIDs since the executor container has a UUID generated by
> the agent.
> 
> With this change, we now validate that the user supplied IDs are
> setting the parent component of the ID correctly, and that the
> value does not contain problematic characters.
> 
> 
> Diffs
> -----
> 
>   include/mesos/agent/agent.proto 3397c4177813252b777bbe328ff92c9428366b70 
>   include/mesos/v1/agent/agent.proto 93c817c0be92f68dea28a9272ac034386ade02af 
>   src/slave/validation.hpp 94286f08ee124753dbac394d9c358fca725be594 
>   src/slave/validation.cpp 62b273302845dfa2bc1c07d753dfcce122674809 
>   src/tests/api_tests.cpp e857b17cfe5f05d59859263c025564d33700a26c 
>   src/tests/slave_validation_tests.cpp ee59aee6d68b518d3263de47d3fe4cfbeb1ab477 
> 
> Diff: https://reviews.apache.org/r/52292/diff/
> 
> 
> Testing
> -------
> 
> Updated existing tests and added a ContainerID validation test.
> 
> 
> Thanks,
> 
> Benjamin Mahler
> 
>


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