You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@mesos.apache.org by Guangya Liu <gy...@gmail.com> on 2016/01/11 02:47:41 UTC

Review Request 42123: Enabled load qos controller use USAGE SLACK revocable resources.

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

Review request for mesos, Ben Mahler, Artem Harutyunyan, Joris Van Remoortere, Joseph Wu, Klaus Ma, and Jian Qiu.


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


Repository: mesos


Description
-------

The current load qos controller is calculating executor used
resources via revocable() API, but this is not right as the
revocable() will include both USAGE SLACK and ALLOCATION SLACK.

This patch is updating the load qos controller only get USAGE
SLACK revocable resources for executors.

The unit test was already covered in oversubscription_tests.cpp.


Diffs
-----

  src/slave/qos_controllers/load.cpp 52520d6220784423c09001b8bd59e090ebf787ec 

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


Testing
-------

make
make check


Thanks,

Guangya Liu


Re: Review Request 42123: Enabled load qos controller use USAGE SLACK revocable resources.

Posted by Klaus Ma <kl...@gmail.com>.

> On Jan. 11, 2016, 11:33 a.m., Klaus Ma wrote:
> > Would you add test case for that? It seems `make check` will also pass without this patch :).
> 
> Guangya Liu wrote:
>     I think that we need to update oversubscription_tests.cpp to use usageSlack() instead of revocable() to get usage slack resources which is more accurate.

That's not enough; after updating to use usageSlack(), the case in this patch is still not covered :). I'm going to add some mix case (both `USAGE_SLACK` & `ALLOCATION_SLACK` are reported) to catch such kind of issue.


- Klaus


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


On Jan. 11, 2016, 9:56 a.m., Guangya Liu wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/42123/
> -----------------------------------------------------------
> 
> (Updated Jan. 11, 2016, 9:56 a.m.)
> 
> 
> Review request for mesos, Ben Mahler, Artem Harutyunyan, Joris Van Remoortere, Joseph Wu, Klaus Ma, and Jian Qiu.
> 
> 
> Bugs: MESOS-4322
>     https://issues.apache.org/jira/browse/MESOS-4322
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> The current load qos controller is calculating executor used
> resources via revocable() API, but this is not right as the
> revocable() will include both USAGE SLACK and ALLOCATION SLACK.
> 
> This patch is updating the load qos controller only get USAGE
> SLACK revocable resources for executors.
> 
> The unit test was already covered in oversubscription_tests.cpp.
> 
> 
> Diffs
> -----
> 
>   src/slave/qos_controllers/load.cpp 52520d6220784423c09001b8bd59e090ebf787ec 
> 
> Diff: https://reviews.apache.org/r/42123/diff/
> 
> 
> Testing
> -------
> 
> make
> make check
> 
> 
> Thanks,
> 
> Guangya Liu
> 
>


Re: Review Request 42123: Enabled load qos controller use USAGE SLACK revocable resources.

Posted by Guangya Liu <gy...@gmail.com>.

> On 一月 11, 2016, 3:33 a.m., Klaus Ma wrote:
> > Would you add test case for that? It seems `make check` will also pass without this patch :).

I think that we need to update oversubscription_tests.cpp to use usageSlack() instead of revocable() to get usage slack resources which is more accurate.


- Guangya


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


On 一月 11, 2016, 1:56 a.m., Guangya Liu wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/42123/
> -----------------------------------------------------------
> 
> (Updated 一月 11, 2016, 1:56 a.m.)
> 
> 
> Review request for mesos, Ben Mahler, Artem Harutyunyan, Joris Van Remoortere, Joseph Wu, Klaus Ma, and Jian Qiu.
> 
> 
> Bugs: MESOS-4322
>     https://issues.apache.org/jira/browse/MESOS-4322
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> The current load qos controller is calculating executor used
> resources via revocable() API, but this is not right as the
> revocable() will include both USAGE SLACK and ALLOCATION SLACK.
> 
> This patch is updating the load qos controller only get USAGE
> SLACK revocable resources for executors.
> 
> The unit test was already covered in oversubscription_tests.cpp.
> 
> 
> Diffs
> -----
> 
>   src/slave/qos_controllers/load.cpp 52520d6220784423c09001b8bd59e090ebf787ec 
> 
> Diff: https://reviews.apache.org/r/42123/diff/
> 
> 
> Testing
> -------
> 
> make
> make check
> 
> 
> Thanks,
> 
> Guangya Liu
> 
>


