Re: Review Request 42471: Multiple Disk: Adjusted resource arithmetic for 'DiskInfo.Source'.

2016-01-23 Thread Joris Van Remoortere 

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

(Updated Jan. 23, 2016, 10:47 a.m.)


Review request for mesos, Jie Yu and Michael Park.


Changes
---

Fixed subtractable


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


Repository: mesos


Description
---

Multiple Disk: Adjusted resource arithmetic for 'DiskInfo.Source'.


Diffs (updated)
-

  include/mesos/type_utils.hpp efe2b1de0c277db62d7f7cc5ff1cd9143b9f632a 
  include/mesos/v1/mesos.hpp 961042d8e4944a475076b829966020d62175d726 
  src/common/resources.cpp 575d6651185d8431f01d589f4afc255cb751181a 
  src/v1/resources.cpp 8de6672ba9b34947db81c74b8e03e8965e8af5fc 

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


Testing
---


Thanks,

Joris Van Remoortere

Re: Review Request 42471: Multiple Disk: Adjusted resource arithmetic for 'DiskInfo.Source'.

2016-01-23 Thread Joris Van Remoortere 


> On Jan. 22, 2016, 5:03 p.m., Jie Yu wrote:
> > src/common/resources.cpp, line 314
> > 
> >
> > This is not correct. You've already checked left.disk() == 
> > right.disk(), why do you need this? Instead, you should check if left != 
> > right:
> > 
> > ```
> > if (left.disk().has_source() &&
> > left.disk().source().type() == MOUNT &&
> > left != right) {
> >   return false;
> > }
> > ```
> 
> Jie Yu wrote:
> Also, could you please add a test case for this?

Added test cases in https://reviews.apache.org/r/42683/


- Joris


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


