You are viewing a plain text version of this content. The canonical link for it is here.

Posted to reviews@mesos.apache.org by Joris Van Remoortere <jo...@gmail.com> on 2016/01/19 16:51:52 UTC

Review Request 42504: Multiple Disk: Modified 'DiskInfo' stripping logic.

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

Review request for mesos, Jie Yu and Michael Park.


Repository: mesos


Description
-------

Refactored persistent volume tests to use common slave resources.


Diffs
-----

  src/common/resources.cpp 575d6651185d8431f01d589f4afc255cb751181a 
  src/common/resources_utils.cpp d64db54a175299bf7c32ecbeeb1ce9a7afb1c88a 
  src/tests/mesos.hpp 3d9ebc6c9dc3cd1be02dc3771fbd847386907fac 
  src/tests/persistent_volume_tests.cpp 7acf7ab29d64d891f3288f8042d267dcc82a32e9 
  src/tests/resources_tests.cpp b42610f1bf8eacfd7bf388d351f8745f1d96f666 
  src/v1/resources.cpp 8de6672ba9b34947db81c74b8e03e8965e8af5fc 

Diff: https://reviews.apache.org/r/42504/diff/


Testing
-------


Thanks,

Joris Van Remoortere

Re: Review Request 42504: Multiple Disk: Modified 'DiskInfo' stripping logic.

Posted by Jie Yu <yu...@gmail.com>.

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



src/common/resources.cpp (lines 918 - 919)
<https://reviews.apache.org/r/42504/#comment176227>

    We should update the comments here.



src/common/resources.cpp (line 926)
<https://reviews.apache.org/r/42504/#comment176232>

    No need for the 'has_disk()' check before it's already checked above. THe following looks more clean to me.
    
    ```
    if (stripped.disk().has_source()) {
      ...
    } else {
      ...
    }
    ```



src/common/resources.cpp (lines 961 - 966)
<https://reviews.apache.org/r/42504/#comment176233>

    Do you need to update this as well?



src/tests/persistent_volume_tests.cpp (lines 91 - 106)
<https://reviews.apache.org/r/42504/#comment176253>

    Instead of overwriting CreateSlaveFlags() which is a little implicit, can we add a SlaveResources() method to return the slave's resources to be more explicit.
    
    ```
    Resources getDiskResources(const Megabytes& mb)
    {
      ...
    }
    
    slaveFlags.resources = getDiskResources(...);
    ```



src/tests/persistent_volume_tests.cpp (lines 576 - 580)
<https://reviews.apache.org/r/42504/#comment176262>

    Wondering if we should introduce an overload of 'createPersistentVolume' that takes resource, id and containerPath. The might be able to simply your subsequent patch on tests.


- Jie Yu


On Jan. 19, 2016, 3:51 p.m., Joris Van Remoortere wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/42504/
> -----------------------------------------------------------
> 
> (Updated Jan. 19, 2016, 3:51 p.m.)
> 
> 
> Review request for mesos, Jie Yu and Michael Park.
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> Refactored persistent volume tests to use common slave resources.
> 
> 
> Diffs
> -----
> 
>   src/common/resources.cpp 575d6651185d8431f01d589f4afc255cb751181a 
>   src/common/resources_utils.cpp d64db54a175299bf7c32ecbeeb1ce9a7afb1c88a 
>   src/tests/mesos.hpp 3d9ebc6c9dc3cd1be02dc3771fbd847386907fac 
>   src/tests/persistent_volume_tests.cpp 7acf7ab29d64d891f3288f8042d267dcc82a32e9 
>   src/tests/resources_tests.cpp b42610f1bf8eacfd7bf388d351f8745f1d96f666 
>   src/v1/resources.cpp 8de6672ba9b34947db81c74b8e03e8965e8af5fc 
> 
> Diff: https://reviews.apache.org/r/42504/diff/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Joris Van Remoortere
> 
>

Re: Review Request 42504: Multiple Disk: Modified 'DiskInfo' stripping logic.

Posted by Jie Yu <yu...@gmail.com>.

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



src/common/resources.cpp (line 968)
<https://reviews.apache.org/r/42504/#comment176457>

    Let's strip the volume as well.



src/common/resources.cpp (line 1006)
<https://reviews.apache.org/r/42504/#comment176458>

    Ditto. Strip volume as well.



src/common/resources_utils.cpp (line 64)
<https://reviews.apache.org/r/42504/#comment176459>

    strip volume as well.



src/tests/mesos.hpp (lines 545 - 546)
<https://reviews.apache.org/r/42504/#comment176461>

    Hum, we should not create DiskInfo if it's not persistent volume and no source is specifed?



src/tests/persistent_volume_tests.cpp (line 94)
<https://reviews.apache.org/r/42504/#comment176462>

    See my comments above. You don't need this.



src/v1/resources.cpp (line 962)
<https://reviews.apache.org/r/42504/#comment176463>

    Ditto on stripping volume.



src/v1/resources.cpp (line 1000)
<https://reviews.apache.org/r/42504/#comment176464>

    Same here.


- Jie Yu


On Jan. 20, 2016, 7:18 p.m., Joris Van Remoortere wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/42504/
> -----------------------------------------------------------
> 
> (Updated Jan. 20, 2016, 7:18 p.m.)
> 
> 
> Review request for mesos, Jie Yu and Michael Park.
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> Refactored persistent volume tests to use a helper to generate disk
> resources.
> 
> 
> Diffs
> -----
> 
>   src/common/resources.cpp 575d6651185d8431f01d589f4afc255cb751181a 
>   src/common/resources_utils.cpp d64db54a175299bf7c32ecbeeb1ce9a7afb1c88a 
>   src/tests/mesos.hpp 3d9ebc6c9dc3cd1be02dc3771fbd847386907fac 
>   src/tests/persistent_volume_tests.cpp 7acf7ab29d64d891f3288f8042d267dcc82a32e9 
>   src/tests/resources_tests.cpp b42610f1bf8eacfd7bf388d351f8745f1d96f666 
>   src/v1/resources.cpp 8de6672ba9b34947db81c74b8e03e8965e8af5fc 
> 
> Diff: https://reviews.apache.org/r/42504/diff/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Joris Van Remoortere
> 
>