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:13 UTC

Review Request 66308: Delayed construction of the agent's resource provider manager.

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/66308/
-----------------------------------------------------------

Review request for mesos, Jie Yu and Jan Schlicht.


Bugs: MESOS-8735
    https://issues.apache.org/jira/browse/MESOS-8735


Repository: mesos


Description
-------

By delaying the construction of the agent's resource provider manager
we prepare for a following patch introducing a dependency on the
resource provider manager on the agent's ID.

Depending on whether the agent was able to recover an agent ID from
its log or still needs to obtain on in a first registration with the
master, we can only construct the resource provider manager after
different points in the initialization of the agent. To capture the
common code we introduce a helper function performing the necessary
steps.


Diffs
-----

  src/slave/slave.hpp 37f0361251524e63d02d251e8a03901812b8affb 
  src/slave/slave.cpp e76daabad0d2d68aa42d1da809d4a23459eaaacb 
  src/tests/resource_provider_manager_tests.cpp d947bd037190e6b7ea7b2277b5fbe47816878de4 


Diff: https://reviews.apache.org/r/66308/diff/1/


Testing
-------

`make check`


Thanks,

Benjamin Bannier


Re: Review Request 66308: Delayed construction of the agent's resource provider manager.

Posted by Chun-Hung Hsiao <ch...@apache.org>.

> On April 18, 2018, 3:03 a.m., Chun-Hung Hsiao wrote:
> > src/slave/slave.cpp
> > Lines 8833-8840 (patched)
> > <https://reviews.apache.org/r/66308/diff/3/?file=1994904#file1994904line8844>
> >
> >     There was a recent discussion in the API WG about adding routes dynamically (after initialization). The discussion started with this ticket: https://issues.apache.org/jira/browse/MESOS-7697
> >     
> >     In summary, libprocess would return a 404 if a route has not been added yet, but for certain endpoints (e.g., the `/api/v1/scheduler`, or the agent resource provider config API calls), 404 might also be used by the API handler to represent missing resources. A client would have no way to distinguish what's the meaning of a 404, and if it should retry.
> >     
> >     Several ideas had been proposed, sech as:
> >     (1) Establishing a convention that a Mesos API handler never uses 404, but use 410 instead. But the semantics of "GONE" dose not quite match "NOT FOUND".
> >     
> >     (2) Make libprocess returns 503 in the beginning, and then at certain point of time, mark libprocess as "initialized", meaning that no more routes will be added, and after that libprocess could return 404 if a route is added.
> >     
> >     Along with the discussion of (2), people seems to agree that in most cases dynamic addition of routes is not very useful and (2) seems a viable solution. There's no conclusion yet, but if we're going to follow (2), I'd avoid adding `/api/v1/resource_provider` here, but just registering this in `Slave::initialize()`, and let the resource provider manager rejects requests until it obtains the slave ID. This is also what I did for `LocalResourceProviderDaemon`: https://github.com/apache/mesos/blob/master/src/resource_provider/daemon.cpp#L147.
> >     
> >     Before we establish the convention, I'd suggest that we avoid adding routes after `Slave::initialize()`. Thoughts?
> 
> Chun-Hung Hsiao wrote:
>     Some typo:
>     "sech as" -> "such as"
>     "return 404 if a route is added" -> "return 404 if a route is *not* there"
> 
> Benjamin Bannier wrote:
>     I changed the code to add the route statically on agent initialization, but not servicing it until a resource provider manager has be created.
>     
>     This seems to make sense as while the route is owned by the agent (and should have a corresponding lifetime), the agent's ability to serve it depends on whether the rp manager is available. Does that capture what you had in mind?

Yeah this change looks good to me. In the case of `LocalResourceProviderDaemon`, it still serves requests (modifying configs) even if the agent is not resistered yet. In the case of `ResourceProviderManager` it cannot serve anything.


- Chun-Hung


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/66308/#review201377
-----------------------------------------------------------


