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 Bannier <be...@mesosphere.io> on 2018/04/10 12:07:56 UTC
Re: Review Request 66310: Passed on registrar when constructing
resource provider manager.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/66310/
-----------------------------------------------------------
(Updated April 10, 2018, 2:07 p.m.)
Review request for mesos, Jie Yu and Jan Schlicht.
Changes
-------
Rebased.
Bugs: MESOS-8735
https://issues.apache.org/jira/browse/MESOS-8735
Repository: mesos
Description
-------
In order to support recovering resource provider manager information
in the future, we need to adjust the construction of the manager to be
able to consume a registrar.
This patch lays the groundwork by adjusting interfaces and their
usage; we will make use of the passed on information in a following
patch.
Diffs (updated)
-----
src/resource_provider/manager.hpp bc017fa3998b780cec82718cabe2a8b470a66db2
src/resource_provider/manager.cpp 2d3ffcdd01671f81729fe538348f2ca4e555eb4f
src/slave/slave.cpp e5d6c3fac5054a6b0a0b5b77abd850a35be6ccc5
src/tests/resource_provider_manager_tests.cpp c52541bf130ccf4795b989b53331176a64a097ea
Diff: https://reviews.apache.org/r/66310/diff/2/
Changes: https://reviews.apache.org/r/66310/diff/1-2/
Testing
-------
`make check`
Thanks,
Benjamin Bannier
Re: Review Request 66310: Passed on registrar when constructing
resource provider manager.
Posted by Benjamin Bannier <be...@mesosphere.io>.
> On April 19, 2018, 12:25 a.m., Chun-Hung Hsiao wrote:
> > src/resource_provider/manager.cpp
> > Line 160 (original), 161 (patched)
> > <https://reviews.apache.org/r/66310/diff/4/?file=2005447#file2005447line161>
> >
> > `Owned<Registrar>&& _registrar`?
https://reviews.apache.org/r/66309/#comment_rc282702-201502
> On April 19, 2018, 12:25 a.m., Chun-Hung Hsiao wrote:
> > src/slave/slave.cpp
> > Lines 8869-8871 (patched)
> > <https://reviews.apache.org/r/66310/diff/4/?file=2005448#file2005448line8869>
> >
> > The current implementation makes this assertion never fail ;)
That's exactly why this is an assertion, not a branch :D
- Benjamin
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/66310/#review201475
-----------------------------------------------------------
On April 19, 2018, 2:28 p.m., Benjamin Bannier wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/66310/
> -----------------------------------------------------------
>
> (Updated April 19, 2018, 2:28 p.m.)
>
>
> Review request for mesos, Chun-Hung Hsiao, Jie Yu, and Jan Schlicht.
>
>
> Bugs: MESOS-8735
> https://issues.apache.org/jira/browse/MESOS-8735
>
>
> Repository: mesos
>
>
> Description
> -------
>
> In order to support recovering resource provider manager information
> in the future, we need to adjust the construction of the manager to be
> able to consume a registrar.
>
> This patch lays the groundwork by adjusting interfaces and their
> usage; we will make use of the passed on information in a following
> patch.
>
>
> Diffs
> -----
>
> src/resource_provider/manager.hpp bc017fa3998b780cec82718cabe2a8b470a66db2
> src/resource_provider/manager.cpp 68e1866986bfb91bf8355099ee1f0a2a86aba39a
> src/slave/slave.cpp e5d6c3fac5054a6b0a0b5b77abd850a35be6ccc5
> src/tests/resource_provider_manager_tests.cpp c52541bf130ccf4795b989b53331176a64a097ea
>
>
> Diff: https://reviews.apache.org/r/66310/diff/5/
>
>
> Testing
> -------
>
> `make check`
>
>
> Thanks,
>
> Benjamin Bannier
>
>
Re: Review Request 66310: Passed on registrar when constructing
resource provider manager.
Posted by Chun-Hung Hsiao <ch...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/66310/#review201475
-----------------------------------------------------------
src/resource_provider/manager.hpp
Lines 42 (patched)
<https://reviews.apache.org/r/66310/#comment282722>
`process::Owned<mesos::resource_provider::Registrar>&& registrar`?
Also it seems to me that the `mesos::` prefix can be omitted.
src/resource_provider/manager.cpp
Line 160 (original), 161 (patched)
<https://reviews.apache.org/r/66310/#comment282723>
`Owned<Registrar>&& _registrar`?
src/slave/slave.cpp
Lines 8869-8871 (patched)
<https://reviews.apache.org/r/66310/#comment282730>
The current implementation makes this assertion never fail ;)
src/slave/slave.cpp
Lines 8873 (patched)
<https://reviews.apache.org/r/66310/#comment282731>
This is completely redundent because of line 8848.
- Chun-Hung Hsiao
On April 10, 2018, 12:07 p.m., Benjamin Bannier wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/66310/
> -----------------------------------------------------------
>
> (Updated April 10, 2018, 12:07 p.m.)
>
>
> Review request for mesos, Jie Yu and Jan Schlicht.
>
>
> Bugs: MESOS-8735
> https://issues.apache.org/jira/browse/MESOS-8735
>
>
> Repository: mesos
>
>
> Description
> -------
>
> In order to support recovering resource provider manager information
> in the future, we need to adjust the construction of the manager to be
> able to consume a registrar.
>
> This patch lays the groundwork by adjusting interfaces and their
> usage; we will make use of the passed on information in a following
> patch.
>
>
> Diffs
> -----
>
> src/resource_provider/manager.hpp bc017fa3998b780cec82718cabe2a8b470a66db2
> src/resource_provider/manager.cpp 68e1866986bfb91bf8355099ee1f0a2a86aba39a
> src/slave/slave.cpp e5d6c3fac5054a6b0a0b5b77abd850a35be6ccc5
> src/tests/resource_provider_manager_tests.cpp c52541bf130ccf4795b989b53331176a64a097ea
>
>
> Diff: https://reviews.apache.org/r/66310/diff/4/
>
>
> Testing
> -------
>
> `make check`
>
>
> Thanks,
>
> Benjamin Bannier
>
>
Re: Review Request 66310: Passed on registrar when constructing
resource provider manager.
Posted by Chun-Hung Hsiao <ch...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/66310/#review201606
-----------------------------------------------------------
Ship it!
Ship It!
- Chun-Hung Hsiao
On April 19, 2018, 12:28 p.m., Benjamin Bannier wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/66310/
> -----------------------------------------------------------
>
> (Updated April 19, 2018, 12:28 p.m.)
>
>
> Review request for mesos, Chun-Hung Hsiao, Jie Yu, and Jan Schlicht.
>
>
> Bugs: MESOS-8735
> https://issues.apache.org/jira/browse/MESOS-8735
>
>
> Repository: mesos
>
>
> Description
> -------
>
> In order to support recovering resource provider manager information
> in the future, we need to adjust the construction of the manager to be
> able to consume a registrar.
>
> This patch lays the groundwork by adjusting interfaces and their
> usage; we will make use of the passed on information in a following
> patch.
>
>
> Diffs
> -----
>
> src/resource_provider/manager.hpp bc017fa3998b780cec82718cabe2a8b470a66db2
> src/resource_provider/manager.cpp 68e1866986bfb91bf8355099ee1f0a2a86aba39a
> src/slave/slave.cpp e5d6c3fac5054a6b0a0b5b77abd850a35be6ccc5
> src/tests/resource_provider_manager_tests.cpp c52541bf130ccf4795b989b53331176a64a097ea
>
>
> Diff: https://reviews.apache.org/r/66310/diff/5/
>
>
> Testing
> -------
>
> `make check`
>
>
> Thanks,
>
> Benjamin Bannier
>
>
Re: Review Request 66310: Passed on registrar when constructing
resource provider manager.
Posted by Benjamin Bannier <be...@mesosphere.io>.
> On April 23, 2018, 3:05 a.m., Chun-Hung Hsiao wrote:
> > src/tests/resource_provider_manager_tests.cpp
> > Line 132 (original), 132 (patched)
> > <https://reviews.apache.org/r/66310/diff/6/?file=2007713#file2007713line132>
> >
> > Do you think it is reasonable to always require a registrar, i.e., disallow `nullptr` and use `InMemoryStore` here? Please drop this if you think we should allow `nullptr`.
There's a trade-off between simpler testing code and internal complexity. In this case I agree with you that we can simplify the implementation of the manager a lot by requiring users to always pass in a registry. Adjusted the code for that, also added some assertions.
- Benjamin
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/66310/#review201711
-----------------------------------------------------------
On April 23, 2018, 1:18 p.m., Benjamin Bannier wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/66310/
> -----------------------------------------------------------
>
> (Updated April 23, 2018, 1:18 p.m.)
>
>
> Review request for mesos, Chun-Hung Hsiao, Jie Yu, and Jan Schlicht.
>
>
> Bugs: MESOS-8735
> https://issues.apache.org/jira/browse/MESOS-8735
>
>
> Repository: mesos
>
>
> Description
> -------
>
> In order to support recovering resource provider manager information
> in the future, we need to adjust the construction of the manager to be
> able to consume a registrar.
>
> This patch lays the groundwork by adjusting interfaces and their
> usage; we will make use of the passed on information in a following
> patch.
>
>
> Diffs
> -----
>
> src/resource_provider/manager.hpp bc017fa3998b780cec82718cabe2a8b470a66db2
> src/resource_provider/manager.cpp 68e1866986bfb91bf8355099ee1f0a2a86aba39a
> src/resource_provider/registrar.cpp 92ef9aecb1e93d10f46e53984391558f9901a60b
> src/slave/slave.cpp 2b8c6e0771a58efacf26628abae6cbe46380a369
> src/tests/resource_provider_manager_tests.cpp c52541bf130ccf4795b989b53331176a64a097ea
>
>
> Diff: https://reviews.apache.org/r/66310/diff/7/
>
>
> Testing
> -------
>
> `make check`
>
>
> Thanks,
>
> Benjamin Bannier
>
>
Re: Review Request 66310: Passed on registrar when constructing
resource provider manager.
Posted by Chun-Hung Hsiao <ch...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/66310/#review201711
-----------------------------------------------------------
src/tests/resource_provider_manager_tests.cpp
Line 132 (original), 132 (patched)
<https://reviews.apache.org/r/66310/#comment283264>
Do you think it is reasonable to always require a registrar, i.e., disallow `nullptr` and use `InMemoryStore` here? Please drop this if you think we should allow `nullptr`.
- Chun-Hung Hsiao
On April 19, 2018, 12:28 p.m., Benjamin Bannier wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/66310/
> -----------------------------------------------------------
>
> (Updated April 19, 2018, 12:28 p.m.)
>
>
> Review request for mesos, Chun-Hung Hsiao, Jie Yu, and Jan Schlicht.
>
>
> Bugs: MESOS-8735
> https://issues.apache.org/jira/browse/MESOS-8735
>
>
> Repository: mesos
>
>
> Description
> -------
>
> In order to support recovering resource provider manager information
> in the future, we need to adjust the construction of the manager to be
> able to consume a registrar.
>
> This patch lays the groundwork by adjusting interfaces and their
> usage; we will make use of the passed on information in a following
> patch.
>
>
> Diffs
> -----
>
> src/resource_provider/manager.hpp bc017fa3998b780cec82718cabe2a8b470a66db2
> src/resource_provider/manager.cpp 68e1866986bfb91bf8355099ee1f0a2a86aba39a
> src/slave/slave.cpp e5d6c3fac5054a6b0a0b5b77abd850a35be6ccc5
> src/tests/resource_provider_manager_tests.cpp c52541bf130ccf4795b989b53331176a64a097ea
>
>
> Diff: https://reviews.apache.org/r/66310/diff/6/
>
>
> Testing
> -------
>
> `make check`
>
>
> Thanks,
>
> Benjamin Bannier
>
>
Re: Review Request 66310: Passed on registrar when constructing
resource provider manager.
Posted by Chun-Hung Hsiao <ch...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/66310/#review201753
-----------------------------------------------------------
Ship it!
Ship It!
- Chun-Hung Hsiao
On April 23, 2018, 11:18 a.m., Benjamin Bannier wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/66310/
> -----------------------------------------------------------
>
> (Updated April 23, 2018, 11:18 a.m.)
>
>
> Review request for mesos, Chun-Hung Hsiao, Jie Yu, and Jan Schlicht.
>
>
> Bugs: MESOS-8735
> https://issues.apache.org/jira/browse/MESOS-8735
>
>
> Repository: mesos
>
>
> Description
> -------
>
> In order to support recovering resource provider manager information
> in the future, we need to adjust the construction of the manager to be
> able to consume a registrar.
>
> This patch lays the groundwork by adjusting interfaces and their
> usage; we will make use of the passed on information in a following
> patch.
>
>
> Diffs
> -----
>
> src/resource_provider/manager.hpp bc017fa3998b780cec82718cabe2a8b470a66db2
> src/resource_provider/manager.cpp 68e1866986bfb91bf8355099ee1f0a2a86aba39a
> src/resource_provider/registrar.cpp 92ef9aecb1e93d10f46e53984391558f9901a60b
> src/slave/slave.cpp 2b8c6e0771a58efacf26628abae6cbe46380a369
> src/tests/resource_provider_manager_tests.cpp c52541bf130ccf4795b989b53331176a64a097ea
>
>
> Diff: https://reviews.apache.org/r/66310/diff/7/
>
>
> Testing
> -------
>
> `make check`
>
>
> Thanks,
>
> Benjamin Bannier
>
>
Re: Review Request 66310: Passed on registrar when constructing
resource provider manager.
Posted by Chun-Hung Hsiao <ch...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/66310/#review202029
-----------------------------------------------------------
Ship it!
Ship It!
- Chun-Hung Hsiao
On April 26, 2018, 12:39 p.m., Benjamin Bannier wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/66310/
> -----------------------------------------------------------
>
> (Updated April 26, 2018, 12:39 p.m.)
>
>
> Review request for mesos, Chun-Hung Hsiao, Jie Yu, and Jan Schlicht.
>
>
> Bugs: MESOS-8735
> https://issues.apache.org/jira/browse/MESOS-8735
>
>
> Repository: mesos
>
>
> Description
> -------
>
> In order to support recovering resource provider manager information
> in the future, we need to adjust the construction of the manager to be
> able to consume a registrar.
>
> This patch lays the groundwork by adjusting interfaces and their
> usage; we will make use of the passed on information in a following
> patch.
>
>
> Diffs
> -----
>
> src/resource_provider/manager.hpp bc017fa3998b780cec82718cabe2a8b470a66db2
> src/resource_provider/manager.cpp 68e1866986bfb91bf8355099ee1f0a2a86aba39a
> src/resource_provider/registrar.cpp 92ef9aecb1e93d10f46e53984391558f9901a60b
> src/slave/slave.cpp d0ff5f872433b3891b32e35aea6212bf45693ae5
> src/tests/resource_provider_manager_tests.cpp c52541bf130ccf4795b989b53331176a64a097ea
>
>
> Diff: https://reviews.apache.org/r/66310/diff/10/
>
>
> Testing
> -------
>
> `make check`
>
>
> Thanks,
>
> Benjamin Bannier
>
>
Re: Review Request 66310: Passed on registrar when constructing
resource provider manager.
Posted by Benjamin Bannier <be...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/66310/
-----------------------------------------------------------
(Updated April 26, 2018, 2:39 p.m.)
Review request for mesos, Chun-Hung Hsiao, Jie Yu, and Jan Schlicht.
Changes
-------
Explicitly tore down resource provider manager with the agent.
This prevents multiple resource provider managers from being active when
an agent is terminated (without being destructed) and reassigned from a
newly constructed one.
Bugs: MESOS-8735
https://issues.apache.org/jira/browse/MESOS-8735
Repository: mesos
Description
-------
In order to support recovering resource provider manager information
in the future, we need to adjust the construction of the manager to be
able to consume a registrar.
This patch lays the groundwork by adjusting interfaces and their
usage; we will make use of the passed on information in a following
patch.
Diffs (updated)
-----
src/resource_provider/manager.hpp bc017fa3998b780cec82718cabe2a8b470a66db2
src/resource_provider/manager.cpp 68e1866986bfb91bf8355099ee1f0a2a86aba39a
src/resource_provider/registrar.cpp 92ef9aecb1e93d10f46e53984391558f9901a60b
src/slave/slave.cpp d0ff5f872433b3891b32e35aea6212bf45693ae5
src/tests/resource_provider_manager_tests.cpp c52541bf130ccf4795b989b53331176a64a097ea
Diff: https://reviews.apache.org/r/66310/diff/10/
Changes: https://reviews.apache.org/r/66310/diff/9-10/
Testing
-------
`make check`
Thanks,
Benjamin Bannier
Re: Review Request 66310: Passed on registrar when constructing
resource provider manager.
Posted by Benjamin Bannier <be...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/66310/
-----------------------------------------------------------
(Updated April 23, 2018, 1:18 p.m.)
Review request for mesos, Chun-Hung Hsiao, Jie Yu, and Jan Schlicht.
Changes
-------
Addressed comment from Chun.
Bugs: MESOS-8735
https://issues.apache.org/jira/browse/MESOS-8735
Repository: mesos
Description
-------
In order to support recovering resource provider manager information
in the future, we need to adjust the construction of the manager to be
able to consume a registrar.
This patch lays the groundwork by adjusting interfaces and their
usage; we will make use of the passed on information in a following
patch.
Diffs (updated)
-----
src/resource_provider/manager.hpp bc017fa3998b780cec82718cabe2a8b470a66db2
src/resource_provider/manager.cpp 68e1866986bfb91bf8355099ee1f0a2a86aba39a
src/resource_provider/registrar.cpp 92ef9aecb1e93d10f46e53984391558f9901a60b
src/slave/slave.cpp 2b8c6e0771a58efacf26628abae6cbe46380a369
src/tests/resource_provider_manager_tests.cpp c52541bf130ccf4795b989b53331176a64a097ea
Diff: https://reviews.apache.org/r/66310/diff/7/
Changes: https://reviews.apache.org/r/66310/diff/6-7/
Testing
-------
`make check`
Thanks,
Benjamin Bannier
Re: Review Request 66310: Passed on registrar when constructing
resource provider manager.
Posted by Benjamin Bannier <be...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/66310/
-----------------------------------------------------------
(Updated April 19, 2018, 2:28 p.m.)
Review request for mesos, Chun-Hung Hsiao, Jie Yu, and Jan Schlicht.
Changes
-------
Addressed issue raised by Chun.
Bugs: MESOS-8735
https://issues.apache.org/jira/browse/MESOS-8735
Repository: mesos
Description
-------
In order to support recovering resource provider manager information
in the future, we need to adjust the construction of the manager to be
able to consume a registrar.
This patch lays the groundwork by adjusting interfaces and their
usage; we will make use of the passed on information in a following
patch.
Diffs (updated)
-----
src/resource_provider/manager.hpp bc017fa3998b780cec82718cabe2a8b470a66db2
src/resource_provider/manager.cpp 68e1866986bfb91bf8355099ee1f0a2a86aba39a
src/slave/slave.cpp e5d6c3fac5054a6b0a0b5b77abd850a35be6ccc5
src/tests/resource_provider_manager_tests.cpp c52541bf130ccf4795b989b53331176a64a097ea
Diff: https://reviews.apache.org/r/66310/diff/5/
Changes: https://reviews.apache.org/r/66310/diff/4-5/
Testing
-------
`make check`
Thanks,
Benjamin Bannier