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




src/slave/containerizer/mesos/isolators/docker/volume/isolator.hpp (line 65)
<https://reviews.apache.org/r/45370/#comment193796>

    s/DockerVolumeInfo/VolumeInfo/



src/slave/containerizer/mesos/isolators/docker/volume/isolator.hpp (lines 65 - 
70)
<https://reviews.apache.org/r/45370/#comment193817>

    We typically just put stuff that's needed for cleanup in 'Info'. Also, 
remember that any fields in 'Info' should be able to be recovered after agent 
restarts.
    
    So I would suggest that we don't put that in Info:
    ```
    struct VolumeInfo
    {
      DockerVolume volume;
      string containerPath;
      Option<hashmap<...>> options;
    };
    
    struct Info
    {
      vector<DockerVolume> volumes;
    };
    ```



src/slave/containerizer/mesos/isolators/docker/volume/isolator.hpp (line 67)
<https://reviews.apache.org/r/45370/#comment193797>

    s/dockerVolume/volume/



src/slave/containerizer/mesos/isolators/docker/volume/isolator.hpp (line 77)
<https://reviews.apache.org/r/45370/#comment193823>

    Should that be a hashset given that we don't allow duplicate?



src/slave/containerizer/mesos/isolators/docker/volume/isolator.cpp (line 139)
<https://reviews.apache.org/r/45370/#comment193821>

    s/dockerVolumeInfos/volumeInfos/



src/slave/containerizer/mesos/isolators/docker/volume/isolator.cpp (lines 141 - 
143)
<https://reviews.apache.org/r/45370/#comment193808>

    This sentense is broken. Please fix the gramma.



src/slave/containerizer/mesos/isolators/docker/volume/isolator.cpp (line 144)
<https://reviews.apache.org/r/45370/#comment193799>

    Instead of that, let's introduce a hash function for DockerVolume. You can 
put the hash function in state.hpp.
    
    ```
    template <>
    struct hash<mesos::internal::slave::DockerVolume>
    {
      ...
    };
    ```



src/slave/containerizer/mesos/isolators/docker/volume/isolator.cpp (lines 144 - 
145)
<https://reviews.apache.org/r/45370/#comment193800>

    s/dockerVolumeKey/volumes/



src/slave/containerizer/mesos/isolators/docker/volume/isolator.cpp (lines 151 - 
152)
<https://reviews.apache.org/r/45370/#comment193801>

    Please check the source.type as well. We might want to add more types later.



src/slave/containerizer/mesos/isolators/docker/volume/isolator.cpp (line 154)
<https://reviews.apache.org/r/45370/#comment193805>

    Once you define the hash for DockerVolume, you don't have to do that.



src/slave/containerizer/mesos/isolators/docker/volume/isolator.cpp (lines 156 - 
159)
<https://reviews.apache.org/r/45370/#comment193806>

    I would return a Failure here.



src/slave/containerizer/mesos/isolators/docker/volume/isolator.cpp (line 168)
<https://reviews.apache.org/r/45370/#comment193807>

    I don't think this check is necessary.



src/slave/containerizer/mesos/isolators/docker/volume/isolator.cpp (line 169)
<https://reviews.apache.org/r/45370/#comment193819>

    Let's kill this temp variable as well. We try to avoid temp variables if 
possible.



src/slave/containerizer/mesos/isolators/docker/volume/isolator.cpp (lines 170 - 
171)
<https://reviews.apache.org/r/45370/#comment193818>

    I would remove this logging.



src/slave/containerizer/mesos/isolators/docker/volume/isolator.cpp (lines 200 - 
202)
<https://reviews.apache.org/r/45370/#comment193836>

    I would add 'volume' to 'info.volumes' after we successfully create the 
containerDir  and successfully checkpoint the state file.
    
    This can save us from some unnecessary cleanup if any of the above failed.



src/slave/containerizer/mesos/isolators/docker/volume/isolator.cpp (lines 215 - 
217)
<https://reviews.apache.org/r/45370/#comment193813>

    I would move this log below after the checkpointing  is done.



src/slave/containerizer/mesos/isolators/docker/volume/isolator.cpp (line 229)
<https://reviews.apache.org/r/45370/#comment193837>

    s/checkpointed/checkpoint/



src/slave/containerizer/mesos/isolators/docker/volume/isolator.cpp (line 235)
<https://reviews.apache.org/r/45370/#comment193840>

    s/external/docker/



src/slave/containerizer/mesos/isolators/docker/volume/isolator.cpp (line 241)
<https://reviews.apache.org/r/45370/#comment193838>

    Please use const ref here.



src/slave/containerizer/mesos/isolators/docker/volume/isolator.cpp (line 242)
<https://reviews.apache.org/r/45370/#comment193839>

    YOu can iterate through `volumeInfos` here.



src/slave/containerizer/mesos/isolators/docker/volume/isolator.cpp (line 258)
<https://reviews.apache.org/r/45370/#comment193842>

    you can bind `volumeInfos` here.



src/slave/containerizer/mesos/isolators/docker/volume/isolator.cpp (line 266)
<https://reviews.apache.org/r/45370/#comment193844>

    s/volumes/futures/



src/slave/slave.cpp (lines 3813 - 3818)
<https://reviews.apache.org/r/45370/#comment193794>

    Can you do that in a separate patch?


- Jie Yu


On April 22, 2016, 4:08 p.m., Guangya Liu wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/45370/
> -----------------------------------------------------------
> 
> (Updated April 22, 2016, 4:08 p.m.)
> 
> 
> Review request for mesos, David vonThenen, Gilbert Song, haosdent huang, and 
> Jie Yu.
> 
> 
> Bugs: MESOS-5013
>     https://issues.apache.org/jira/browse/MESOS-5013
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> Implemented prepare() for volume isolator.
> 
> 
> Diffs
> -----
> 
>   src/slave/containerizer/mesos/isolators/docker/volume/isolator.hpp 
> bedc687cc280d0b721fb84801039fd3614364cca 
>   src/slave/containerizer/mesos/isolators/docker/volume/isolator.cpp 
> 915e5ae755a55a02b7dfcda88165f27346cad955 
>   src/slave/slave.cpp a365e8f5f8d63bf80614fccf6adf35a4fee07526 
> 
> Diff: https://reviews.apache.org/r/45370/diff/
> 
> 
> Testing
> -------
> 
> make
> make check
> 
> Will update `Sequence` later.
> 
> 
> Thanks,
> 
> Guangya Liu
> 
>

Reply via email to