Re: Review Request 47258: Make sure 'activate' is a no-op if the client is already active.

2016-06-07 Thread Joris Van Remoortere 

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


Ship it!




Ship It!

- Joris Van Remoortere


On May 19, 2016, 6:27 p.m., Jiang Yan Xu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/47258/
> ---
> 
> (Updated May 19, 2016, 6:27 p.m.)
> 
> 
> Review request for mesos, Dario Rexin and Joris Van Remoortere.
> 
> 
> Bugs: MESOS-5279
> https://issues.apache.org/jira/browse/MESOS-5279
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Make sure 'activate' is a no-op if the client is already active.
> 
> 
> Diffs
> -
> 
>   src/master/allocator/sorter/drf/sorter.cpp 
> 4306973277b9d32356eed31ceabac09fb2a03e6c 
>   src/master/allocator/sorter/sorter.hpp 
> 9e04adf54f2d80541a95f0a9a49b329dc9e8f5e3 
> 
> Diff: https://reviews.apache.org/r/47258/diff/
> 
> 
> Testing
> ---
> 
> make check.
> 
> 
> Thanks,
> 
> Jiang Yan Xu
> 
>

Re: Review Request 47258: Make sure 'activate' is a no-op if the client is already active.

2016-05-26 Thread Jiang Yan Xu 


> On May 25, 2016, 1:24 p.m., Joris Van Remoortere wrote:
> > src/master/allocator/sorter/drf/sorter.cpp, lines 90-91
> > 
> >
> > Is this a guard as opposed to a `CHECK` because someone is using this 
> > API correctly and we are mitigating the performance impact of that?
> > 
> > We could argue that it shouldn't be a `CHECK` because it's not an 
> > internal variant; however, I would like to know if there's a higher level 
> > bug we should be fixing :-)
> 
> Jiang Yan Xu wrote:
> I don't think it should be a CHECK here but it should be a CHECK in 
> /r/47259/ as I commented on /r/47259/ for CHECK vs. if guard. 
> 
> The motivation for this patch is from debugging /r/43666/. 
> 
> > We could argue that it shouldn't be a CHECK because it's not an 
> internal variant;
> 
> I think this is exactly right. The "high-level bug" is probably that due 
> to the tight coupling between the allocator and the sorter we are forced to 
> CHECK public method arguments as well. I think we should miminize this.

Plus we can look at the 
[deactive](https://github.com/apache/mesos/blob/6ce476461f0fedfb4ed4e40c15f25bb79a39b0f3/src/master/allocator/sorter/drf/sorter.cpp#L95)
 as an example: if it's already not "active", deactivate is a no-op.


- Jiang Yan


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


On May 19, 2016, 11:27 a.m., Jiang Yan Xu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/47258/
> ---
> 
> (Updated May 19, 2016, 11:27 a.m.)
> 
> 
> Review request for mesos, Dario Rexin and Joris Van Remoortere.
> 
> 
> Bugs: MESOS-5279
> https://issues.apache.org/jira/browse/MESOS-5279
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Make sure 'activate' is a no-op if the client is already active.
> 
> 
> Diffs
> -
> 
>   src/master/allocator/sorter/drf/sorter.cpp 
> 4306973277b9d32356eed31ceabac09fb2a03e6c 
>   src/master/allocator/sorter/sorter.hpp 
> 9e04adf54f2d80541a95f0a9a49b329dc9e8f5e3 
> 
> Diff: https://reviews.apache.org/r/47258/diff/
> 
> 
> Testing
> ---
> 
> make check.
> 
> 
> Thanks,
> 
> Jiang Yan Xu
> 
>

Re: Review Request 47258: Make sure 'activate' is a no-op if the client is already active.

2016-05-25 Thread Jiang Yan Xu 


> On May 25, 2016, 1:24 p.m., Joris Van Remoortere wrote:
> > src/master/allocator/sorter/drf/sorter.cpp, lines 90-91
> > 
> >
> > Is this a guard as opposed to a `CHECK` because someone is using this 
> > API correctly and we are mitigating the performance impact of that?
> > 
> > We could argue that it shouldn't be a `CHECK` because it's not an 
> > internal variant; however, I would like to know if there's a higher level 
> > bug we should be fixing :-)

I don't think it should be a CHECK here but it should be a CHECK in /r/47259/ 
as I commented on /r/47259/ for CHECK vs. if guard. 

The motivation for this patch is from debugging /r/43666/. 

> We could argue that it shouldn't be a CHECK because it's not an internal 
> variant;

I think this is exactly right. The "high-level bug" is probably that due to the 
tight coupling between the allocator and the sorter we are forced to CHECK 
public method arguments as well. I think we should miminize this.


- Jiang Yan


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


On May 19, 2016, 11:27 a.m., Jiang Yan Xu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/47258/
> ---
> 
> (Updated May 19, 2016, 11:27 a.m.)
> 
> 
> Review request for mesos, Dario Rexin and Joris Van Remoortere.
> 
> 
> Bugs: MESOS-5279
> https://issues.apache.org/jira/browse/MESOS-5279
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Make sure 'activate' is a no-op if the client is already active.
> 
> 
> Diffs
> -
> 
>   src/master/allocator/sorter/drf/sorter.cpp 
> 4306973277b9d32356eed31ceabac09fb2a03e6c 
>   src/master/allocator/sorter/sorter.hpp 
> 9e04adf54f2d80541a95f0a9a49b329dc9e8f5e3 
> 
> Diff: https://reviews.apache.org/r/47258/diff/
> 
> 
> Testing
> ---
> 
> make check.
> 
> 
> Thanks,
> 
> Jiang Yan Xu
> 
>

