mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Benjamin Bannier <benjamin.bann...@mesosphere.io>
Subject Re: Review Request 68144: Added methods to remove resource providers from provider manager.
Date Wed, 15 Aug 2018 13:53:20 GMT


> On Aug. 15, 2018, 3:57 a.m., Chun-Hung Hsiao wrote:
> > src/resource_provider/message.hpp
> > Lines 47 (patched)
> > <https://reviews.apache.org/r/68144/diff/1/?file=2065823#file2065823line47>
> >
> >     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?

I agree that the verbs we use for the types are a little weird, but they are also consistent
(both internally and with e.g., the form we use for master-agent message types). I believe
should we ever rename to some form based on the past participle we should also rename `UPDATE_STATE`
to e.g., `STATE_UPDATED` and similarly `UPDATE_OPERATION_STATUS` to e.g., `OPERATION_STATUS_UPDATED`.
I vote for keeping them as is for now.

Regarding the concern on duplication, I believe the setup form is fine as it allows us to
e.g., access resource provider information directly in the agent (or in the future: master)
actor which simplifies agent actions.

Dropping this for now.


> On Aug. 15, 2018, 3:57 a.m., Chun-Hung Hsiao wrote:
> > src/tests/resource_provider_manager_tests.cpp
> > Lines 1510-1515 (patched)
> > <https://reviews.apache.org/r/68144/diff/1/?file=2065825#file2065825line1510>
> >
> >     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`.

Agreed. I created https://issues.apache.org/jira/browse/MESOS-9155 to track such a cleanup.


- Benjamin


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


On Aug. 15, 2018, 3:53 p.m., Benjamin Bannier wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/68144/
> -----------------------------------------------------------
> 
> (Updated Aug. 15, 2018, 3:53 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 78e8666f402be58af5b6e20a715da4998af2615c 
>   src/tests/resource_provider_manager_tests.cpp 0b9e985cf3b4ded8590615b418e52a2a11f1c1aa

> 
> 
> Diff: https://reviews.apache.org/r/68144/diff/2/
> 
> 
> Testing
> -------
> 
> `make check`
> 
> Additional testing with the test case added in https://reviews.apache.org/r/68147/.
> 
> 
> Thanks,
> 
> Benjamin Bannier
> 
>


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