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 45370: Implemented prepare() for volume isolator.
Date Fri, 22 Apr 2016 18:54:43 GMT

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


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