Re: Review Request 49376: Added assertions to DRFSorter.

2016-07-04 Thread Guangya Liu 


> On 七月 4, 2016, 8:01 a.m., Guangya Liu wrote:
> > src/master/allocator/sorter/drf/sorter.cpp, lines 272-275
> > 
> >
> > How about adjust the order a bit as following?
> > 
> > CHECK(allocations[name].resources[slaveId].contains(resources));
> > 
> > const Resources resourcesQuantity = 
> > resources.createStrippedScalarQuantity();
> > CHECK(allocations[name].scalarQuantities.contains(resourcesQuantity));
> 
> Neil Conway wrote:
> Personally, I prefer grouping the two `CHECKs` together because they are 
> semantically related.

I'm ok either way, the only advantage of updating 
`CHECK(allocations[name].resources[slaveId].contains(resources));` to be 
checked first can avoid the case of `const Resources resourcesQuantity = 
resources.createStrippedScalarQuantity();` was calculated but 
`CHECK(allocations[name].resources[slaveId].contains(resources));` failed.


- Guangya


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


On 七月 3, 2016, 8:36 a.m., Neil Conway wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/49376/
> ---
> 
> (Updated 七月 3, 2016, 8:36 a.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov, Joris Van Remoortere, and 
> Michael Park.
> 
> 
> Bugs: MESOS-5698
> https://issues.apache.org/jira/browse/MESOS-5698
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added assertions to DRFSorter.
> 
> 
> Diffs
> -
> 
>   src/master/allocator/sorter/drf/sorter.cpp 
> 967290d4d1100208900b4b724422c3218abc23cb 
> 
> Diff: https://reviews.apache.org/r/49376/diff/
> 
> 
> Testing
> ---
> 
> These assertions DO NOT PASS. They are conceptually correct, however -- after 
> r/49377 they pass on `make check`.
> 
> 
> Thanks,
> 
> Neil Conway
> 
>

Re: Review Request 49376: Added assertions to DRFSorter.

2016-07-04 Thread Neil Conway 


> On July 4, 2016, 8:01 a.m., Guangya Liu wrote:
> > src/master/allocator/sorter/drf/sorter.cpp, lines 272-275
> > 
> >
> > How about adjust the order a bit as following?
> > 
> > CHECK(allocations[name].resources[slaveId].contains(resources));
> > 
> > const Resources resourcesQuantity = 
> > resources.createStrippedScalarQuantity();
> > CHECK(allocations[name].scalarQuantities.contains(resourcesQuantity));

Personally, I prefer grouping the two `CHECKs` together because they are 
semantically related.


- Neil


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


On July 3, 2016, 8:36 a.m., Neil Conway wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/49376/
> ---
> 
> (Updated July 3, 2016, 8:36 a.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov, Joris Van Remoortere, and 
> Michael Park.
> 
> 
> Bugs: MESOS-5698
> https://issues.apache.org/jira/browse/MESOS-5698
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added assertions to DRFSorter.
> 
> 
> Diffs
> -
> 
>   src/master/allocator/sorter/drf/sorter.cpp 
> 967290d4d1100208900b4b724422c3218abc23cb 
> 
> Diff: https://reviews.apache.org/r/49376/diff/
> 
> 
> Testing
> ---
> 
> These assertions DO NOT PASS. They are conceptually correct, however -- after 
> r/49377 they pass on `make check`.
> 
> 
> Thanks,
> 
> Neil Conway
> 
>

Re: Review Request 49376: Added assertions to DRFSorter.

2016-07-04 Thread Guangya Liu 

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




src/master/allocator/sorter/drf/sorter.cpp (lines 272 - 275)


How about adjust the order a bit as following?

CHECK(allocations[name].resources[slaveId].contains(resources));

const Resources resourcesQuantity = 
resources.createStrippedScalarQuantity();
CHECK(allocations[name].scalarQuantities.contains(resourcesQuantity));


- Guangya Liu


On 七月 3, 2016, 8:36 a.m., Neil Conway wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/49376/
> ---
> 
> (Updated 七月 3, 2016, 8:36 a.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov, Joris Van Remoortere, and 
> Michael Park.
> 
> 
> Bugs: MESOS-5698
> https://issues.apache.org/jira/browse/MESOS-5698
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added assertions to DRFSorter.
> 
> 
> Diffs
> -
> 
>   src/master/allocator/sorter/drf/sorter.cpp 
> 967290d4d1100208900b4b724422c3218abc23cb 
> 
> Diff: https://reviews.apache.org/r/49376/diff/
> 
> 
> Testing
> ---
> 
> These assertions DO NOT PASS. They are conceptually correct, however -- after 
> r/49377 they pass on `make check`.
> 
> 
> Thanks,
> 
> Neil Conway
> 
>

Re: Review Request 49376: Added assertions to DRFSorter.

2016-07-03 Thread Michael Park 

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


Ship it!




Ship It!

- Michael Park


On July 3, 2016, 8:36 a.m., Neil Conway wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/49376/
> ---
> 
> (Updated July 3, 2016, 8:36 a.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov, Joris Van Remoortere, and 
> Michael Park.
> 
> 
> Bugs: MESOS-5698
> https://issues.apache.org/jira/browse/MESOS-5698
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added assertions to DRFSorter.
> 
> 
> Diffs
> -
> 
>   src/master/allocator/sorter/drf/sorter.cpp 
> 967290d4d1100208900b4b724422c3218abc23cb 
> 
> Diff: https://reviews.apache.org/r/49376/diff/
> 
> 
> Testing
> ---
> 
> These assertions DO NOT PASS. They are conceptually correct, however -- after 
> r/49377 they pass on `make check`.
> 
> 
> Thanks,
> 
> Neil Conway
> 
>

Re: Review Request 49376: Added assertions to DRFSorter.

2016-07-03 Thread Neil Conway 

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

(Updated July 3, 2016, 8:36 a.m.)


Review request for mesos, Alexander Rukletsov, Joris Van Remoortere, and 
Michael Park.


Changes
---

Rename `removalQuantity` to `resourcesQuantity` per review.


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


Repository: mesos


Description
---

Added assertions to DRFSorter.


Diffs (updated)
-

  src/master/allocator/sorter/drf/sorter.cpp 
967290d4d1100208900b4b724422c3218abc23cb 

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


Testing
---

These assertions DO NOT PASS. They are conceptually correct, however -- after 
r/49377 they pass on `make check`.


Thanks,

Neil Conway

Re: Review Request 49376: Added assertions to DRFSorter.

2016-07-03 Thread Alexander Rukletsov 

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


Fix it, then Ship it!





src/master/allocator/sorter/drf/sorter.cpp (line 307)


Can we call it `resourcesQuantity` for consistency? `removalQuantity` seems 
like a better name, but the same variable above is called `resourcesQuantity`.


- Alexander Rukletsov


On July 1, 2016, 9:47 a.m., Neil Conway wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/49376/
> ---
> 
> (Updated July 1, 2016, 9:47 a.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov, Joris Van Remoortere, and 
> Michael Park.
> 
> 
> Bugs: MESOS-5698
> https://issues.apache.org/jira/browse/MESOS-5698
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added assertions to DRFSorter.
> 
> 
> Diffs
> -
> 
>   src/master/allocator/sorter/drf/sorter.cpp 
> 967290d4d1100208900b4b724422c3218abc23cb 
> 
> Diff: https://reviews.apache.org/r/49376/diff/
> 
> 
> Testing
> ---
> 
> These assertions DO NOT PASS. They are conceptually correct, however -- after 
> r/49377 they pass on `make check`.
> 
> 
> Thanks,
> 
> Neil Conway
> 
>

Re: Review Request 49376: Added assertions to DRFSorter.

2016-07-01 Thread Neil Conway 

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

(Updated July 1, 2016, 9:47 a.m.)


Review request for mesos, Alexander Rukletsov, Joris Van Remoortere, and 
Michael Park.


Changes
---

Rebase.


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


Repository: mesos


Description
---

Added assertions to DRFSorter.


Diffs (updated)
-

  src/master/allocator/sorter/drf/sorter.cpp 
967290d4d1100208900b4b724422c3218abc23cb 

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


Testing
---

These assertions DO NOT PASS. They are conceptually correct, however -- after 
r/49377 they pass on `make check`.


Thanks,

Neil Conway

Review Request 49376: Added assertions to DRFSorter.

2016-06-29 Thread Neil Conway 

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

Review request for mesos, Alexander Rukletsov, Joris Van Remoortere, and 
Michael Park.


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


Repository: mesos


Description
---

Added assertions to DRFSorter.


Diffs
-

  src/master/allocator/sorter/drf/sorter.cpp 
27d56f274c41bbabe6f2abbbcebd2c4eff52693e 

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


Testing
---

These assertions DO NOT PASS. They are conceptually correct, however -- after 
r/49377 they pass on `make check`.


Thanks,

Neil Conway