You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@mesos.apache.org by Michael Park <mp...@apache.org> on 2017/06/30 09:02:15 UTC
Review Request 60564: Performed validation/upgrade of `Resource`
objects before authorization.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/60564/
-----------------------------------------------------------
Review request for mesos and Benjamin Mahler.
Bugs: MESOS-7735
https://issues.apache.org/jira/browse/MESOS-7735
Repository: mesos
Description
-------
See summary.
Diffs
-----
src/master/master.cpp 276b18e4d1bab7e9b28c1c93fe13c93a38abc5cd
src/tests/master_tests.cpp cfb799fd105e9880cd56415b2a84e604c8f62703
Diff: https://reviews.apache.org/r/60564/diff/1/
Testing
-------
Added a new test + `make check`
Thanks,
Michael Park
Re: Review Request 60564: Performed validation/upgrade of `Resource`
objects before authorization.
Posted by Benjamin Mahler <bm...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/60564/#review179464
-----------------------------------------------------------
Looks good, but I would really like to see the logic for dropping operations in accept() just once, rather than duplicating it here and the two being inconsistent.
src/master/master.cpp
Lines 3944 (patched)
<https://reviews.apache.org/r/60564/#comment254173>
It seems to me the `error.isSome()` handling above and the one here should be consistent, and can be merged into one? Both are responsible for dropping operations.
Note also that the duplication here is inconsistent, for example the above calls forward and increments dropped/lost metrics, but this one calls sendstatusupdates?
src/tests/master_tests.cpp
Lines 7588 (patched)
<https://reviews.apache.org/r/60564/#comment254174>
"such" means.. tasks that have post reservation refinement formatted resources?
- Benjamin Mahler
On June 30, 2017, 9:02 a.m., Michael Park wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/60564/
> -----------------------------------------------------------
>
> (Updated June 30, 2017, 9:02 a.m.)
>
>
> Review request for mesos and Benjamin Mahler.
>
>
> Bugs: MESOS-7735
> https://issues.apache.org/jira/browse/MESOS-7735
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Performed validation/upgrade of `Resource` objects before authorization.
>
>
> Diffs
> -----
>
> src/master/master.cpp 95e9691b3e3ef101e3e7ddc7a498d5c5bf2276e3
> src/tests/master_tests.cpp cfb799fd105e9880cd56415b2a84e604c8f62703
>
>
> Diff: https://reviews.apache.org/r/60564/diff/2/
>
>
> Testing
> -------
>
> Added a new test + `make check`
>
>
> Thanks,
>
> Michael Park
>
>
Re: Review Request 60564: Performed validation/upgrade of `Resource`
objects before authorization.
Posted by Vinod Kone <vi...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/60564/#review179452
-----------------------------------------------------------
src/master/master.cpp
Lines 3845-3856 (original), 3845-3856 (patched)
<https://reviews.apache.org/r/60564/#comment254148>
don't we need to do this (recovering of resources when offer is in error) in your below block as well?
src/master/master.cpp
Lines 3999 (patched)
<https://reviews.apache.org/r/60564/#comment254149>
see my comment above. you need to recover resources for dropped operations. can you add a test?
- Vinod Kone
On June 30, 2017, 9:02 a.m., Michael Park wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/60564/
> -----------------------------------------------------------
>
> (Updated June 30, 2017, 9:02 a.m.)
>
>
> Review request for mesos and Benjamin Mahler.
>
>
> Bugs: MESOS-7735
> https://issues.apache.org/jira/browse/MESOS-7735
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Performed validation/upgrade of `Resource` objects before authorization.
>
>
> Diffs
> -----
>
> src/master/master.cpp 95e9691b3e3ef101e3e7ddc7a498d5c5bf2276e3
> src/tests/master_tests.cpp cfb799fd105e9880cd56415b2a84e604c8f62703
>
>
> Diff: https://reviews.apache.org/r/60564/diff/2/
>
>
> Testing
> -------
>
> Added a new test + `make check`
>
>
> Thanks,
>
> Michael Park
>
>
Re: Review Request 60564: Performed validation/upgrade of `Resource`
objects before authorization.
Posted by Benjamin Bannier <be...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/60564/#review179399
-----------------------------------------------------------
Fix it, then Ship it!
src/master/master.cpp
Lines 3955-3978 (patched)
<https://reviews.apache.org/r/60564/#comment254079>
I find these nested `switch` statements (with `default` cases) pretty hard to read. What about pulling below `for` loop into a lambda, and calling it from explicit cases for `LAUNCH` or `LAUNCHGROUP` instead.
- Benjamin Bannier
On June 30, 2017, 11:02 a.m., Michael Park wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/60564/
> -----------------------------------------------------------
>
> (Updated June 30, 2017, 11:02 a.m.)
>
>
> Review request for mesos and Benjamin Mahler.
>
>
> Bugs: MESOS-7735
> https://issues.apache.org/jira/browse/MESOS-7735
>
>
> Repository: mesos
>
>
> Description
> -------
>
> See summary.
>
>
> Diffs
> -----
>
> src/master/master.cpp 276b18e4d1bab7e9b28c1c93fe13c93a38abc5cd
> src/tests/master_tests.cpp cfb799fd105e9880cd56415b2a84e604c8f62703
>
>
> Diff: https://reviews.apache.org/r/60564/diff/1/
>
>
> Testing
> -------
>
> Added a new test + `make check`
>
>
> Thanks,
>
> Michael Park
>
>