On April 18, 2018, 2:28 p.m., Benjamin Bannier wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/66308/
> -----------------------------------------------------------
> 
> (Updated April 18, 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
> -------
> 
> By delaying the construction of the agent's resource provider manager
> we prepare for a following patch introducing a dependency on the
> resource provider manager on the agent's ID.
> 
> Depending on whether the agent was able to recover an agent ID from
> its log or still needs to obtain on in a first registration with the
> master, we can only construct the resource provider manager after
> different points in the initialization of the agent. To capture the
> common code we introduce a helper function performing the necessary
> steps.
> 
> 
> Diffs
> -----
> 
>   src/slave/slave.hpp d00c7b2e2466fd1de71e9e55343a27d3808bde3e 
>   src/slave/slave.cpp e5d6c3fac5054a6b0a0b5b77abd850a35be6ccc5 
>   src/tests/resource_provider_manager_tests.cpp c52541bf130ccf4795b989b53331176a64a097ea 
> 
> 
> Diff: https://reviews.apache.org/r/66308/diff/4/
> 
> 
> Testing
> -------
> 
> `make check`
> 
> 
> Thanks,
> 
> Benjamin Bannier
> 
>


Re: Review Request 66308: Delayed construction of the agent's resource provider manager.

Posted by Benjamin Bannier <be...@mesosphere.io>.

> On April 18, 2018, 5:03 a.m., Chun-Hung Hsiao wrote:
> > src/slave/slave.hpp
> > Line 815 (original), 819 (patched)
> > <https://reviews.apache.org/r/66308/diff/3/?file=1994903#file1994903line819>
> >
> >     This is inconsistent with the existing codebase. Could you justify why favoring `unique_ptr` over `Owned`?

I went for a `unique_ptr` while developing because it is much easier to use it correctly (see e.g., MESOS-5122). I changed the code to used `Owned` now for consistency.


> On April 18, 2018, 5:03 a.m., Chun-Hung Hsiao wrote:
> > src/slave/slave.cpp
> > Line 4606 (original), 4601-4616 (patched)
> > <https://reviews.apache.org/r/66308/diff/3/?file=1994904#file1994904line4610>
> >
> >     How about the following?
> >     ```
> >     Result<ResourceProviderID> resourceProviderId =
> >       getResourceProviderId(operation->info());
> >     
> >     CHECK(!resourceProviderId.isError());
> >     
> >     if (CHECKresourceProviderId.isSome()) {
> >       CHECK_NOTNULL(resourceProviderManager.get())
> >         ->acknowedgeOperationStatus(acknowledgement);
> >     }
> >     ```
> >     I prefer this way to encapsulate the implementation details about how to get the resource provider ID from the consumed resources.

I like that suggestion, adopted.


> On April 18, 2018, 5:03 a.m., Chun-Hung Hsiao wrote:
> > src/slave/slave.cpp
> > Lines 8177-8179 (patched)
> > <https://reviews.apache.org/r/66308/diff/3/?file=1994904#file1994904line8188>
> >
> >     Let's check that all resources are agent default resources.

I have added an assertion to that effect here.


> On April 18, 2018, 5:03 a.m., Chun-Hung Hsiao wrote:
> > src/slave/slave.cpp
> > Lines 8822 (patched)
> > <https://reviews.apache.org/r/66308/diff/3/?file=1994904#file1994904line8833>
> >
> >     `flags` is not used in this function.

It will be used in https://reviews.apache.org/r/66310/ and I added it here already.


> On April 18, 2018, 5:03 a.m., Chun-Hung Hsiao wrote:
> > src/slave/slave.cpp
> > Lines 8829 (patched)
> > <https://reviews.apache.org/r/66308/diff/3/?file=1994904#file1994904line8840>
> >
> >     Could you elaborate on this? Or make it a TODO if the comment is too complicated to be added ;)

Ups, I did remove this in https://reviews.apache.org/r/66310/, and hadn't realized it was still _here_. Fixed the artifact here now.


