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




src/resource_provider/message.hpp
Lines 47 (patched)
<https://reviews.apache.org/r/68144/#comment290619>

    I was wondering that if `SUBSCRIBED`, `DISCONNECTED`, and `REMOVED`/`GONE` 
are better names than the current ones, since we're notifying the agent about 
the state of a single resource provider.
    
    But on the other hand, `UPDATE_STATE` and `UPDATE_OPERATION_STATUS` sound 
reasonable to me.
    
    A more fundamental question would be: why do we need to duplicate these 
medatada in both the agent actor and the RP manager actor? Is it possible to 
refactor the agent by making some functions asynchronous and delegating 
RP-related state queries to the manager? WDYT?



src/tests/resource_provider_manager_tests.cpp
Lines 1510-1515 (patched)
<https://reviews.apache.org/r/68144/#comment290624>

    I was wondering if we could introduce some helper function such as `post` 
to avoid the code redundancy. But it might require us to put `manager` and 
`streamId` as data members of `ResourceProviderHttpApiTest`.


- Chun-Hung Hsiao


On Aug. 1, 2018, 3:47 p.m., Benjamin Bannier wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/68144/
> -----------------------------------------------------------
> 
> (Updated Aug. 1, 2018, 3:47 p.m.)
> 
> 
> Review request for mesos, Chun-Hung Hsiao and Jan Schlicht.
> 
> 
> Bugs: MESOS-8403
>     https://issues.apache.org/jira/browse/MESOS-8403
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> This patch adds a method to remove a resource provider from the
> resource provider manager. The resource provider will be marked as
> removed in the manager's registry and disconnected. We also expose a
> new `REMOVE` event whenever a resource provider was removed.
> 
> This patch does not add integration with e.g., the agent.
> 
> 
> Diffs
> -----
> 
>   src/resource_provider/manager.hpp 6c57956f0ec51820bfbe78e7fa213af2352b5a7f 
>   src/resource_provider/manager.cpp abd7e38e5517ea600f9fc9b8a96c7d0d26df0620 
>   src/resource_provider/message.hpp 9307f8859035dfafe0952e9026b078b44121537e 
>   src/slave/slave.cpp e574c249f81e0e77abe982c126fe210a6ee8b591 
>   src/tests/resource_provider_manager_tests.cpp 
> 0b9e985cf3b4ded8590615b418e52a2a11f1c1aa 
> 
> 
> Diff: https://reviews.apache.org/r/68144/diff/1/
> 
> 
> Testing
> -------
> 
> `make check`
> 
> Additional testing with the test case added in 
> https://reviews.apache.org/r/68147/.
> 
> 
> Thanks,
> 
> Benjamin Bannier
> 
>

Reply via email to