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

2016-01-23 Thread Joris Van Remoortere 

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

(Updated Jan. 23, 2016, 10:53 a.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 (updated)
-

  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.

2016-01-23 Thread Joris Van Remoortere 

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

(Updated Jan. 24, 2016, 1:20 a.m.)


Review request for mesos, Jie Yu and Michael Park.


Bugs: MESOS-4380
https://issues.apache.org/jira/browse/MESOS-4380


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

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

2016-01-23 Thread Joris Van Remoortere 

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

(Updated Jan. 24, 2016, 1:01 a.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 (updated)
-

  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.

2016-01-22 Thread Jie Yu 

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

Ship it!


Ship It!

- Jie Yu


On Jan. 22, 2016, 9:31 a.m., Joris Van Remoortere wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/42504/
> ---
> 
> (Updated Jan. 22, 2016, 9:31 a.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
> 
>

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

2016-01-22 Thread Joris Van Remoortere 

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

(Updated Jan. 22, 2016, 9:31 a.m.)


Review request for mesos, Jie Yu and Michael Park.


Changes
---

rebased.


Repository: mesos


Description
---

Refactored persistent volume tests to use a helper to generate disk
resources.


Diffs (updated)
-

  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.

2016-01-20 Thread Joris Van Remoortere 

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

(Updated Jan. 21, 2016, 7:39 a.m.)


Review request for mesos, Jie Yu and Michael Park.


Changes
---

Re-added volume stripping.
Added `createPersistentVolume` overload.


Repository: mesos


Description
---

Refactored persistent volume tests to use a helper to generate disk
resources.


Diffs (updated)
-

  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.

2016-01-20 Thread Joris Van Remoortere 

---
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 (updated)
---

Refactored persistent volume tests to use a helper to generate disk
resources.


Diffs (updated)
-

  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.

2016-01-20 Thread Jie Yu 

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



src/common/resources.cpp (line 968)


Let's strip the volume as well.



src/common/resources.cpp (line 1006)


Ditto. Strip volume as well.



src/common/resources_utils.cpp (line 64)


strip volume as well.



src/tests/mesos.hpp (lines 545 - 546)


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)


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



src/v1/resources.cpp (line 962)


Ditto on stripping volume.



src/v1/resources.cpp (line 1000)


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

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

2016-01-19 Thread Jie Yu 

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



src/common/resources.cpp (lines 918 - 919)


We should update the comments here.



src/common/resources.cpp (line 926)


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)


Do you need to update this as well?



src/tests/persistent_volume_tests.cpp (lines 91 - 106)


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)


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

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

2016-01-19 Thread Joris Van Remoortere 

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