Re: Review Request 35473: Removed a few incorrect CHECKs in DRF sorter.

2015-06-16 Thread Jie Yu 


> On June 16, 2015, 5:09 p.m., Niklas Nielsen wrote:
> > src/master/allocator/sorter/drf/sorter.cpp, line 177
> > 
> >
> > This isn't just fixing the check's then but is introducing a fast path? 
> > Should we capture this in the review description? Or maybe comment here?

Niq, this is not just a fast path. The invariant here is that we don't create a 
key (i.e., slaveId) in 'resources' unless there is a NON EMPTY resources 
associated with that slave. And we'll delete the key if the corresponding 
resources become empty.


- Jie


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


On June 15, 2015, 10:03 p.m., Jie Yu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/35473/
> ---
> 
> (Updated June 15, 2015, 10:03 p.m.)
> 
> 
> Review request for mesos, Ben Mahler, Niklas Nielsen, and Vinod Kone.
> 
> 
> Bugs: MESOS-2627
> https://issues.apache.org/jira/browse/MESOS-2627
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Removed a few incorrect CHECKs in DRF sorter.
> 
> See details in the ticket for the reason why this is needed.
> 
> 
> Diffs
> -
> 
>   src/master/allocator/sorter/drf/sorter.cpp 
> ac05afdc7d408735dd796faa68c943e75540aaa7 
>   src/tests/examples_tests.cpp 41a9265e91ea54b657afbd32c22e204c5f64ca17 
> 
> Diff: https://reviews.apache.org/r/35473/diff/
> 
> 
> Testing
> ---
> 
> sudo make check
> 
> 
> Thanks,
> 
> Jie Yu
> 
>

Re: Review Request 35473: Removed a few incorrect CHECKs in DRF sorter.

2015-06-16 Thread Niklas Nielsen 

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


LGTM module Ben's comment and removing the (now obsolete) test comment.


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


This isn't just fixing the check's then but is introducing a fast path? 
Should we capture this in the review description? Or maybe comment here?


- Niklas Nielsen


On June 15, 2015, 3:03 p.m., Jie Yu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/35473/
> ---
> 
> (Updated June 15, 2015, 3:03 p.m.)
> 
> 
> Review request for mesos, Ben Mahler, Niklas Nielsen, and Vinod Kone.
> 
> 
> Bugs: MESOS-2627
> https://issues.apache.org/jira/browse/MESOS-2627
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Removed a few incorrect CHECKs in DRF sorter.
> 
> See details in the ticket for the reason why this is needed.
> 
> 
> Diffs
> -
> 
>   src/master/allocator/sorter/drf/sorter.cpp 
> ac05afdc7d408735dd796faa68c943e75540aaa7 
>   src/tests/examples_tests.cpp 41a9265e91ea54b657afbd32c22e204c5f64ca17 
> 
> Diff: https://reviews.apache.org/r/35473/diff/
> 
> 
> Testing
> ---
> 
> sudo make check
> 
> 
> Thanks,
> 
> Jie Yu
> 
>

Re: Review Request 35473: Removed a few incorrect CHECKs in DRF sorter.

2015-06-15 Thread Ben Mahler 

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

Ship it!


I personally find it easier to read if we don't have the special 'return' cases:

```
{
  if (!resources.empty()) {
// Do work.
  }
}

Seems clearer than:
{
  if (resources.empty()) {
// Don't do work by returning.
  }
  
  // Do work.
}
```

I think most of the cases where we've done the early returns were motivated by 
a few reasons that don't hold here:

(1) We wanted to avoid deep nesting of logic (using 'returns' allowed us to 
flatten the logical structure), and/or
(2) We wanted to eliminate many special cases in sequence, until only the 
correct case remains (e.g. message handlers, (a) can't lookup framework, (b) 
message sent by wrong pid, (c) invalid offers used, ...).

Up to you!


src/tests/examples_tests.cpp


Don't forget to delete this


- Ben Mahler


On June 15, 2015, 10:03 p.m., Jie Yu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/35473/
> ---
> 
> (Updated June 15, 2015, 10:03 p.m.)
> 
> 
> Review request for mesos, Ben Mahler, Niklas Nielsen, and Vinod Kone.
> 
> 
> Bugs: MESOS-2627
> https://issues.apache.org/jira/browse/MESOS-2627
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Removed a few incorrect CHECKs in DRF sorter.
> 
> See details in the ticket for the reason why this is needed.
> 
> 
> Diffs
> -
> 
>   src/master/allocator/sorter/drf/sorter.cpp 
> ac05afdc7d408735dd796faa68c943e75540aaa7 
>   src/tests/examples_tests.cpp 41a9265e91ea54b657afbd32c22e204c5f64ca17 
> 
> Diff: https://reviews.apache.org/r/35473/diff/
> 
> 
> Testing
> ---
> 
> sudo make check
> 
> 
> Thanks,
> 
> Jie Yu
> 
>