Re: Review Request 42123: Enabled load qos controller use USAGE SLACK revocable resources.

Posted by Guangya Liu <gy...@gmail.com>.

> On 一月 11, 2016, 3:33 a.m., Klaus Ma wrote:
> > Would you add test case for that? It seems `make check` will also pass without this patch :).
> 
> Guangya Liu wrote:
>     I think that we need to update oversubscription_tests.cpp to use usageSlack() instead of revocable() to get usage slack resources which is more accurate.
> 
> Klaus Ma wrote:
>     That's not enough; after updating to use usageSlack(), the case in this patch is still not covered :). I'm going to add some mix case (both `USAGE_SLACK` & `ALLOCATION_SLACK` are reported) to catch such kind of issue.

Does there are any cases which makes resources estimator report some allocation slack resources?


- Guangya


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


On 一月 11, 2016, 1:56 a.m., Guangya Liu wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/42123/
> -----------------------------------------------------------
> 
> (Updated 一月 11, 2016, 1:56 a.m.)
> 
> 
> Review request for mesos, Ben Mahler, Artem Harutyunyan, Joris Van Remoortere, Joseph Wu, Klaus Ma, and Jian Qiu.
> 
> 
> Bugs: MESOS-4322
>     https://issues.apache.org/jira/browse/MESOS-4322
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> The current load qos controller is calculating executor used
> resources via revocable() API, but this is not right as the
> revocable() will include both USAGE SLACK and ALLOCATION SLACK.
> 
> This patch is updating the load qos controller only get USAGE
> SLACK revocable resources for executors.
> 
> The unit test was already covered in oversubscription_tests.cpp.
> 
> 
> Diffs
> -----
> 
>   src/slave/qos_controllers/load.cpp 52520d6220784423c09001b8bd59e090ebf787ec 
> 
> Diff: https://reviews.apache.org/r/42123/diff/
> 
> 
> Testing
> -------
> 
> make
> make check
> 
> 
> Thanks,
> 
> Guangya Liu
> 
>


Re: Review Request 42123: Enabled load qos controller use USAGE SLACK revocable resources.

Posted by Klaus Ma <kl...@gmail.com>.

> On Jan. 11, 2016, 11:33 a.m., Klaus Ma wrote:
> > Would you add test case for that? It seems `make check` will also pass without this patch :).
> 
> Guangya Liu wrote:
>     I think that we need to update oversubscription_tests.cpp to use usageSlack() instead of revocable() to get usage slack resources which is more accurate.
> 
> Klaus Ma wrote:
>     That's not enough; after updating to use usageSlack(), the case in this patch is still not covered :). I'm going to add some mix case (both `USAGE_SLACK` & `ALLOCATION_SLACK` are reported) to catch such kind of issue.
> 
> Guangya Liu wrote:
>     Does there are any cases which makes resources estimator report some allocation slack resources?

Noop, but we need some cases to launch tasks on both `USAGE_SLACK` & `ALLOCATION_SLACK`. Current test case for Oversubscription only launch task on `USAGE_SLACK`, so no case failed. Anyway, update the related code to use `usageSlack()` make code more clear.


- Klaus


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


On Jan. 11, 2016, 9:56 a.m., Guangya Liu wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/42123/
> -----------------------------------------------------------
> 
> (Updated Jan. 11, 2016, 9:56 a.m.)
> 
> 
> Review request for mesos, Ben Mahler, Artem Harutyunyan, Joris Van Remoortere, Joseph Wu, Klaus Ma, and Jian Qiu.
> 
> 
> Bugs: MESOS-4322
>     https://issues.apache.org/jira/browse/MESOS-4322
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> The current load qos controller is calculating executor used
> resources via revocable() API, but this is not right as the
> revocable() will include both USAGE SLACK and ALLOCATION SLACK.
> 
> This patch is updating the load qos controller only get USAGE
> SLACK revocable resources for executors.
> 
> The unit test was already covered in oversubscription_tests.cpp.
> 
> 
> Diffs
> -----
> 
>   src/slave/qos_controllers/load.cpp 52520d6220784423c09001b8bd59e090ebf787ec 
> 
> Diff: https://reviews.apache.org/r/42123/diff/
> 
> 
> Testing
> -------
> 
> make
> make check
> 
> 
> Thanks,
> 
> Guangya Liu
> 
>


Re: Review Request 42123: Enabled load qos controller use USAGE SLACK revocable resources.

