You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@mesos.apache.org by Benjamin Mahler <bm...@apache.org> on 2019/03/22 15:54:33 UTC
Review Request 70279: Updated quota overcommit message to include
total quota and capacity.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/70279/
-----------------------------------------------------------
Review request for mesos, Benno Evers and Meng Zhu.
Bugs: MESOS-9292
https://issues.apache.org/jira/browse/MESOS-9292
Repository: mesos
Description
-------
Per MESOS-9292, users would find it helpful for the quota overcommit
error message to include more information about the cluster capactiy
vs total quota guarantees.
Diffs
-----
src/master/quota_handler.cpp 2dcfdcaf836f21d7d39b5ef5c36de0db25ca7517
src/tests/master_quota_tests.cpp 3e7a94cad94b64faf01e73f880281d24098fadbd
Diff: https://reviews.apache.org/r/70279/diff/1/
Testing
-------
Updated a test.
Thanks,
Benjamin Mahler
Re: Review Request 70279: Updated quota overcommit message to include
total quota and capacity.
Posted by Mesos Reviewbot Windows <re...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/70279/#review213922
-----------------------------------------------------------
PASS: Mesos patch 70279 was successfully built and tested.
Reviews applied: `['70272', '70273', '70274', '70275', '70276', '70277', '70278', '70279']`
All the build artifacts available at: http://dcos-win.westus2.cloudapp.azure.com/artifacts/mesos-reviewbot-testing/2994/mesos-review-70279
- Mesos Reviewbot Windows
On March 22, 2019, 3:54 p.m., Benjamin Mahler wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/70279/
> -----------------------------------------------------------
>
> (Updated March 22, 2019, 3:54 p.m.)
>
>
> Review request for mesos, Benno Evers and Meng Zhu.
>
>
> Bugs: MESOS-9292
> https://issues.apache.org/jira/browse/MESOS-9292
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Per MESOS-9292, users would find it helpful for the quota overcommit
> error message to include more information about the cluster capactiy
> vs total quota guarantees.
>
>
> Diffs
> -----
>
> src/master/quota_handler.cpp 2dcfdcaf836f21d7d39b5ef5c36de0db25ca7517
> src/tests/master_quota_tests.cpp 3e7a94cad94b64faf01e73f880281d24098fadbd
>
>
> Diff: https://reviews.apache.org/r/70279/diff/1/
>
>
> Testing
> -------
>
> Updated a test.
>
>
> Thanks,
>
> Benjamin Mahler
>
>
Re: Review Request 70279: Updated quota overcommit message to include
total quota and capacity.
Posted by Benjamin Mahler <bm...@apache.org>.
> On March 25, 2019, 12:08 a.m., Meng Zhu wrote:
> > src/master/quota_handler.cpp
> > Lines 223-225 (original), 223-228 (patched)
> > <https://reviews.apache.org/r/70279/diff/1/?file=2133535#file2133535line223>
> >
> > The TODO is a good point, I think we can just include the existing portion and the new portion in the message:
> >
> > "Total quota guarantees exceed cluster capacity '" + stringify(...) + "': existing quota guarantee '" + stringify(...) + "', new guarantee request '" + stringify(...) + "'"
The reason I left it as a TODO was because I needed to update `QuotaTree` to support removal or updating of an existing quota. That would allow us to efficiently compute the old and new quota at the start of this function without having to build two `QuotaTree`s for before vs after, which seemed cumbersome and inefficient.
Did you have a specific code suggestion for this patch? (e.g. let's build a second quota tree before printing the error and follow up by removing it) Or do you agree we should follow up by enhancing `QuotaTree` and then enhacing this message? I'm ok either way
- Benjamin
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/70279/#review213962
-----------------------------------------------------------
On March 22, 2019, 3:54 p.m., Benjamin Mahler wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/70279/
> -----------------------------------------------------------
>
> (Updated March 22, 2019, 3:54 p.m.)
>
>
> Review request for mesos, Benno Evers and Meng Zhu.
>
>
> Bugs: MESOS-9292
> https://issues.apache.org/jira/browse/MESOS-9292
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Per MESOS-9292, users would find it helpful for the quota overcommit
> error message to include more information about the cluster capactiy
> vs total quota guarantees.
>
>
> Diffs
> -----
>
> src/master/quota_handler.cpp 2dcfdcaf836f21d7d39b5ef5c36de0db25ca7517
> src/tests/master_quota_tests.cpp 3e7a94cad94b64faf01e73f880281d24098fadbd
>
>
> Diff: https://reviews.apache.org/r/70279/diff/1/
>
>
> Testing
> -------
>
> Updated a test.
>
>
> Thanks,
>
> Benjamin Mahler
>
>
Re: Review Request 70279: Updated quota overcommit message to include
total quota and capacity.
Posted by Meng Zhu <mz...@mesosphere.io>.
> On March 24, 2019, 5:08 p.m., Meng Zhu wrote:
> > src/master/quota_handler.cpp
> > Lines 223-225 (original), 223-228 (patched)
> > <https://reviews.apache.org/r/70279/diff/1/?file=2133535#file2133535line223>
> >
> > The TODO is a good point, I think we can just include the existing portion and the new portion in the message:
> >
> > "Total quota guarantees exceed cluster capacity '" + stringify(...) + "': existing quota guarantee '" + stringify(...) + "', new guarantee request '" + stringify(...) + "'"
>
> Benjamin Mahler wrote:
> The reason I left it as a TODO was because I needed to update `QuotaTree` to support removal or updating of an existing quota. That would allow us to efficiently compute the old and new quota at the start of this function without having to build two `QuotaTree`s for before vs after, which seemed cumbersome and inefficient.
>
> Did you have a specific code suggestion for this patch? (e.g. let's build a second quota tree before printing the error and follow up by removing it) Or do you agree we should follow up by enhancing `QuotaTree` and then enhacing this message? I'm ok either way
Ah, I forgot that we can't update the quotaTree at the moment. Let me drop this and you second approach (update the quota tree sounds good to me).
- Meng
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/70279/#review213962
-----------------------------------------------------------
On March 22, 2019, 8:54 a.m., Benjamin Mahler wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/70279/
> -----------------------------------------------------------
>
> (Updated March 22, 2019, 8:54 a.m.)
>
>
> Review request for mesos, Benno Evers and Meng Zhu.
>
>
> Bugs: MESOS-9292
> https://issues.apache.org/jira/browse/MESOS-9292
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Per MESOS-9292, users would find it helpful for the quota overcommit
> error message to include more information about the cluster capactiy
> vs total quota guarantees.
>
>
> Diffs
> -----
>
> src/master/quota_handler.cpp 2dcfdcaf836f21d7d39b5ef5c36de0db25ca7517
> src/tests/master_quota_tests.cpp 3e7a94cad94b64faf01e73f880281d24098fadbd
>
>
> Diff: https://reviews.apache.org/r/70279/diff/1/
>
>
> Testing
> -------
>
> Updated a test.
>
>
> Thanks,
>
> Benjamin Mahler
>
>
Re: Review Request 70279: Updated quota overcommit message to include
total quota and capacity.
Posted by Meng Zhu <mz...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/70279/#review213962
-----------------------------------------------------------
Fix it, then Ship it!
src/master/quota_handler.cpp
Lines 223-225 (original), 223-228 (patched)
<https://reviews.apache.org/r/70279/#comment300079>
The TODO is a good point, I think we can just include the existing portion and the new portion in the message:
"Total quota guarantees exceed cluster capacity '" + stringify(...) + "': existing quota guarantee '" + stringify(...) + "', new guarantee request '" + stringify(...) + "'"
- Meng Zhu
On March 22, 2019, 8:54 a.m., Benjamin Mahler wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/70279/
> -----------------------------------------------------------
>
> (Updated March 22, 2019, 8:54 a.m.)
>
>
> Review request for mesos, Benno Evers and Meng Zhu.
>
>
> Bugs: MESOS-9292
> https://issues.apache.org/jira/browse/MESOS-9292
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Per MESOS-9292, users would find it helpful for the quota overcommit
> error message to include more information about the cluster capactiy
> vs total quota guarantees.
>
>
> Diffs
> -----
>
> src/master/quota_handler.cpp 2dcfdcaf836f21d7d39b5ef5c36de0db25ca7517
> src/tests/master_quota_tests.cpp 3e7a94cad94b64faf01e73f880281d24098fadbd
>
>
> Diff: https://reviews.apache.org/r/70279/diff/1/
>
>
> Testing
> -------
>
> Updated a test.
>
>
> Thanks,
>
> Benjamin Mahler
>
>
Re: Review Request 70279: Updated quota overcommit message to include
total quota and capacity.
Posted by Mesos Reviewbot <re...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/70279/#review213923
-----------------------------------------------------------
Patch looks great!
Reviews applied: [70272, 70273, 70274, 70275, 70276, 70277, 70278, 70279]
Passed command: export OS='ubuntu:14.04' BUILDTOOL='autotools' COMPILER='gcc' CONFIGURATION='--verbose --disable-libtool-wrappers --disable-parallel-test-execution' ENVIRONMENT='GLOG_v=1 MESOS_VERBOSE=1'; ./support/docker-build.sh
- Mesos Reviewbot
On March 22, 2019, 3:54 p.m., Benjamin Mahler wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/70279/
> -----------------------------------------------------------
>
> (Updated March 22, 2019, 3:54 p.m.)
>
>
> Review request for mesos, Benno Evers and Meng Zhu.
>
>
> Bugs: MESOS-9292
> https://issues.apache.org/jira/browse/MESOS-9292
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Per MESOS-9292, users would find it helpful for the quota overcommit
> error message to include more information about the cluster capactiy
> vs total quota guarantees.
>
>
> Diffs
> -----
>
> src/master/quota_handler.cpp 2dcfdcaf836f21d7d39b5ef5c36de0db25ca7517
> src/tests/master_quota_tests.cpp 3e7a94cad94b64faf01e73f880281d24098fadbd
>
>
> Diff: https://reviews.apache.org/r/70279/diff/1/
>
>
> Testing
> -------
>
> Updated a test.
>
>
> Thanks,
>
> Benjamin Mahler
>
>