> On April 18, 2018, 5:03 a.m., Chun-Hung Hsiao wrote:
> > src/slave/slave.cpp
> > Lines 8833-8840 (patched)
> > <https://reviews.apache.org/r/66308/diff/3/?file=1994904#file1994904line8844>
> >
> >     There was a recent discussion in the API WG about adding routes dynamically (after initialization). The discussion started with this ticket: https://issues.apache.org/jira/browse/MESOS-7697
> >     
> >     In summary, libprocess would return a 404 if a route has not been added yet, but for certain endpoints (e.g., the `/api/v1/scheduler`, or the agent resource provider config API calls), 404 might also be used by the API handler to represent missing resources. A client would have no way to distinguish what's the meaning of a 404, and if it should retry.
> >     
> >     Several ideas had been proposed, sech as:
> >     (1) Establishing a convention that a Mesos API handler never uses 404, but use 410 instead. But the semantics of "GONE" dose not quite match "NOT FOUND".
> >     
> >     (2) Make libprocess returns 503 in the beginning, and then at certain point of time, mark libprocess as "initialized", meaning that no more routes will be added, and after that libprocess could return 404 if a route is added.
> >     
> >     Along with the discussion of (2), people seems to agree that in most cases dynamic addition of routes is not very useful and (2) seems a viable solution. There's no conclusion yet, but if we're going to follow (2), I'd avoid adding `/api/v1/resource_provider` here, but just registering this in `Slave::initialize()`, and let the resource provider manager rejects requests until it obtains the slave ID. This is also what I did for `LocalResourceProviderDaemon`: https://github.com/apache/mesos/blob/master/src/resource_provider/daemon.cpp#L147.
> >     
> >     Before we establish the convention, I'd suggest that we avoid adding routes after `Slave::initialize()`. Thoughts?
> 
> Chun-Hung Hsiao wrote:
>     Some typo:
>     "sech as" -> "such as"
>     "return 404 if a route is added" -> "return 404 if a route is *not* there"

I changed the code to add the route statically on agent initialization, but not servicing it until a resource provider manager has be created.

This seems to make sense as while the route is owned by the agent (and should have a corresponding lifetime), the agent's ability to serve it depends on whether the rp manager is available. Does that capture what you had in mind?


- Benjamin


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/66308/#review201377
-----------------------------------------------------------


On April 18, 2018, 4:28 p.m., Benjamin Bannier wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/66308/
> -----------------------------------------------------------
> 
> (Updated April 18, 2018, 4: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
> -------
> 
> By delaying the construction of the agent's resource provider manager
> we prepare for a following patch introducing a dependency on the
> resource provider manager on the agent's ID.
> 
> Depending on whether the agent was able to recover an agent ID from
> its log or still needs to obtain on in a first registration with the
> master, we can only construct the resource provider manager after
> different points in the initialization of the agent. To capture the
> common code we introduce a helper function performing the necessary
> steps.
> 
> 
> Diffs
> -----
> 
>   src/slave/slave.hpp d00c7b2e2466fd1de71e9e55343a27d3808bde3e 
>   src/slave/slave.cpp e5d6c3fac5054a6b0a0b5b77abd850a35be6ccc5 
>   src/tests/resource_provider_manager_tests.cpp c52541bf130ccf4795b989b53331176a64a097ea 
> 
> 
> Diff: https://reviews.apache.org/r/66308/diff/4/
> 
> 
> Testing
> -------
> 
> `make check`
> 
> 
> Thanks,
> 
> Benjamin Bannier
> 
>


Re: Review Request 66308: Delayed construction of the agent's 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/66308/#review201377
-----------------------------------------------------------




src/slave/slave.hpp
Line 815 (original), 819 (patched)
<https://reviews.apache.org/r/66308/#comment282589>

    This is inconsistent with the existing codebase. Could you justify why favoring `unique_ptr` over `Owned`?



src/slave/slave.cpp
Line 4606 (original), 4601-4616 (patched)
<https://reviews.apache.org/r/66308/#comment282596>

    How about the following?
    ```
    Result<ResourceProviderID> resourceProviderId =
      getResourceProviderId(operation->info());
    
    CHECK(!resourceProviderId.isError());
    
    if (CHECKresourceProviderId.isSome()) {
      CHECK_NOTNULL(resourceProviderManager.get())
        ->acknowedgeOperationStatus(acknowledgement);
    }
    ```
    I prefer this way to encapsulate the implementation details about how to get the resource provider ID from the consumed resources.



src/slave/slave.cpp
Lines 8177-8179 (patched)
<https://reviews.apache.org/r/66308/#comment282599>

    Let's check that all resources are agent default resources.



src/slave/slave.cpp
Line 8187 (original), 8201 (patched)
<https://reviews.apache.org/r/66308/#comment282598>

    ```
    CHECK_NOTNULL(resourceProviderManager.get())
      ->publishResources(resources);
    ```



src/slave/slave.cpp
Lines 8822 (patched)
<https://reviews.apache.org/r/66308/#comment282593>

    `flags` is not used in this function.



src/slave/slave.cpp
Lines 8829 (patched)
<https://reviews.apache.org/r/66308/#comment282591>

    Could you elaborate on this? Or make it a TODO if the comment is too complicated to be added ;)



