mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Jie Yu" <yujie....@gmail.com>
Subject Re: Review Request 37881: Implemented AppcProvisioner.
Date Tue, 01 Sep 2015 05:33:05 GMT

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

Ship it!


Overall LGTM! Please do all more tests later. Some of them can be unit tested (e.g., in appc::paths).


src/slave/containerizer/provisioner.cpp (line 47)
<https://reviews.apache.org/r/37881/#comment153063>

    Can you just use a string->create func hashmap so that you don't need Image::Type_Parse.
    
    ```
    hashmap<string, ...> creators = {
      {"appc", &appc::AppcProvisioner::create}
    }
    ```



src/slave/containerizer/provisioners/appc.cpp (line 117)
<https://reviews.apache.org/r/37881/#comment153064>

    kill this line.



src/slave/containerizer/provisioners/appc.cpp (lines 180 - 187)
<https://reviews.apache.org/r/37881/#comment153070>

    AppcProvisioner is a wrapper of AppcProvisionerProcess. In our code base, we delegate
all calls to the underlying process. There're several benefits of this. One benefit is that
all logics are in the same function (not splitted like this case).
    
    I still suggest that you move those checks to AppcProvisionerProcess::provision.



src/slave/containerizer/provisioners/appc.cpp (line 228)
<https://reviews.apache.org/r/37881/#comment153080>

    What about those containers that does not specify a container info but we filled in with
default container info with appc image? What should we do?



src/slave/containerizer/provisioners/appc.cpp (line 248)
<https://reviews.apache.org/r/37881/#comment153082>

    no container has



src/slave/containerizer/provisioners/appc.cpp (line 292)
<https://reviews.apache.org/r/37881/#comment153085>

    Please use Owned<Info> here.



src/slave/containerizer/provisioners/appc.cpp (line 376)
<https://reviews.apache.org/r/37881/#comment153074>

    Insert one blank line above.



src/slave/containerizer/provisioners/appc.cpp (lines 382 - 391)
<https://reviews.apache.org/r/37881/#comment153075>

    Let's simply this for now and revisit it later. Just return true here.
    
    ```
    // TODO: revisit later.
    return collect(futures);
      .then([]() { return true; });
    ```



src/slave/containerizer/provisioners/appc/paths.cpp (line 125)
<https://reviews.apache.org/r/37881/#comment153088>

    Kill this line.



src/slave/containerizer/provisioners/appc/paths.cpp (line 132)
<https://reviews.apache.org/r/37881/#comment153089>

    KIll this line.



src/slave/containerizer/provisioners/appc/paths.cpp (line 177)
<https://reviews.apache.org/r/37881/#comment153090>

    Kill this line.


sts

- Jie Yu


On Aug. 31, 2015, 5:45 p.m., Jiang Yan Xu wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37881/
> -----------------------------------------------------------
> 
> (Updated Aug. 31, 2015, 5:45 p.m.)
> 
> 
> Review request for mesos, Chi Zhang, Jie Yu, and Timothy Chen.
> 
> 
> Bugs: MESOS-2796
>     https://issues.apache.org/jira/browse/MESOS-2796
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> Implemented AppcProvisioner.
> 
> 
> Diffs
> -----
> 
>   src/Makefile.am 7b620ff66856b3f0adac121b3297d55ed71a3d99 
>   src/slave/containerizer/provisioner.hpp 541dd4e0b2f0c92a45c00cab6132a2be69654838 
>   src/slave/containerizer/provisioner.cpp efc7e6996ff6663bebaf61989a7e040bd2ad7a5e 
>   src/slave/containerizer/provisioners/appc.hpp PRE-CREATION 
>   src/slave/containerizer/provisioners/appc.cpp PRE-CREATION 
>   src/slave/containerizer/provisioners/appc/paths.hpp 41e3bf79da0854406c488855f953111e67353829

>   src/slave/containerizer/provisioners/appc/paths.cpp 3113c84b9526dd9e9e89fb9aa4ec75ed66a996c7

>   src/slave/flags.hpp e56738e2dfd6593ef8f093687919da287af78f77 
>   src/slave/flags.cpp b36710d6d7a7250bc071a57310a2d54bfb3bc624 
>   src/tests/containerizer/appc_provisioner_tests.cpp 47b66b9c30cefe8f9a8e2c1c1341776c2d235020

> 
> Diff: https://reviews.apache.org/r/37881/diff/
> 
> 
> Testing
> -------
> 
> sudo make check.
> 
> More test cases coming.
> 
> 
> Thanks,
> 
> Jiang Yan Xu
> 
>


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