On Jan. 23, 2016, 10:47 a.m., Joris Van Remoortere wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/42471/
> ---
> 
> (Updated Jan. 23, 2016, 10:47 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
> ---
> 
> Multiple Disk: Adjusted resource arithmetic for 'DiskInfo.Source'.
> 
> 
> Diffs
> -
> 
>   include/mesos/type_utils.hpp efe2b1de0c277db62d7f7cc5ff1cd9143b9f632a 
>   include/mesos/v1/mesos.hpp 961042d8e4944a475076b829966020d62175d726 
>   src/common/resources.cpp 575d6651185d8431f01d589f4afc255cb751181a 
>   src/v1/resources.cpp 8de6672ba9b34947db81c74b8e03e8965e8af5fc 
> 
> Diff: https://reviews.apache.org/r/42471/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Joris Van Remoortere
> 
>

Re: Review Request 42471: Multiple Disk: Adjusted resource arithmetic for 'DiskInfo.Source'.

2016-01-23 Thread Jie Yu 

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


Ship it!




Ship It!

- Jie Yu


On Jan. 23, 2016, 10:47 a.m., Joris Van Remoortere wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/42471/
> ---
> 
> (Updated Jan. 23, 2016, 10:47 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
> ---
> 
> Multiple Disk: Adjusted resource arithmetic for 'DiskInfo.Source'.
> 
> 
> Diffs
> -
> 
>   include/mesos/type_utils.hpp efe2b1de0c277db62d7f7cc5ff1cd9143b9f632a 
>   include/mesos/v1/mesos.hpp 961042d8e4944a475076b829966020d62175d726 
>   src/common/resources.cpp 575d6651185d8431f01d589f4afc255cb751181a 
>   src/v1/resources.cpp 8de6672ba9b34947db81c74b8e03e8965e8af5fc 
> 
> Diff: https://reviews.apache.org/r/42471/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Joris Van Remoortere
> 
>

Re: Review Request 42471: Multiple Disk: Adjusted resource arithmetic for 'DiskInfo.Source'.

2016-01-22 Thread Joris Van Remoortere 

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

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


Review request for mesos, Jie Yu and Michael Park.


Changes
---

Addressed comments.


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


Repository: mesos


Description
---

Multiple Disk: Adjusted resource arithmetic for 'DiskInfo.Source'.


Diffs (updated)
-

  include/mesos/type_utils.hpp efe2b1de0c277db62d7f7cc5ff1cd9143b9f632a 
  include/mesos/v1/mesos.hpp 961042d8e4944a475076b829966020d62175d726 
  src/common/resources.cpp 575d6651185d8431f01d589f4afc255cb751181a 
  src/v1/resources.cpp 8de6672ba9b34947db81c74b8e03e8965e8af5fc 

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


Testing
---


Thanks,

Joris Van Remoortere

Re: Review Request 42471: Multiple Disk: Adjusted resource arithmetic for 'DiskInfo.Source'.

2016-01-22 Thread Jie Yu 

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



src/common/resources.cpp (line 310)


This is not correct. You've already checked left.disk() == right.disk(), 
why do you need this? Instead, you should check if left != right:

```
if (left.disk().has_source() &&
left.disk().source().type() == MOUNT &&
left != right) {
  return false;
}
```



src/v1/resources.cpp (lines 308 - 310)


Ditto.


- Jie Yu


On Jan. 22, 2016, 9:27 a.m., Joris Van Remoortere wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/42471/
> ---
> 
> (Updated Jan. 22, 2016, 9:27 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
> ---
> 
> Multiple Disk: Adjusted resource arithmetic for 'DiskInfo.Source'.
> 
> 
> Diffs
> -
> 
>   include/mesos/type_utils.hpp efe2b1de0c277db62d7f7cc5ff1cd9143b9f632a 
>   include/mesos/v1/mesos.hpp 961042d8e4944a475076b829966020d62175d726 
>   src/common/resources.cpp 575d6651185d8431f01d589f4afc255cb751181a 
>   src/v1/resources.cpp 8de6672ba9b34947db81c74b8e03e8965e8af5fc 
> 
> Diff: https://reviews.apache.org/r/42471/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Joris Van Remoortere
> 
>

Re: Review Request 42471: Multiple Disk: Adjusted resource arithmetic for 'DiskInfo.Source'.

2016-01-22 Thread Jie Yu 


> On Jan. 22, 2016, 5:03 p.m., Jie Yu wrote:
> > src/common/resources.cpp, line 314
> > 
> >
> > This is not correct. You've already checked left.disk() == 
> > right.disk(), why do you need this? Instead, you should check if left != 
> > right:
> > 
> > ```
> > if (left.disk().has_source() &&
> > left.disk().source().type() == MOUNT &&
> > left != right) {
> >   return false;
> > }
> > ```

Also, could you please add a test case for this?


- Jie


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


On Jan. 22, 2016, 9:27 a.m., Joris Van Remoortere wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/42471/
> ---
> 
> (Updated Jan. 22, 2016, 9:27 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
> ---
> 
> Multiple Disk: Adjusted resource arithmetic for 'DiskInfo.Source'.
> 
> 
> Diffs
> -
> 
>   include/mesos/type_utils.hpp efe2b1de0c277db62d7f7cc5ff1cd9143b9f632a 
>   include/mesos/v1/mesos.hpp 961042d8e4944a475076b829966020d62175d726 
>   src/common/resources.cpp 575d6651185d8431f01d589f4afc255cb751181a 
>   src/v1/resources.cpp 8de6672ba9b34947db81c74b8e03e8965e8af5fc 
> 
> Diff: https://reviews.apache.org/r/42471/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Joris Van Remoortere
> 
>

Re: Review Request 42471: Multiple Disk: Adjusted resource arithmetic for 'DiskInfo.Source'.

2016-01-21 Thread Jie Yu 

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



src/common/resources.cpp (lines 102 - 108)


Can you do the following to be more consistent with others
```
if (left.has_path() && left.path() != right.path()) {
  return false;
}
if (left.has_mount() && left.mount() != right.mount()) {
  return false;
}
```



src/common/resources.cpp (lines 251 - 255)


I'd prefer removing this CHECK here since it's on the critical path of 
resource calculation.



src/common/resources.cpp (lines 309 - 328)


Ditto on merge them into a single if clause.



src/common/resources.cpp (lines 315 - 317)


Ditto on removing this CHECK.



src/common/resources.cpp (lines 319 - 322)


This does not look correct. Two identical MOUNT disk resource should be 
subtractable.



src/v1/resources.cpp (lines 102 - 104)


Can you do the following to be consistent with others:
```
if (left.has_path() && left.path() != right.path()) {
  return false;
}

if (left.has_mount() && left.mount() != right.mount()) {
  return false;
}

return true;
```



src/v1/resources.cpp (lines 247 - 268)


We should merge those check under the same `has_disk` if clause:

```
if (left.has_disk()) {
  // Check if disk() are equal
  // MOUNT check
  // persistence check
}
```



src/v1/resources.cpp (lines 253 - 255)


Ditto on removing it.



src/v1/resources.cpp (lines 315 - 317)


Ditto on removing it.



src/v1/resources.cpp (lines 319 - 322)


Ditto. Please adjust the logics here according to my comments above.


- Jie Yu


On Jan. 21, 2016, 7:36 a.m., Joris Van Remoortere wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/42471/
> ---
> 
> (Updated Jan. 21, 2016, 7:36 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
> ---
> 
> Multiple Disk: Adjusted resource arithmetic for 'DiskInfo.Source'.
> 
> 
> Diffs
> -
> 
>   include/mesos/type_utils.hpp efe2b1de0c277db62d7f7cc5ff1cd9143b9f632a 
>   include/mesos/v1/mesos.hpp 961042d8e4944a475076b829966020d62175d726 
>   src/common/resources.cpp 575d6651185d8431f01d589f4afc255cb751181a 
>   src/v1/resources.cpp 8de6672ba9b34947db81c74b8e03e8965e8af5fc 
> 
> Diff: https://reviews.apache.org/r/42471/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Joris Van Remoortere
> 
>

Re: Review Request 42471: Multiple Disk: Adjusted resource arithmetic for 'DiskInfo.Source'.

2016-01-20 Thread Joris Van Remoortere 

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

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


Review request for mesos, Jie Yu and Michael Park.


Changes
---

Added logic for `Mount` type.


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


Repository: mesos


Description
---

Multiple Disk: Adjusted resource arithmetic for 'DiskInfo.Source'.


Diffs (updated)
-

  include/mesos/type_utils.hpp efe2b1de0c277db62d7f7cc5ff1cd9143b9f632a 
  include/mesos/v1/mesos.hpp 961042d8e4944a475076b829966020d62175d726 
  src/common/resources.cpp 575d6651185d8431f01d589f4afc255cb751181a 
  src/v1/resources.cpp 8de6672ba9b34947db81c74b8e03e8965e8af5fc 

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


Testing
---


Thanks,

Joris Van Remoortere

Re: Review Request 42471: Multiple Disk: Adjusted resource arithmetic for 'DiskInfo.Source'.

2016-01-19 Thread Jie Yu 

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


Do we want to expose == and != for DiskInfo::Source yet? We don't even have == 
and != exposed for DiskInfo yet (same for ReservationInfo, etc.). Maybe make 
them file local helpers in src/common/resources.cpp for now.


include/mesos/type_utils.hpp (line 29)


Do you need this?



include/mesos/type_utils.hpp (lines 64 - 74)


Not yours. I wish we could sort them according to lexical order as we did 
before.



include/mesos/type_utils.hpp (lines 167 - 175)


Can you move this above `operator==(const SlaveID&`



include/mesos/type_utils.hpp (line 171)


Can you move non trivial impl. to common/type_utils.cpp?



include/mesos/type_utils.hpp (lines 179 - 187)


Ditto. Move non trivial impl. to cpp file.



include/mesos/type_utils.hpp (lines 242 - 255)


Not yours, can you put these above SlaveID.



include/mesos/v1/mesos.hpp (line 30)


No you need this?



include/mesos/v1/values.hpp (line 22)


Why this change?



src/common/resources.cpp (lines 93 - 99)


I prefer to check 'source' first. Can we move this up above the persistence 
checks.



src/v1/resources.cpp (lines 93 - 99)


Ditto.


- Jie Yu


On Jan. 18, 2016, 9:23 p.m., Joris Van Remoortere wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/42471/
> ---
> 
> (Updated Jan. 18, 2016, 9:23 p.m.)
> 
> 
> Review request for mesos, Jie Yu and Michael Park.
> 
> 
> Bugs: MESOS-4380
> https://issues.apache.org/jira/browse/MESOS-4380
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> See summary.
> 
> 
> Diffs
> -
> 
>   include/mesos/type_utils.hpp efe2b1de0c277db62d7f7cc5ff1cd9143b9f632a 
>   include/mesos/v1/mesos.hpp 961042d8e4944a475076b829966020d62175d726 
>   include/mesos/v1/values.hpp 03a19ee44b3d7f801f7ca2a61f1d42d51e0c144b 
>   src/common/resources.cpp 575d6651185d8431f01d589f4afc255cb751181a 
>   src/v1/resources.cpp 8de6672ba9b34947db81c74b8e03e8965e8af5fc 
> 
> Diff: https://reviews.apache.org/r/42471/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Joris Van Remoortere
> 
>

Re: Review Request 42471: Multiple Disk: Adjusted resource arithmetic for 'DiskInfo.Source'.

2016-01-19 Thread Jie Yu 

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

Ship it!


Ship It!

- Jie Yu


On Jan. 20, 2016, 2:29 a.m., Joris Van Remoortere wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/42471/
> ---
> 
> (Updated Jan. 20, 2016, 2:29 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
> ---
> 
> Multiple Disk: Adjusted resource arithmetic for 'DiskInfo.Source'.
> 
> 
> Diffs
> -
> 
>   include/mesos/type_utils.hpp efe2b1de0c277db62d7f7cc5ff1cd9143b9f632a 
>   include/mesos/v1/mesos.hpp 961042d8e4944a475076b829966020d62175d726 
>   src/common/resources.cpp 575d6651185d8431f01d589f4afc255cb751181a 
>   src/v1/resources.cpp 8de6672ba9b34947db81c74b8e03e8965e8af5fc 
> 
> Diff: https://reviews.apache.org/r/42471/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Joris Van Remoortere
> 
>

Re: Review Request 42471: Multiple Disk: Adjusted resource arithmetic for 'DiskInfo.Source'.

2016-01-19 Thread Joris Van Remoortere 


> On Jan. 19, 2016, 10:50 p.m., Jie Yu wrote:
> > include/mesos/type_utils.hpp, line 29
> > 
> >
> > Do you need this?

Not after we get rid of `total_size`. I needed it for the scalar comparison.


- Joris


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


On Jan. 18, 2016, 9:23 p.m., Joris Van Remoortere wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/42471/
> ---
> 
> (Updated Jan. 18, 2016, 9:23 p.m.)
> 
> 
> Review request for mesos, Jie Yu and Michael Park.
> 
> 
> Bugs: MESOS-4380
> https://issues.apache.org/jira/browse/MESOS-4380
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> See summary.
> 
> 
> Diffs
> -
> 
>   include/mesos/type_utils.hpp efe2b1de0c277db62d7f7cc5ff1cd9143b9f632a 
>   include/mesos/v1/mesos.hpp 961042d8e4944a475076b829966020d62175d726 
>   include/mesos/v1/values.hpp 03a19ee44b3d7f801f7ca2a61f1d42d51e0c144b 
>   src/common/resources.cpp 575d6651185d8431f01d589f4afc255cb751181a 
>   src/v1/resources.cpp 8de6672ba9b34947db81c74b8e03e8965e8af5fc 
> 
> Diff: https://reviews.apache.org/r/42471/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Joris Van Remoortere
> 
>

Re: Review Request 42471: Multiple Disk: Adjusted resource arithmetic for 'DiskInfo.Source'.

2016-01-19 Thread Joris Van Remoortere 


> On Jan. 19, 2016, 10:50 p.m., Jie Yu wrote:
> > include/mesos/v1/values.hpp, line 22
> > 
> >
> > Why this change?

It was due to a cyclical include between `values.hpp` and `mesos.hpp`; however, 
since I moved the operators it's not necessary anymore.


- Joris


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


On Jan. 18, 2016, 9:23 p.m., Joris Van Remoortere wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/42471/
> ---
> 
> (Updated Jan. 18, 2016, 9:23 p.m.)
> 
> 
> Review request for mesos, Jie Yu and Michael Park.
> 
> 
> Bugs: MESOS-4380
> https://issues.apache.org/jira/browse/MESOS-4380
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> See summary.
> 
> 
> Diffs
> -
> 
>   include/mesos/type_utils.hpp efe2b1de0c277db62d7f7cc5ff1cd9143b9f632a 
>   include/mesos/v1/mesos.hpp 961042d8e4944a475076b829966020d62175d726 
>   include/mesos/v1/values.hpp 03a19ee44b3d7f801f7ca2a61f1d42d51e0c144b 
>   src/common/resources.cpp 575d6651185d8431f01d589f4afc255cb751181a 
>   src/v1/resources.cpp 8de6672ba9b34947db81c74b8e03e8965e8af5fc 
> 
> Diff: https://reviews.apache.org/r/42471/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Joris Van Remoortere
> 
>

Re: Review Request 42471: Multiple Disk: Adjusted resource arithmetic for 'DiskInfo.Source'.

2016-01-19 Thread Joris Van Remoortere 

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

(Updated Jan. 20, 2016, 2:29 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 (updated)
---

Multiple Disk: Adjusted resource arithmetic for 'DiskInfo.Source'.


Diffs (updated)
-

  include/mesos/type_utils.hpp efe2b1de0c277db62d7f7cc5ff1cd9143b9f632a 
  include/mesos/v1/mesos.hpp 961042d8e4944a475076b829966020d62175d726 
  src/common/resources.cpp 575d6651185d8431f01d589f4afc255cb751181a 
  src/v1/resources.cpp 8de6672ba9b34947db81c74b8e03e8965e8af5fc 

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


Testing
---


Thanks,

Joris Van Remoortere

Review Request 42471: Multiple Disk: Adjusted resource arithmetic for 'DiskInfo.Source'.

2016-01-18 Thread Joris Van Remoortere 

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

Review request for mesos, Jie Yu and Michael Park.


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


Repository: mesos


Description
---

See summary.


Diffs
-

  include/mesos/type_utils.hpp efe2b1de0c277db62d7f7cc5ff1cd9143b9f632a 
  include/mesos/v1/mesos.hpp 961042d8e4944a475076b829966020d62175d726 
  include/mesos/v1/values.hpp 03a19ee44b3d7f801f7ca2a61f1d42d51e0c144b 
  src/common/resources.cpp 575d6651185d8431f01d589f4afc255cb751181a 
  src/v1/resources.cpp 8de6672ba9b34947db81c74b8e03e8965e8af5fc 

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


Testing
---


Thanks,

Joris Van Remoortere