src/slave/slave.cpp
Lines 8833-8840 (patched)
<https://reviews.apache.org/r/66308/#comment282590>

    There was a recent discussion in the API WG about adding routes dynamically (after initialization). The discussion started with this ticket: https://issues.apache.org/jira/browse/MESOS-7697
    
    In summary, libprocess would return a 404 if a route has not been added yet, but for certain endpoints (e.g., the `/api/v1/scheduler`, or the agent resource provider config API calls), 404 might also be used by the API handler to represent missing resources. A client would have no way to distinguish what's the meaning of a 404, and if it should retry.
    
    Several ideas had been proposed, sech as:
    (1) Establishing a convention that a Mesos API handler never uses 404, but use 410 instead. But the semantics of "GONE" dose not quite match "NOT FOUND".
    
    (2) Make libprocess returns 503 in the beginning, and then at certain point of time, mark libprocess as "initialized", meaning that no more routes will be added, and after that libprocess could return 404 if a route is added.
    
    Along with the discussion of (2), people seems to agree that in most cases dynamic addition of routes is not very useful and (2) seems a viable solution. There's no conclusion yet, but if we're going to follow (2), I'd avoid adding `/api/v1/resource_provider` here, but just registering this in `Slave::initialize()`, and let the resource provider manager rejects requests until it obtains the slave ID. This is also what I did for `LocalResourceProviderDaemon`: https://github.com/apache/mesos/blob/master/src/resource_provider/daemon.cpp#L147.
    
    Before we establish the convention, I'd suggest that we avoid adding routes after `Slave::initialize()`. Thoughts?