Re: Review Request 47258: Make sure 'activate' is a no-op if the client is already active.

2016-05-25 Thread Joris Van Remoortere 

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




src/master/allocator/sorter/drf/sorter.cpp (lines 90 - 91)


Is this a guard as opposed to a `CHECK` because someone is using this API 
correctly and we are mitigating the performance impact of that?

We could argue that it shouldn't be a `CHECK` because it's not an internal 
variant; however, I would like to know if there's a higher level bug we should 
be fixing :-)


- Joris Van Remoortere


On May 19, 2016, 6:27 p.m., Jiang Yan Xu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/47258/
> ---
> 
> (Updated May 19, 2016, 6:27 p.m.)
> 
> 
> Review request for mesos, Dario Rexin and Joris Van Remoortere.
> 
> 
> Bugs: MESOS-5279
> https://issues.apache.org/jira/browse/MESOS-5279
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Make sure 'activate' is a no-op if the client is already active.
> 
> 
> Diffs
> -
> 
>   src/master/allocator/sorter/drf/sorter.cpp 
> 4306973277b9d32356eed31ceabac09fb2a03e6c 
>   src/master/allocator/sorter/sorter.hpp 
> 9e04adf54f2d80541a95f0a9a49b329dc9e8f5e3 
> 
> Diff: https://reviews.apache.org/r/47258/diff/
> 
> 
> Testing
> ---
> 
> make check.
> 
> 
> Thanks,
> 
> Jiang Yan Xu
> 
>

Re: Review Request 47258: Make sure 'activate' is a no-op if the client is already active.

2016-05-13 Thread Guangya Liu 

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




src/master/allocator/sorter/sorter.hpp (line 70)


s/already not/not


- Guangya Liu


On 五月 11, 2016, 9:15 p.m., Jiang Yan Xu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/47258/
> ---
> 
> (Updated 五月 11, 2016, 9:15 p.m.)
> 
> 
> Review request for mesos and Dario Rexin.
> 
> 
> Bugs: MESOS-5279
> https://issues.apache.org/jira/browse/MESOS-5279
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Make sure 'activate' is a no-op if the client is already active.
> 
> 
> Diffs
> -
> 
>   src/master/allocator/sorter/drf/sorter.cpp 
> 4306973277b9d32356eed31ceabac09fb2a03e6c 
>   src/master/allocator/sorter/sorter.hpp 
> 9e04adf54f2d80541a95f0a9a49b329dc9e8f5e3 
> 
> Diff: https://reviews.apache.org/r/47258/diff/
> 
> 
> Testing
> ---
> 
> make check.
> 
> 
> Thanks,
> 
> Jiang Yan Xu
> 
>

Re: Review Request 47258: Make sure 'activate' is a no-op if the client is already active.

2016-05-11 Thread Mesos ReviewBot 

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



Patch looks great!

Reviews applied: [47258]

Passed command: export OS='ubuntu:14.04' CONFIGURATION='--verbose' 
COMPILER='gcc' ENVIRONMENT='GLOG_v=1 MESOS_VERBOSE=1'; ./support/docker_build.sh

- Mesos ReviewBot


On May 11, 2016, 9:15 p.m., Jiang Yan Xu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/47258/
> ---
> 
> (Updated May 11, 2016, 9:15 p.m.)
> 
> 
> Review request for mesos and Dario Rexin.
> 
> 
> Bugs: MESOS-5279
> https://issues.apache.org/jira/browse/MESOS-5279
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Make sure 'activate' is a no-op if the client is already active.
> 
> 
> Diffs
> -
> 
>   src/master/allocator/sorter/drf/sorter.cpp 
> 4306973277b9d32356eed31ceabac09fb2a03e6c 
>   src/master/allocator/sorter/sorter.hpp 
> 9e04adf54f2d80541a95f0a9a49b329dc9e8f5e3 
> 
> Diff: https://reviews.apache.org/r/47258/diff/
> 
> 
> Testing
> ---
> 
> make check.
> 
> 
> Thanks,
> 
> Jiang Yan Xu
> 
>

Re: Review Request 47258: Make sure 'activate' is a no-op if the client is already active.

2016-05-11 Thread Jiang Yan Xu 

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

(Updated May 11, 2016, 2:15 p.m.)


Review request for mesos and Dario Rexin.


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


Repository: mesos


Description
---

Make sure 'activate' is a no-op if the client is already active.


Diffs
-

  src/master/allocator/sorter/drf/sorter.cpp 
4306973277b9d32356eed31ceabac09fb2a03e6c 
  src/master/allocator/sorter/sorter.hpp 
9e04adf54f2d80541a95f0a9a49b329dc9e8f5e3 

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


Testing
---

make check.


Thanks,

Jiang Yan Xu

Review Request 47258: Make sure 'activate' is a no-op if the client is already active.

2016-05-11 Thread Jiang Yan Xu 

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

Review request for mesos and Dario Rexin.


Repository: mesos


Description
---

Make sure 'activate' is a no-op if the client is already active.


Diffs
-

  src/master/allocator/sorter/drf/sorter.cpp 
4306973277b9d32356eed31ceabac09fb2a03e6c 
  src/master/allocator/sorter/sorter.hpp 
9e04adf54f2d80541a95f0a9a49b329dc9e8f5e3 

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


Testing
---

make check.


Thanks,

Jiang Yan Xu