mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Chun-Hung Hsiao <chhs...@apache.org>
Subject Re: Review Request 66308: Delayed construction of the agent's resource provider manager.
Date Wed, 18 Apr 2018 03:03:05 GMT

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


Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message