- 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/66308/
> -----------------------------------------------------------
> 
> (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
> -------
> 
> By delaying the construction of the agent's resource provider manager
> we prepare for a following patch introducing a dependency on the
> resource provider manager on the agent's ID.
> 
> Depending on whether the agent was able to recover an agent ID from
> its log or still needs to obtain on in a first registration with the
> master, we can only construct the resource provider manager after
> different points in the initialization of the agent. To capture the
> common code we introduce a helper function performing the necessary
> steps.
> 
> 
> Diffs
> -----
> 
>   src/slave/slave.hpp d00c7b2e2466fd1de71e9e55343a27d3808bde3e 
>   src/slave/slave.cpp e5d6c3fac5054a6b0a0b5b77abd850a35be6ccc5 
>   src/tests/resource_provider_manager_tests.cpp c52541bf130ccf4795b989b53331176a64a097ea 
> 
> 
> Diff: https://reviews.apache.org/r/66308/diff/3/
> 
> 
> Testing
> -------
> 
> `make check`
> 
> 
> Thanks,
> 
> Benjamin Bannier
> 
>


Re: Review Request 66308: Delayed construction of the agent's resource provider manager.

Posted by Benjamin Bannier <be...@mesosphere.io>.

> On April 18, 2018, 8:58 p.m., Chun-Hung Hsiao wrote:
> > src/slave/slave.cpp
> > Lines 839 (patched)
> > <https://reviews.apache.org/r/66308/diff/4/?file=2005439#file2005439line848>
> >
> >     One line apart. Actually, let's move this back to it's original place so all `/api/v1/*` endpoints are declared together.

I moved this up to be grouped with the routes of `/api/v1` and `/api/v1/executor`.


> On April 18, 2018, 8:58 p.m., Chun-Hung Hsiao wrote:
> > src/slave/slave.cpp
> > Lines 8189 (patched)
> > <https://reviews.apache.org/r/66308/diff/4/?file=2005439#file2005439line8200>
> >
> >     Maybe add a comment saying that checking `additionalResources` is enough because all resources needs to be published must have been checked before?

I am not sure that is what is happening here. The assertion here just validates that no new additional resources from RPs enter while we haven't even initialized the RP manager. This should hold as RP resources can only appear after a RP has subscribed which requires the manager. I don't believe that this performs a very stringent check, and we just perform the easy check here.

I added a comment to that effect, PTAL.


- Benjamin


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/66308/#review201438
-----------------------------------------------------------


On April 18, 2018, 4:28 p.m., Benjamin Bannier wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/66308/
> -----------------------------------------------------------
> 
> (Updated April 18, 2018, 4: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
> -------
> 
> By delaying the construction of the agent's resource provider manager
> we prepare for a following patch introducing a dependency on the
> resource provider manager on the agent's ID.
> 
> Depending on whether the agent was able to recover an agent ID from
> its log or still needs to obtain on in a first registration with the
> master, we can only construct the resource provider manager after
> different points in the initialization of the agent. To capture the
> common code we introduce a helper function performing the necessary
> steps.
> 
> 
> Diffs
> -----
> 
>   src/slave/slave.hpp d00c7b2e2466fd1de71e9e55343a27d3808bde3e 
>   src/slave/slave.cpp e5d6c3fac5054a6b0a0b5b77abd850a35be6ccc5 
>   src/tests/resource_provider_manager_tests.cpp c52541bf130ccf4795b989b53331176a64a097ea 
> 
> 
> Diff: https://reviews.apache.org/r/66308/diff/4/
> 
> 
> Testing
> -------
> 
> `make check`
> 
> 
> Thanks,
> 
> Benjamin Bannier
> 
>


Re: Review Request 66308: Delayed construction of the agent's 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/66308/#review201438
-----------------------------------------------------------


Fix it, then Ship it!





src/slave/slave.cpp
Lines 839 (patched)
<https://reviews.apache.org/r/66308/#comment282676>

    One line apart. Actually, let's move this back to it's original place so all `/api/v1/*` endpoints are declared together.



src/slave/slave.cpp
Lines 8189 (patched)
<https://reviews.apache.org/r/66308/#comment282678>

    Maybe add a comment saying that checking `additionalResources` is enough because all resources needs to be published must have been checked before?


- Chun-Hung Hsiao


On April 18, 2018, 2:28 p.m., Benjamin Bannier wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/66308/
> -----------------------------------------------------------
> 
> (Updated April 18, 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
> -------
> 
> By delaying the construction of the agent's resource provider manager
> we prepare for a following patch introducing a dependency on the
> resource provider manager on the agent's ID.
> 
> Depending on whether the agent was able to recover an agent ID from
> its log or still needs to obtain on in a first registration with the
> master, we can only construct the resource provider manager after
> different points in the initialization of the agent. To capture the
> common code we introduce a helper function performing the necessary
> steps.
> 
> 
> Diffs
> -----
> 
>   src/slave/slave.hpp d00c7b2e2466fd1de71e9e55343a27d3808bde3e 
>   src/slave/slave.cpp e5d6c3fac5054a6b0a0b5b77abd850a35be6ccc5 
>   src/tests/resource_provider_manager_tests.cpp c52541bf130ccf4795b989b53331176a64a097ea 
> 
> 
> Diff: https://reviews.apache.org/r/66308/diff/4/
> 
> 
> Testing
> -------
> 
> `make check`
> 
> 
> Thanks,
> 
> Benjamin Bannier
> 
>


Re: Review Request 66308: Delayed construction of the agent's 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/66308/
-----------------------------------------------------------

(Updated April 20, 2018, 12:23 p.m.)


Review request for mesos, Chun-Hung Hsiao, Jie Yu, and Jan Schlicht.


Changes
-------

Added a comment as suggested by Chun.


Bugs: MESOS-8735
    https://issues.apache.org/jira/browse/MESOS-8735


Repository: mesos


Description
-------

By delaying the construction of the agent's resource provider manager
we prepare for a following patch introducing a dependency on the
resource provider manager on the agent's ID.

Depending on whether the agent was able to recover an agent ID from
its log or still needs to obtain on in a first registration with the
master, we can only construct the resource provider manager after
different points in the initialization of the agent. To capture the
common code we introduce a helper function performing the necessary
steps.


Diffs (updated)
-----

  src/slave/slave.hpp d00c7b2e2466fd1de71e9e55343a27d3808bde3e 
  src/slave/slave.cpp e5d6c3fac5054a6b0a0b5b77abd850a35be6ccc5 
  src/tests/resource_provider_manager_tests.cpp c52541bf130ccf4795b989b53331176a64a097ea 


Diff: https://reviews.apache.org/r/66308/diff/6/

Changes: https://reviews.apache.org/r/66308/diff/5-6/


Testing
-------

`make check`


Thanks,

Benjamin Bannier


Re: Review Request 66308: Delayed construction of the agent's 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/66308/#review201605
-----------------------------------------------------------




src/slave/slave.cpp
Lines 8849 (patched)
<https://reviews.apache.org/r/66308/#comment282930>

    How about adding a comment to explain why this might be called twice and we should ignore the subsequent call?


- 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/66308/
> -----------------------------------------------------------
> 
> (Updated April 19, 2018, 9:19 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
> -------
> 
> By delaying the construction of the agent's resource provider manager
> we prepare for a following patch introducing a dependency on the
> resource provider manager on the agent's ID.
> 
> Depending on whether the agent was able to recover an agent ID from
> its log or still needs to obtain on in a first registration with the
> master, we can only construct the resource provider manager after
> different points in the initialization of the agent. To capture the
> common code we introduce a helper function performing the necessary
> steps.
> 
> 
> Diffs
> -----
> 
>   src/slave/slave.hpp d00c7b2e2466fd1de71e9e55343a27d3808bde3e 
>   src/slave/slave.cpp e5d6c3fac5054a6b0a0b5b77abd850a35be6ccc5 
>   src/tests/resource_provider_manager_tests.cpp c52541bf130ccf4795b989b53331176a64a097ea 
> 
> 
> Diff: https://reviews.apache.org/r/66308/diff/5/
> 
> 
> Testing
> -------
> 
> `make check`
> 
> 
> Thanks,
> 
> Benjamin Bannier
> 
>


Re: Review Request 66308: Delayed construction of the agent's 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/66308/
-----------------------------------------------------------

(Updated April 19, 2018, 11:19 a.m.)


Review request for mesos, Chun-Hung Hsiao, Jie Yu, and Jan Schlicht.


Changes
-------

Addressed more issues raised by Chun.


Bugs: MESOS-8735
    https://issues.apache.org/jira/browse/MESOS-8735


Repository: mesos


Description
-------

By delaying the construction of the agent's resource provider manager
we prepare for a following patch introducing a dependency on the
resource provider manager on the agent's ID.

Depending on whether the agent was able to recover an agent ID from
its log or still needs to obtain on in a first registration with the
master, we can only construct the resource provider manager after
different points in the initialization of the agent. To capture the
common code we introduce a helper function performing the necessary
steps.


Diffs (updated)
-----

  src/slave/slave.hpp d00c7b2e2466fd1de71e9e55343a27d3808bde3e 
  src/slave/slave.cpp e5d6c3fac5054a6b0a0b5b77abd850a35be6ccc5 
  src/tests/resource_provider_manager_tests.cpp c52541bf130ccf4795b989b53331176a64a097ea 


Diff: https://reviews.apache.org/r/66308/diff/5/

Changes: https://reviews.apache.org/r/66308/diff/4-5/


Testing
-------

`make check`


Thanks,

Benjamin Bannier


Re: Review Request 66308: Delayed construction of the agent's 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/66308/
-----------------------------------------------------------

(Updated April 18, 2018, 4:28 p.m.)


Review request for mesos, Chun-Hung Hsiao, Jie Yu, and Jan Schlicht.


Changes
-------

Addressed comments from Chun.


Bugs: MESOS-8735
    https://issues.apache.org/jira/browse/MESOS-8735


Repository: mesos


Description
-------

By delaying the construction of the agent's resource provider manager
we prepare for a following patch introducing a dependency on the
resource provider manager on the agent's ID.

Depending on whether the agent was able to recover an agent ID from
its log or still needs to obtain on in a first registration with the
master, we can only construct the resource provider manager after
different points in the initialization of the agent. To capture the
common code we introduce a helper function performing the necessary
steps.


Diffs (updated)
-----

  src/slave/slave.hpp d00c7b2e2466fd1de71e9e55343a27d3808bde3e 
  src/slave/slave.cpp e5d6c3fac5054a6b0a0b5b77abd850a35be6ccc5 
  src/tests/resource_provider_manager_tests.cpp c52541bf130ccf4795b989b53331176a64a097ea 


Diff: https://reviews.apache.org/r/66308/diff/4/

Changes: https://reviews.apache.org/r/66308/diff/3-4/


Testing
-------

`make check`


Thanks,

Benjamin Bannier


Re: Review Request 66308: Delayed construction of the agent's 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/66308/
-----------------------------------------------------------

(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
-------

By delaying the construction of the agent's resource provider manager
we prepare for a following patch introducing a dependency on the
resource provider manager on the agent's ID.

Depending on whether the agent was able to recover an agent ID from
its log or still needs to obtain on in a first registration with the
master, we can only construct the resource provider manager after
different points in the initialization of the agent. To capture the
common code we introduce a helper function performing the necessary
steps.


Diffs (updated)
-----

  src/slave/slave.hpp d00c7b2e2466fd1de71e9e55343a27d3808bde3e 
  src/slave/slave.cpp e5d6c3fac5054a6b0a0b5b77abd850a35be6ccc5 
  src/tests/resource_provider_manager_tests.cpp c52541bf130ccf4795b989b53331176a64a097ea 


Diff: https://reviews.apache.org/r/66308/diff/3/

Changes: https://reviews.apache.org/r/66308/diff/2-3/


Testing
-------

`make check`


Thanks,

Benjamin Bannier


Re: Review Request 66308: Delayed construction of the agent's 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/66308/
-----------------------------------------------------------

(Updated March 28, 2018, 4:28 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
-------

By delaying the construction of the agent's resource provider manager
we prepare for a following patch introducing a dependency on the
resource provider manager on the agent's ID.

Depending on whether the agent was able to recover an agent ID from
its log or still needs to obtain on in a first registration with the
master, we can only construct the resource provider manager after
different points in the initialization of the agent. To capture the
common code we introduce a helper function performing the necessary
steps.


Diffs (updated)
-----

  src/slave/slave.hpp 37f0361251524e63d02d251e8a03901812b8affb 
  src/slave/slave.cpp e76daabad0d2d68aa42d1da809d4a23459eaaacb 
  src/tests/resource_provider_manager_tests.cpp d947bd037190e6b7ea7b2277b5fbe47816878de4 


Diff: https://reviews.apache.org/r/66308/diff/2/

Changes: https://reviews.apache.org/r/66308/diff/1-2/


Testing
-------

`make check`


Thanks,

Benjamin Bannier