Re: Review Request 35473: Removed a few incorrect CHECKs in DRF sorter.

2015-06-15 Thread Mesos ReviewBot 

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


Patch looks great!

Reviews applied: [35473]

All tests passed.

- Mesos ReviewBot


On June 15, 2015, 10:03 p.m., Jie Yu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/35473/
> ---
> 
> (Updated June 15, 2015, 10:03 p.m.)
> 
> 
> Review request for mesos, Ben Mahler, Niklas Nielsen, and Vinod Kone.
> 
> 
> Bugs: MESOS-2627
> https://issues.apache.org/jira/browse/MESOS-2627
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Removed a few incorrect CHECKs in DRF sorter.
> 
> See details in the ticket for the reason why this is needed.
> 
> 
> Diffs
> -
> 
>   src/master/allocator/sorter/drf/sorter.cpp 
> ac05afdc7d408735dd796faa68c943e75540aaa7 
>   src/tests/examples_tests.cpp 41a9265e91ea54b657afbd32c22e204c5f64ca17 
> 
> Diff: https://reviews.apache.org/r/35473/diff/
> 
> 
> Testing
> ---
> 
> sudo make check
> 
> 
> Thanks,
> 
> Jie Yu
> 
>

Re: Review Request 35473: Removed a few incorrect CHECKs in DRF sorter.

2015-06-15 Thread Jie Yu 

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

(Updated June 15, 2015, 10:03 p.m.)


Review request for mesos, Ben Mahler, Niklas Nielsen, and Vinod Kone.


Changes
---

Review comments.


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


Repository: mesos


Description
---

Removed a few incorrect CHECKs in DRF sorter.

See details in the ticket for the reason why this is needed.


Diffs (updated)
-

  src/master/allocator/sorter/drf/sorter.cpp 
ac05afdc7d408735dd796faa68c943e75540aaa7 
  src/tests/examples_tests.cpp 41a9265e91ea54b657afbd32c22e204c5f64ca17 

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


Testing
---

sudo make check


Thanks,

Jie Yu

Re: Review Request 35473: Removed a few incorrect CHECKs in DRF sorter.

2015-06-15 Thread Ben Mahler 


> On June 15, 2015, 9:40 p.m., Ben Mahler wrote:
> > src/master/allocator/sorter/drf/sorter.cpp, lines 176-188
> > 
> >
> > How about the same logic in both?
> > 
> > ```
> > {
> >   if (_resources.empty()) {
> > // Do work.
> >   }
> > }
> > ```
> > 
> > In the case of remove(), we can have a CHECK that the slave id is 
> > contained in the map as part of the work?

Sorry:

```
{
  if (!_resources.empty()) {
// Do work.
  }
}
```


- Ben


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


On June 15, 2015, 9:15 p.m., Jie Yu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/35473/
> ---
> 
> (Updated June 15, 2015, 9:15 p.m.)
> 
> 
> Review request for mesos, Ben Mahler, Niklas Nielsen, and Vinod Kone.
> 
> 
> Bugs: MESOS-2627
> https://issues.apache.org/jira/browse/MESOS-2627
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Removed a few incorrect CHECKs in DRF sorter.
> 
> See details in the ticket for the reason why this is needed.
> 
> 
> Diffs
> -
> 
>   src/master/allocator/sorter/drf/sorter.cpp 
> ac05afdc7d408735dd796faa68c943e75540aaa7 
> 
> Diff: https://reviews.apache.org/r/35473/diff/
> 
> 
> Testing
> ---
> 
> sudo make check
> 
> 
> Thanks,
> 
> Jie Yu
> 
>

Re: Review Request 35473: Removed a few incorrect CHECKs in DRF sorter.

2015-06-15 Thread Ben Mahler 

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



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


How about the same logic in both?

```
{
  if (_resources.empty()) {
// Do work.
  }
}
```

In the case of remove(), we can have a CHECK that the slave id is contained 
in the map as part of the work?


- Ben Mahler


On June 15, 2015, 9:15 p.m., Jie Yu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/35473/
> ---
> 
> (Updated June 15, 2015, 9:15 p.m.)
> 
> 
> Review request for mesos, Ben Mahler, Niklas Nielsen, and Vinod Kone.
> 
> 
> Bugs: MESOS-2627
> https://issues.apache.org/jira/browse/MESOS-2627
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Removed a few incorrect CHECKs in DRF sorter.
> 
> See details in the ticket for the reason why this is needed.
> 
> 
> Diffs
> -
> 
>   src/master/allocator/sorter/drf/sorter.cpp 
> ac05afdc7d408735dd796faa68c943e75540aaa7 
> 
> Diff: https://reviews.apache.org/r/35473/diff/
> 
> 
> Testing
> ---
> 
> sudo make check
> 
> 
> Thanks,
> 
> Jie Yu
> 
>