Posted by Klaus Ma <kl...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/42123/#review113670
-----------------------------------------------------------


Would you add test case for that? It seems `make check` will also pass without this patch :).

- Klaus Ma


On Jan. 11, 2016, 9:56 a.m., Guangya Liu wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/42123/
> -----------------------------------------------------------
> 
> (Updated Jan. 11, 2016, 9:56 a.m.)
> 
> 
> Review request for mesos, Ben Mahler, Artem Harutyunyan, Joris Van Remoortere, Joseph Wu, Klaus Ma, and Jian Qiu.
> 
> 
> Bugs: MESOS-4322
>     https://issues.apache.org/jira/browse/MESOS-4322
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> The current load qos controller is calculating executor used
> resources via revocable() API, but this is not right as the
> revocable() will include both USAGE SLACK and ALLOCATION SLACK.
> 
> This patch is updating the load qos controller only get USAGE
> SLACK revocable resources for executors.
> 
> The unit test was already covered in oversubscription_tests.cpp.
> 
> 
> Diffs
> -----
> 
>   src/slave/qos_controllers/load.cpp 52520d6220784423c09001b8bd59e090ebf787ec 
> 
> Diff: https://reviews.apache.org/r/42123/diff/
> 
> 
> Testing
> -------
> 
> make
> make check
> 
> 
> Thanks,
> 
> Guangya Liu
> 
>


Re: Review Request 42123: Enabled load qos controller use USAGE SLACK revocable resources.

Posted by Guangya Liu <gy...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/42123/
-----------------------------------------------------------

(Updated 一月 23, 2016, 6:08 a.m.)


Review request for mesos, Ben Mahler, Artem Harutyunyan, Joris Van Remoortere, Joseph Wu, Klaus Ma, and Jian Qiu.


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


Repository: mesos


Description
-------

The current load qos controller is calculating executor used
resources via revocable() API, but this is not right as the
revocable() will include both USAGE SLACK and ALLOCATION SLACK.

This patch is updating the load qos controller only get USAGE
SLACK revocable resources for executors.

The unit test was already covered in oversubscription_tests.cpp.


Diffs (updated)
-----

  src/slave/qos_controllers/load.cpp 52520d6220784423c09001b8bd59e090ebf787ec 
  src/tests/oversubscription_tests.cpp 6f43103e81303015fb614653e3bfece55009d1bf 

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


Testing
-------

make
make check


Thanks,

Guangya Liu


Re: Review Request 42123: Enabled load qos controller use USAGE SLACK revocable resources.

Posted by Guangya Liu <gy...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/42123/
-----------------------------------------------------------

(Updated 一月 16, 2016, 8 a.m.)


Review request for mesos, Ben Mahler, Artem Harutyunyan, Joris Van Remoortere, Joseph Wu, Klaus Ma, and Jian Qiu.


Changes
-------

Updated unit test.


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


Repository: mesos


Description
-------

The current load qos controller is calculating executor used
resources via revocable() API, but this is not right as the
revocable() will include both USAGE SLACK and ALLOCATION SLACK.

This patch is updating the load qos controller only get USAGE
SLACK revocable resources for executors.

The unit test was already covered in oversubscription_tests.cpp.


Diffs (updated)
-----

  src/slave/qos_controllers/load.cpp 52520d6220784423c09001b8bd59e090ebf787ec 
  src/tests/oversubscription_tests.cpp 7a75fb38e0177e33cf0e7cb82b4b9ebf8f05fe0a 

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


Testing
-------

make
make check


Thanks,

Guangya Liu


Re: Review Request 42123: Enabled load qos controller use USAGE SLACK revocable resources.

Posted by Guangya Liu <gy...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/42123/
-----------------------------------------------------------

(Updated 一月 11, 2016, 1:56 a.m.)


Review request for mesos, Ben Mahler, Artem Harutyunyan, Joris Van Remoortere, Joseph Wu, Klaus Ma, and Jian Qiu.


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


Repository: mesos


Description
-------

The current load qos controller is calculating executor used
resources via revocable() API, but this is not right as the
revocable() will include both USAGE SLACK and ALLOCATION SLACK.

This patch is updating the load qos controller only get USAGE
SLACK revocable resources for executors.

The unit test was already covered in oversubscription_tests.cpp.


Diffs (updated)
-----

  src/slave/qos_controllers/load.cpp 52520d6220784423c09001b8bd59e090ebf787ec 

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


Testing
-------

make
make check


Thanks,

Guangya Liu