You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@mesos.apache.org by Zhitao Li <zh...@gmail.com> on 2016/04/08 22:02:47 UTC
Review Request 45941: Add check in agent for incorrect oversubscribed
resource.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/45941/
-----------------------------------------------------------
Review request for mesos, Ben Mahler and Joseph Wu.
Bugs: MESOS-5131
https://issues.apache.org/jira/browse/MESOS-5131
Repository: mesos
Description
-------
Add check in agent for incorrect oversubscribed resource.
I've decided to let agent crash explicitly here instead of fail more silently.
Diffs
-----
src/slave/slave.cpp f090c853b8affc4be5eecb4f616ec881fc2b60c3
Diff: https://reviews.apache.org/r/45941/diff/
Testing
-------
Running existing test, and verify manually that offending resource crashes the agent.
(Any suggestion to test `CHECK` is welcomed).
Thanks,
Zhitao Li
Re: Review Request 45941: Add check in agent for incorrect
oversubscribed resource.
Posted by Zhitao Li <zh...@gmail.com>.
> On April 8, 2016, 11:03 p.m., Ben Mahler wrote:
> > src/slave/slave.cpp, lines 4989-4991
> > <https://reviews.apache.org/r/45941/diff/1/?file=1337482#file1337482line4989>
> >
> > Any reason why this CHECK isn't below the VLOG(1) above? Having it above seems to make it more clear that we're validating the input from the estimator, no?
> >
> > Also, for the comment here, we should avoid talking about the hierarchical allocator since that just happens to be where the failure manifests.
> >
> > Perhaps something like:
> >
> > ```
> > // Oversubscrbable resources must be considered revocable.
> > //
> > // TODO(bmahler): Consider tagging input as revocable
> > // rather than rejecting and crashing here.
> > ```
I'll move it.
- Zhitao
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/45941/#review127914
-----------------------------------------------------------
On April 8, 2016, 8:02 p.m., Zhitao Li wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/45941/
> -----------------------------------------------------------
>
> (Updated April 8, 2016, 8:02 p.m.)
>
>
> Review request for mesos, Ben Mahler and Joseph Wu.
>
>
> Bugs: MESOS-5131
> https://issues.apache.org/jira/browse/MESOS-5131
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Add check in agent for incorrect oversubscribed resource.
>
> I've decided to let agent crash explicitly here instead of fail more silently.
>
>
> Diffs
> -----
>
> src/slave/slave.cpp f090c853b8affc4be5eecb4f616ec881fc2b60c3
>
> Diff: https://reviews.apache.org/r/45941/diff/
>
>
> Testing
> -------
>
> Running existing test, and verify manually that offending resource crashes the agent.
>
> (Any suggestion to test `CHECK` is welcomed).
>
>
> Thanks,
>
> Zhitao Li
>
>
Re: Review Request 45941: Add check in agent for incorrect
oversubscribed resource.
Posted by Ben Mahler <be...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/45941/#review127914
-----------------------------------------------------------
src/slave/slave.cpp (lines 4989 - 4991)
<https://reviews.apache.org/r/45941/#comment191288>
Any reason why this CHECK isn't below the VLOG(1) above? Having it above seems to make it more clear that we're validating the input from the estimator, no?
Also, for the comment here, we should avoid talking about the hierarchical allocator since that just happens to be where the failure manifests.
Perhaps something like:
```
// Oversubscrbable resources must be considered revocable.
//
// TODO(bmahler): Consider tagging input as revocable
// rather than rejecting and crashing here.
```
- Ben Mahler
On April 8, 2016, 8:02 p.m., Zhitao Li wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/45941/
> -----------------------------------------------------------
>
> (Updated April 8, 2016, 8:02 p.m.)
>
>
> Review request for mesos, Ben Mahler and Joseph Wu.
>
>
> Bugs: MESOS-5131
> https://issues.apache.org/jira/browse/MESOS-5131
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Add check in agent for incorrect oversubscribed resource.
>
> I've decided to let agent crash explicitly here instead of fail more silently.
>
>
> Diffs
> -----
>
> src/slave/slave.cpp f090c853b8affc4be5eecb4f616ec881fc2b60c3
>
> Diff: https://reviews.apache.org/r/45941/diff/
>
>
> Testing
> -------
>
> Running existing test, and verify manually that offending resource crashes the agent.
>
> (Any suggestion to test `CHECK` is welcomed).
>
>
> Thanks,
>
> Zhitao Li
>
>
Re: Review Request 45941: Add check in agent for incorrect
oversubscribed resource.
Posted by Mesos ReviewBot <re...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/45941/#review128005
-----------------------------------------------------------
Patch looks great!
Reviews applied: [45941]
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 April 9, 2016, 5:20 p.m., Zhitao Li wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/45941/
> -----------------------------------------------------------
>
> (Updated April 9, 2016, 5:20 p.m.)
>
>
> Review request for mesos, Ben Mahler and Joseph Wu.
>
>
> Bugs: MESOS-5131
> https://issues.apache.org/jira/browse/MESOS-5131
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Add check in agent for incorrect oversubscribed resource.
>
>
> Diffs
> -----
>
> src/slave/slave.cpp f090c853b8affc4be5eecb4f616ec881fc2b60c3
>
> Diff: https://reviews.apache.org/r/45941/diff/
>
>
> Testing
> -------
>
> Running existing test, and verify manually that offending resource crashes the agent.
>
> (Any suggestion to test `CHECK` is welcomed).
>
>
> Thanks,
>
> Zhitao Li
>
>
Re: Review Request 45941: Add check in agent for incorrect
oversubscribed resource.
Posted by Ben Mahler <be...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/45941/#review128502
-----------------------------------------------------------
Fix it, then Ship it!
Thanks! Will get this committed shortly.
src/slave/slave.cpp (line 4972)
<https://reviews.apache.org/r/45941/#comment191911>
newline here
missing an 'i' in 'Oversubscrbable'
src/slave/slave.cpp (line 4976)
<https://reviews.apache.org/r/45941/#comment191912>
s/.get()./->/
- Ben Mahler
On April 9, 2016, 5:20 p.m., Zhitao Li wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/45941/
> -----------------------------------------------------------
>
> (Updated April 9, 2016, 5:20 p.m.)
>
>
> Review request for mesos, Ben Mahler and Joseph Wu.
>
>
> Bugs: MESOS-5131
> https://issues.apache.org/jira/browse/MESOS-5131
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Add check in agent for incorrect oversubscribed resource.
>
>
> Diffs
> -----
>
> src/slave/slave.cpp f090c853b8affc4be5eecb4f616ec881fc2b60c3
>
> Diff: https://reviews.apache.org/r/45941/diff/
>
>
> Testing
> -------
>
> Running existing test, and verify manually that offending resource crashes the agent.
>
> (Any suggestion to test `CHECK` is welcomed).
>
>
> Thanks,
>
> Zhitao Li
>
>
Re: Review Request 45941: Add check in agent for incorrect
oversubscribed resource.
Posted by Zhitao Li <zh...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/45941/
-----------------------------------------------------------
(Updated April 9, 2016, 5:20 p.m.)
Review request for mesos, Ben Mahler and Joseph Wu.
Changes
-------
Move check location.
Bugs: MESOS-5131
https://issues.apache.org/jira/browse/MESOS-5131
Repository: mesos
Description (updated)
-------
Add check in agent for incorrect oversubscribed resource.
Diffs (updated)
-----
src/slave/slave.cpp f090c853b8affc4be5eecb4f616ec881fc2b60c3
Diff: https://reviews.apache.org/r/45941/diff/
Testing
-------
Running existing test, and verify manually that offending resource crashes the agent.
(Any suggestion to test `CHECK` is welcomed).
Thanks,
Zhitao Li