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/03/27 15:34:14 UTC
Review Request 66309: Externalize creation of resource provider
manager backing storage.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/66309/
-----------------------------------------------------------
Review request for mesos, Jie Yu and Jan Schlicht.
Bugs: MESOS-8735
https://issues.apache.org/jira/browse/MESOS-8735
Repository: mesos
Description
-------
This patch changes the way the storage backing an agent's resource
provider registrar is created: while before we created it implicitly
when constructing the registrar, we now consume storage passed on
construction.
Being able to explicitly inject the used storage simplifies testing.
Diffs
-----
src/resource_provider/registrar.hpp 39f45b0a2a7c35bfe72a9305f5248409e4a3ef45
src/resource_provider/registrar.cpp 9eb49f1327a0b598c5464a29a09ee286d7018f09
src/tests/resource_provider_manager_tests.cpp d947bd037190e6b7ea7b2277b5fbe47816878de4
Diff: https://reviews.apache.org/r/66309/diff/1/
Testing
-------
`make check`
Thanks,
Benjamin Bannier
Re: Review Request 66309: Externalized creation of resource provider
manager backing storage.
Posted by Chun-Hung Hsiao <ch...@apache.org>.
> On April 18, 2018, 9:31 p.m., Chun-Hung Hsiao wrote:
> > src/resource_provider/registrar.hpp
> > Lines 71-73 (original), 73-75 (patched)
> > <https://reviews.apache.org/r/66309/diff/3/?file=1995440#file1995440line73>
> >
> > I was wondering that, since we have a generic storage-backed registrar, could we also use this for the master's RP registrar and remove `MasterRegistrar`?
>
> Benjamin Bannier wrote:
> The master registrar is different in that it is already recovered and we need to use it differently when retrieving its recovered state via above interface, see https://reviews.apache.org/r/66311//
I get that we need this interface to retrieve data from the master's registrar. I was wondering if it is necessary or has a lot of advantages to store the RP manager registry in the master's registrar.
- Chun-Hung
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/66309/#review201449
-----------------------------------------------------------
On April 19, 2018, 9:19 a.m., Benjamin Bannier wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/66309/
> -----------------------------------------------------------
>
> (Updated April 19, 2018, 9:19 a.m.)
>
>
> Review request for mesos, Jie Yu and Jan Schlicht.
>
>
> Bugs: MESOS-8735
> https://issues.apache.org/jira/browse/MESOS-8735
>
>
> Repository: mesos
>
>
> Description
> -------
>
> This patch changes the way the storage backing an agent's resource
> provider registrar is created: while before we created it implicitly
> when constructing the registrar, we now consume storage passed on
> construction.
>
> Being able to explicitly inject the used storage simplifies testing.
>
>
> Diffs
> -----
>
> src/resource_provider/registrar.hpp 39f45b0a2a7c35bfe72a9305f5248409e4a3ef45
> src/resource_provider/registrar.cpp 92ef9aecb1e93d10f46e53984391558f9901a60b
> src/tests/resource_provider_manager_tests.cpp c52541bf130ccf4795b989b53331176a64a097ea
>
>
> Diff: https://reviews.apache.org/r/66309/diff/4/
>
>
> Testing
> -------
>
> `make check`
>
>
> Thanks,
>
> Benjamin Bannier
>
>
Re: Review Request 66309: Externalized creation of resource provider
manager backing storage.
Posted by Benjamin Bannier <be...@mesosphere.io>.
> On April 18, 2018, 11:31 p.m., Chun-Hung Hsiao wrote:
> > src/resource_provider/registrar.hpp
> > Line 69 (original), 71 (patched)
> > <https://reviews.apache.org/r/66309/diff/3/?file=1995440#file1995440line71>
> >
> > Does it make sense to use `process::Owned<state::Storage>&& storage`?
I don't think so. Taking a value already implies that we take ownership of the passed `storage`, and to call this function any user would need to provide us with that exclusive ownership (e.g., by casting some local with `std::move`). All that restricting ourself to rvalues here would add is that it would enforce correct usage even for broken types which are only semantically non-copyable, but still define a copy constructor. Unfortunately `Owned` falls into that category (see MESOS-5122), but I prefer to explicitly move args when passing instead of polluting interfaces because of tech debt we will fix eventually.
> On April 18, 2018, 11:31 p.m., Chun-Hung Hsiao wrote:
> > src/resource_provider/registrar.hpp
> > Lines 71-73 (original), 73-75 (patched)
> > <https://reviews.apache.org/r/66309/diff/3/?file=1995440#file1995440line73>
> >
> > I was wondering that, since we have a generic storage-backed registrar, could we also use this for the master's RP registrar and remove `MasterRegistrar`?
The master registrar is different in that it is already recovered and we need to use it differently when retrieving its recovered state via above interface, see https://reviews.apache.org/r/66311//
- Benjamin
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/66309/#review201449
-----------------------------------------------------------
On April 19, 2018, 11:19 a.m., Benjamin Bannier wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/66309/
> -----------------------------------------------------------
>
> (Updated April 19, 2018, 11:19 a.m.)
>
>
> Review request for mesos, Jie Yu and Jan Schlicht.
>
>
> Bugs: MESOS-8735
> https://issues.apache.org/jira/browse/MESOS-8735
>
>
> Repository: mesos
>
>
> Description
> -------
>
> This patch changes the way the storage backing an agent's resource
> provider registrar is created: while before we created it implicitly
> when constructing the registrar, we now consume storage passed on
> construction.
>
> Being able to explicitly inject the used storage simplifies testing.
>
>
> Diffs
> -----
>
> src/resource_provider/registrar.hpp 39f45b0a2a7c35bfe72a9305f5248409e4a3ef45
> src/resource_provider/registrar.cpp 92ef9aecb1e93d10f46e53984391558f9901a60b
> src/tests/resource_provider_manager_tests.cpp c52541bf130ccf4795b989b53331176a64a097ea
>
>
> Diff: https://reviews.apache.org/r/66309/diff/4/
>
>
> Testing
> -------
>
> `make check`
>
>
> Thanks,
>
> Benjamin Bannier
>
>
Re: Review Request 66309: Externalized creation of resource provider
manager backing storage.
Posted by Chun-Hung Hsiao <ch...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/66309/#review201449
-----------------------------------------------------------
src/resource_provider/registrar.hpp
Line 67 (original), 69 (patched)
<https://reviews.apache.org/r/66309/#comment282690>
This comment seems not correct.
src/resource_provider/registrar.hpp
Line 69 (original), 71 (patched)
<https://reviews.apache.org/r/66309/#comment282702>
Does it make sense to use `process::Owned<state::Storage>&& storage`?
src/resource_provider/registrar.hpp
Lines 71-73 (original), 73-75 (patched)
<https://reviews.apache.org/r/66309/#comment282701>
I was wondering that, since we have a generic storage-backed registrar, could we also use this for the master's RP registrar and remove `MasterRegistrar`?
- 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/66309/
> -----------------------------------------------------------
>
> (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
> -------
>
> This patch changes the way the storage backing an agent's resource
> provider registrar is created: while before we created it implicitly
> when constructing the registrar, we now consume storage passed on
> construction.
>
> Being able to explicitly inject the used storage simplifies testing.
>
>
> Diffs
> -----
>
> src/resource_provider/registrar.hpp 39f45b0a2a7c35bfe72a9305f5248409e4a3ef45
> src/resource_provider/registrar.cpp 92ef9aecb1e93d10f46e53984391558f9901a60b
> src/tests/resource_provider_manager_tests.cpp c52541bf130ccf4795b989b53331176a64a097ea
>
>
> Diff: https://reviews.apache.org/r/66309/diff/3/
>
>
> Testing
> -------
>
> `make check`
>
>
> Thanks,
>
> Benjamin Bannier
>
>
Re: Review Request 66309: Externalized creation of resource provider
manager backing storage.
Posted by Chun-Hung Hsiao <ch...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/66309/#review201874
-----------------------------------------------------------
src/tests/resource_provider_manager_tests.cpp
Line 823 (original), 823 (patched)
<https://reviews.apache.org/r/66309/#comment283477>
Please do s/`AgentRegistrar`/`GenericRegistrar`/ and fix the comment above accordingly in the next patch.
- Chun-Hung Hsiao
On April 19, 2018, 9:19 a.m., Benjamin Bannier wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/66309/
> -----------------------------------------------------------
>
> (Updated April 19, 2018, 9:19 a.m.)
>
>
> Review request for mesos, Jie Yu and Jan Schlicht.
>
>
> Bugs: MESOS-8735
> https://issues.apache.org/jira/browse/MESOS-8735
>
>
> Repository: mesos
>
>
> Description
> -------
>
> This patch changes the way the storage backing an agent's resource
> provider registrar is created: while before we created it implicitly
> when constructing the registrar, we now consume storage passed on
> construction.
>
> Being able to explicitly inject the used storage simplifies testing.
>
>
> Diffs
> -----
>
> src/resource_provider/registrar.hpp 39f45b0a2a7c35bfe72a9305f5248409e4a3ef45
> src/resource_provider/registrar.cpp 92ef9aecb1e93d10f46e53984391558f9901a60b
> src/tests/resource_provider_manager_tests.cpp c52541bf130ccf4795b989b53331176a64a097ea
>
>
> Diff: https://reviews.apache.org/r/66309/diff/4/
>
>
> Testing
> -------
>
> `make check`
>
>
> Thanks,
>
> Benjamin Bannier
>
>
Re: Review Request 66309: Externalized creation of resource provider
manager backing storage.
Posted by Chun-Hung Hsiao <ch...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/66309/#review201603
-----------------------------------------------------------
Ship it!
Ship It!
- Chun-Hung Hsiao
On April 19, 2018, 9:19 a.m., Benjamin Bannier wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/66309/
> -----------------------------------------------------------
>
> (Updated April 19, 2018, 9:19 a.m.)
>
>
> Review request for mesos, Jie Yu and Jan Schlicht.
>
>
> Bugs: MESOS-8735
> https://issues.apache.org/jira/browse/MESOS-8735
>
>
> Repository: mesos
>
>
> Description
> -------
>
> This patch changes the way the storage backing an agent's resource
> provider registrar is created: while before we created it implicitly
> when constructing the registrar, we now consume storage passed on
> construction.
>
> Being able to explicitly inject the used storage simplifies testing.
>
>
> Diffs
> -----
>
> src/resource_provider/registrar.hpp 39f45b0a2a7c35bfe72a9305f5248409e4a3ef45
> src/resource_provider/registrar.cpp 92ef9aecb1e93d10f46e53984391558f9901a60b
> src/tests/resource_provider_manager_tests.cpp c52541bf130ccf4795b989b53331176a64a097ea
>
>
> Diff: https://reviews.apache.org/r/66309/diff/4/
>
>
> Testing
> -------
>
> `make check`
>
>
> Thanks,
>
> Benjamin Bannier
>
>
Re: Review Request 66309: Externalized creation of resource provider
manager backing storage.
Posted by Benjamin Bannier <be...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/66309/
-----------------------------------------------------------
(Updated April 19, 2018, 11:19 a.m.)
Review request for mesos, 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
-------
This patch changes the way the storage backing an agent's resource
provider registrar is created: while before we created it implicitly
when constructing the registrar, we now consume storage passed on
construction.
Being able to explicitly inject the used storage simplifies testing.
Diffs (updated)
-----
src/resource_provider/registrar.hpp 39f45b0a2a7c35bfe72a9305f5248409e4a3ef45
src/resource_provider/registrar.cpp 92ef9aecb1e93d10f46e53984391558f9901a60b
src/tests/resource_provider_manager_tests.cpp c52541bf130ccf4795b989b53331176a64a097ea
Diff: https://reviews.apache.org/r/66309/diff/4/
Changes: https://reviews.apache.org/r/66309/diff/3-4/
Testing
-------
`make check`
Thanks,
Benjamin Bannier
Re: Review Request 66309: Externalized creation of resource provider
manager backing storage.
Posted by Benjamin Bannier <be...@mesosphere.io>.
> On April 18, 2018, 11:51 p.m., Chun-Hung Hsiao wrote:
> > src/resource_provider/registrar.hpp
> > Lines 67-73 (original), 69-75 (patched)
> > <https://reviews.apache.org/r/66309/diff/3/?file=1995440#file1995440line69>
> >
> > Not yours, but if each `create()` corresponds to a different type of registrar, how about moving these functions into their corresponding `MasterRegistrar` and `AgentRegistrar`?
> >
> > I guess it depends on how meanful it is to encapsulate the actual type of registrar from the callers.
>
> Chun-Hung Hsiao wrote:
> Also, if there is no need to encapsulate the actual type of registrar, then it is meanleass to have these `create()` functions, since we don't do extra checks and return errors.
I think having these factory functions still adds value as they hide away non-trivial details. The arguments pretty clearly communicate how they can be used, e.g., in the master we would not be able to transfer ownership of the underlying storage leaving only one possible factory to use.
Let's keep this as is, dropping.
- Benjamin
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/66309/#review201469
-----------------------------------------------------------
On April 19, 2018, 11:19 a.m., Benjamin Bannier wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/66309/
> -----------------------------------------------------------
>
> (Updated April 19, 2018, 11:19 a.m.)
>
>
> Review request for mesos, Jie Yu and Jan Schlicht.
>
>
> Bugs: MESOS-8735
> https://issues.apache.org/jira/browse/MESOS-8735
>
>
> Repository: mesos
>
>
> Description
> -------
>
> This patch changes the way the storage backing an agent's resource
> provider registrar is created: while before we created it implicitly
> when constructing the registrar, we now consume storage passed on
> construction.
>
> Being able to explicitly inject the used storage simplifies testing.
>
>
> Diffs
> -----
>
> src/resource_provider/registrar.hpp 39f45b0a2a7c35bfe72a9305f5248409e4a3ef45
> src/resource_provider/registrar.cpp 92ef9aecb1e93d10f46e53984391558f9901a60b
> src/tests/resource_provider_manager_tests.cpp c52541bf130ccf4795b989b53331176a64a097ea
>
>
> Diff: https://reviews.apache.org/r/66309/diff/4/
>
>
> Testing
> -------
>
> `make check`
>
>
> Thanks,
>
> Benjamin Bannier
>
>
Re: Review Request 66309: Externalized creation of resource provider
manager backing storage.
Posted by Chun-Hung Hsiao <ch...@apache.org>.
> On April 18, 2018, 9:51 p.m., Chun-Hung Hsiao wrote:
> > src/resource_provider/registrar.hpp
> > Lines 67-73 (original), 69-75 (patched)
> > <https://reviews.apache.org/r/66309/diff/3/?file=1995440#file1995440line69>
> >
> > Not yours, but if each `create()` corresponds to a different type of registrar, how about moving these functions into their corresponding `MasterRegistrar` and `AgentRegistrar`?
> >
> > I guess it depends on how meanful it is to encapsulate the actual type of registrar from the callers.
Also, if there is no need to encapsulate the actual type of registrar, then it is meanleass to have these `create()` functions, since we don't do extra checks and return errors.
- Chun-Hung
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/66309/#review201469
-----------------------------------------------------------
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/66309/
> -----------------------------------------------------------
>
> (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
> -------
>
> This patch changes the way the storage backing an agent's resource
> provider registrar is created: while before we created it implicitly
> when constructing the registrar, we now consume storage passed on
> construction.
>
> Being able to explicitly inject the used storage simplifies testing.
>
>
> Diffs
> -----
>
> src/resource_provider/registrar.hpp 39f45b0a2a7c35bfe72a9305f5248409e4a3ef45
> src/resource_provider/registrar.cpp 92ef9aecb1e93d10f46e53984391558f9901a60b
> src/tests/resource_provider_manager_tests.cpp c52541bf130ccf4795b989b53331176a64a097ea
>
>
> Diff: https://reviews.apache.org/r/66309/diff/3/
>
>
> Testing
> -------
>
> `make check`
>
>
> Thanks,
>
> Benjamin Bannier
>
>
Re: Review Request 66309: Externalized creation of resource provider
manager backing storage.
Posted by Chun-Hung Hsiao <ch...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/66309/#review201469
-----------------------------------------------------------
src/resource_provider/registrar.hpp
Lines 67-73 (original), 69-75 (patched)
<https://reviews.apache.org/r/66309/#comment282717>
Not yours, but if each `create()` corresponds to a different type of registrar, how about moving these functions into their corresponding `MasterRegistrar` and `AgentRegistrar`?
I guess it depends on how meanful it is to encapsulate the actual type of registrar from the callers.
- 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/66309/
> -----------------------------------------------------------
>
> (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
> -------
>
> This patch changes the way the storage backing an agent's resource
> provider registrar is created: while before we created it implicitly
> when constructing the registrar, we now consume storage passed on
> construction.
>
> Being able to explicitly inject the used storage simplifies testing.
>
>
> Diffs
> -----
>
> src/resource_provider/registrar.hpp 39f45b0a2a7c35bfe72a9305f5248409e4a3ef45
> src/resource_provider/registrar.cpp 92ef9aecb1e93d10f46e53984391558f9901a60b
> src/tests/resource_provider_manager_tests.cpp c52541bf130ccf4795b989b53331176a64a097ea
>
>
> Diff: https://reviews.apache.org/r/66309/diff/3/
>
>
> Testing
> -------
>
> `make check`
>
>
> Thanks,
>
> Benjamin Bannier
>
>
Re: Review Request 66309: Externalized creation of resource provider
manager backing storage.
Posted by Benjamin Bannier <be...@mesosphere.io>.
> On April 19, 2018, 12:19 a.m., Chun-Hung Hsiao wrote:
> > src/resource_provider/registrar.hpp
> > Line 113 (original), 114 (patched)
> > <https://reviews.apache.org/r/66309/diff/3/?file=1995440#file1995440line115>
> >
> > `process::Owned<state::Storage>&& storage`?
See above.
> On April 19, 2018, 12:19 a.m., Chun-Hung Hsiao wrote:
> > src/resource_provider/registrar.cpp
> > Line 163 (original), 151 (patched)
> > <https://reviews.apache.org/r/66309/diff/3/?file=1995441#file1995441line163>
> >
> > `Owned<Storage>&& _storage`? The parameter has an underscore below.
We use the underscore to disambiguate the different names to readers. It is required below, but not here.
- Benjamin
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/66309/#review201476
-----------------------------------------------------------
On April 19, 2018, 11:19 a.m., Benjamin Bannier wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/66309/
> -----------------------------------------------------------
>
> (Updated April 19, 2018, 11:19 a.m.)
>
>
> Review request for mesos, Jie Yu and Jan Schlicht.
>
>
> Bugs: MESOS-8735
> https://issues.apache.org/jira/browse/MESOS-8735
>
>
> Repository: mesos
>
>
> Description
> -------
>
> This patch changes the way the storage backing an agent's resource
> provider registrar is created: while before we created it implicitly
> when constructing the registrar, we now consume storage passed on
> construction.
>
> Being able to explicitly inject the used storage simplifies testing.
>
>
> Diffs
> -----
>
> src/resource_provider/registrar.hpp 39f45b0a2a7c35bfe72a9305f5248409e4a3ef45
> src/resource_provider/registrar.cpp 92ef9aecb1e93d10f46e53984391558f9901a60b
> src/tests/resource_provider_manager_tests.cpp c52541bf130ccf4795b989b53331176a64a097ea
>
>
> Diff: https://reviews.apache.org/r/66309/diff/4/
>
>
> Testing
> -------
>
> `make check`
>
>
> Thanks,
>
> Benjamin Bannier
>
>
Re: Review Request 66309: Externalized creation of resource provider
manager backing storage.
Posted by Chun-Hung Hsiao <ch...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/66309/#review201476
-----------------------------------------------------------
src/resource_provider/registrar.hpp
Line 113 (original), 114 (patched)
<https://reviews.apache.org/r/66309/#comment282728>
`process::Owned<state::Storage>&& storage`?
src/resource_provider/registrar.cpp
Line 163 (original), 151 (patched)
<https://reviews.apache.org/r/66309/#comment282726>
`Owned<Storage>&& _storage`? The parameter has an underscore below.
- 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/66309/
> -----------------------------------------------------------
>
> (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
> -------
>
> This patch changes the way the storage backing an agent's resource
> provider registrar is created: while before we created it implicitly
> when constructing the registrar, we now consume storage passed on
> construction.
>
> Being able to explicitly inject the used storage simplifies testing.
>
>
> Diffs
> -----
>
> src/resource_provider/registrar.hpp 39f45b0a2a7c35bfe72a9305f5248409e4a3ef45
> src/resource_provider/registrar.cpp 92ef9aecb1e93d10f46e53984391558f9901a60b
> src/tests/resource_provider_manager_tests.cpp c52541bf130ccf4795b989b53331176a64a097ea
>
>
> Diff: https://reviews.apache.org/r/66309/diff/3/
>
>
> Testing
> -------
>
> `make check`
>
>
> Thanks,
>
> Benjamin Bannier
>
>
Re: Review Request 66309: Externalize creation of resource provider
manager backing storage.
Posted by Benjamin Bannier <be...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/66309/
-----------------------------------------------------------
(Updated April 10, 2018, 2:07 p.m.)
Review request for mesos, Jie Yu and Jan Schlicht.
Changes
-------
Addressed issues raised by Jan.
Bugs: MESOS-8735
https://issues.apache.org/jira/browse/MESOS-8735
Repository: mesos
Description
-------
This patch changes the way the storage backing an agent's resource
provider registrar is created: while before we created it implicitly
when constructing the registrar, we now consume storage passed on
construction.
Being able to explicitly inject the used storage simplifies testing.
Diffs (updated)
-----
src/resource_provider/registrar.hpp 39f45b0a2a7c35bfe72a9305f5248409e4a3ef45
src/resource_provider/registrar.cpp 9eb49f1327a0b598c5464a29a09ee286d7018f09
src/tests/resource_provider_manager_tests.cpp c52541bf130ccf4795b989b53331176a64a097ea
Diff: https://reviews.apache.org/r/66309/diff/2/
Changes: https://reviews.apache.org/r/66309/diff/1-2/
Testing
-------
`make check`
Thanks,
Benjamin Bannier
Re: Review Request 66309: Externalize creation of resource provider
manager backing storage.
Posted by Benjamin Bannier <be...@mesosphere.io>.
> On March 28, 2018, 3:30 p.m., Jan Schlicht wrote:
> > src/resource_provider/registrar.hpp
> > Lines 68-69 (original), 70-71 (patched)
> > <https://reviews.apache.org/r/66309/diff/1/?file=1989033#file1989033line70>
> >
> > I don't like the semantics of this:
> > A `state::Storage` doesn't provide any information if it belongs to master or agent but we instantiate an `AgentRegistrar` anyways.
> >
> > Hence, please rename `AgentRegistrar` to something more general that indicates that this isn't tied to an agent state.
That makes much sense. I have added a follow up patch to address this to not put too many changes into this one, https://reviews.apache.org/r/66526/.
- Benjamin
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/66309/#review200105
-----------------------------------------------------------
On April 10, 2018, 2:07 p.m., Benjamin Bannier wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/66309/
> -----------------------------------------------------------
>
> (Updated April 10, 2018, 2: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
> -------
>
> This patch changes the way the storage backing an agent's resource
> provider registrar is created: while before we created it implicitly
> when constructing the registrar, we now consume storage passed on
> construction.
>
> Being able to explicitly inject the used storage simplifies testing.
>
>
> Diffs
> -----
>
> src/resource_provider/registrar.hpp 39f45b0a2a7c35bfe72a9305f5248409e4a3ef45
> src/resource_provider/registrar.cpp 9eb49f1327a0b598c5464a29a09ee286d7018f09
> src/tests/resource_provider_manager_tests.cpp c52541bf130ccf4795b989b53331176a64a097ea
>
>
> Diff: https://reviews.apache.org/r/66309/diff/2/
>
>
> Testing
> -------
>
> `make check`
>
>
> Thanks,
>
> Benjamin Bannier
>
>
Re: Review Request 66309: Externalize creation of resource provider
manager backing storage.
Posted by Jan Schlicht <ja...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/66309/#review200105
-----------------------------------------------------------
src/resource_provider/registrar.hpp
Lines 28 (patched)
<https://reviews.apache.org/r/66309/#comment280752>
Include this below `<memory>` and above `<process/future.hpp`.
src/resource_provider/registrar.hpp
Line 67 (original), 69 (patched)
<https://reviews.apache.org/r/66309/#comment280753>
Delete `master's`.
src/resource_provider/registrar.hpp
Lines 68-69 (original), 70-71 (patched)
<https://reviews.apache.org/r/66309/#comment280754>
I don't like the semantics of this:
A `state::Storage` doesn't provide any information if it belongs to master or agent but we instantiate an `AgentRegistrar` anyways.
Hence, please rename `AgentRegistrar` to something more general that indicates that this isn't tied to an agent state.
src/resource_provider/registrar.hpp
Line 110 (original), 111 (patched)
<https://reviews.apache.org/r/66309/#comment280755>
Please rename, see above.
- Jan Schlicht
On March 27, 2018, 5:34 p.m., Benjamin Bannier wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/66309/
> -----------------------------------------------------------
>
> (Updated March 27, 2018, 5:34 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
> -------
>
> This patch changes the way the storage backing an agent's resource
> provider registrar is created: while before we created it implicitly
> when constructing the registrar, we now consume storage passed on
> construction.
>
> Being able to explicitly inject the used storage simplifies testing.
>
>
> Diffs
> -----
>
> src/resource_provider/registrar.hpp 39f45b0a2a7c35bfe72a9305f5248409e4a3ef45
> src/resource_provider/registrar.cpp 9eb49f1327a0b598c5464a29a09ee286d7018f09
> src/tests/resource_provider_manager_tests.cpp d947bd037190e6b7ea7b2277b5fbe47816878de4
>
>
> Diff: https://reviews.apache.org/r/66309/diff/1/
>
>
> Testing
> -------
>
> `make check`
>
>
> Thanks,
>
> Benjamin Bannier
>
>