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/08/01 15:47:40 UTC
Review Request 68144: Added methods to remove resource providers from
provider manager.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/68144/
-----------------------------------------------------------
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
Re: Review Request 68144: Added methods to remove resource providers
from provider manager.
Posted by Benjamin Bannier <be...@mesosphere.io>.
> 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
>
>
Re: Review Request 68144: Added methods to remove resource providers
from 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/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
>
>
Re: Review Request 68144: Added methods to remove resource providers
from 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/68144/#review207555
-----------------------------------------------------------
Fix it, then Ship it!
src/resource_provider/manager.cpp
Lines 620 (patched)
<https://reviews.apache.org/r/68144/#comment290944>
How about `LOG(ERROR)`?
- Chun-Hung Hsiao
On Aug. 16, 2018, 9:16 p.m., Benjamin Bannier wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/68144/
> -----------------------------------------------------------
>
> (Updated Aug. 16, 2018, 9:16 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/5/
>
>
> Testing
> -------
>
> `make check`
>
> Additional testing with the test case added in https://reviews.apache.org/r/68147/.
>
>
> Thanks,
>
> Benjamin Bannier
>
>
Re: Review Request 68144: Added methods to remove resource providers
from provider manager.
Posted by Benjamin Bannier <be...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/68144/
-----------------------------------------------------------
(Updated Aug. 20, 2018, 11:52 a.m.)
Review request for mesos, Chun-Hung Hsiao and Jan Schlicht.
Changes
-------
Log an error.
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 (updated)
-----
src/resource_provider/manager.hpp 6c57956f0ec51820bfbe78e7fa213af2352b5a7f
src/resource_provider/manager.cpp abd7e38e5517ea600f9fc9b8a96c7d0d26df0620
src/resource_provider/message.hpp 9307f8859035dfafe0952e9026b078b44121537e
src/slave/slave.cpp 679394a549cdfe84d64a102164c8652ad96f1eb2
src/tests/resource_provider_manager_tests.cpp 0b9e985cf3b4ded8590615b418e52a2a11f1c1aa
Diff: https://reviews.apache.org/r/68144/diff/6/
Changes: https://reviews.apache.org/r/68144/diff/5-6/
Testing
-------
`make check`
Additional testing with the test case added in https://reviews.apache.org/r/68147/.
Thanks,
Benjamin Bannier
Re: Review Request 68144: Added methods to remove resource providers
from provider manager.
Posted by Benjamin Bannier <be...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/68144/
-----------------------------------------------------------
(Updated Aug. 16, 2018, 11:16 p.m.)
Review request for mesos, Chun-Hung Hsiao and Jan Schlicht.
Changes
-------
Log failed RP manager registrar updates.
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 (updated)
-----
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/4/
Changes: https://reviews.apache.org/r/68144/diff/3-4/
Testing
-------
`make check`
Additional testing with the test case added in https://reviews.apache.org/r/68147/.
Thanks,
Benjamin Bannier
Re: Review Request 68144: Added methods to remove resource providers
from provider manager.
Posted by Benjamin Bannier <be...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/68144/
-----------------------------------------------------------
(Updated Aug. 16, 2018, 4:31 p.m.)
Review request for mesos, Chun-Hung Hsiao and Jan Schlicht.
Changes
-------
Removed `TODO`.
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 (updated)
-----
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/3/
Changes: https://reviews.apache.org/r/68144/diff/2-3/
Testing
-------
`make check`
Additional testing with the test case added in https://reviews.apache.org/r/68147/.
Thanks,
Benjamin Bannier
Re: Review Request 68144: Added methods to remove resource providers
from provider manager.
Posted by Benjamin Bannier <be...@mesosphere.io>.
> On Aug. 15, 2018, 10:15 p.m., Chun-Hung Hsiao wrote:
> > src/resource_provider/manager.cpp
> > Lines 611-612 (patched)
> > <https://reviews.apache.org/r/68144/diff/2/?file=2072997#file2072997line611>
> >
> > These should be done in the agent since it knows about the task resources.
I implemented this in https://reviews.apache.org/r/68147/.
> On Aug. 15, 2018, 10:15 p.m., Chun-Hung Hsiao wrote:
> > src/resource_provider/manager.cpp
> > Lines 617 (patched)
> > <https://reviews.apache.org/r/68144/diff/2/?file=2072997#file2072997line617>
> >
> > Since we log the removing attempt above, should we also log the result as well? Or do you prefer leaving the logging to the caller?
We currently never log success scenarios. I'd leave this as is. We can always come back and add more debugging statements later, but I suspect that would not be _very_ useful as the code here usually does not involve long continuation chains.
Dropping.
> On Aug. 15, 2018, 10:15 p.m., Chun-Hung Hsiao wrote:
> > src/resource_provider/manager.cpp
> > Lines 625-626 (patched)
> > <https://reviews.apache.org/r/68144/diff/2/?file=2072997#file2072997line625>
> >
> > Just for consistency. Anaig, no strong preference.
> > ```
> > ResourceProviderMessage::Remove remove{resourceProviderId};
> > ```
Fixed.
- Benjamin
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/68144/#review207343
-----------------------------------------------------------
On Aug. 16, 2018, 4:31 p.m., Benjamin Bannier wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/68144/
> -----------------------------------------------------------
>
> (Updated Aug. 16, 2018, 4:31 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/3/
>
>
> Testing
> -------
>
> `make check`
>
> Additional testing with the test case added in https://reviews.apache.org/r/68147/.
>
>
> Thanks,
>
> Benjamin Bannier
>
>
Re: Review Request 68144: Added methods to remove resource providers
from provider manager.
Posted by Chun-Hung Hsiao <ch...@apache.org>.
> On Aug. 15, 2018, 8:15 p.m., Chun-Hung Hsiao wrote:
> > src/resource_provider/manager.cpp
> > Lines 617 (patched)
> > <https://reviews.apache.org/r/68144/diff/2/?file=2072997#file2072997line617>
> >
> > Since we log the removing attempt above, should we also log the result as well? Or do you prefer leaving the logging to the caller?
>
> Benjamin Bannier wrote:
> We currently never log success scenarios. I'd leave this as is. We can always come back and add more debugging statements later, but I suspect that would not be _very_ useful as the code here usually does not involve long continuation chains.
>
> Dropping.
Sorry for not being clear. I meant logging for failures. Re-opening this for feedbacks. Please feel free to drop again :)
- Chun-Hung
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/68144/#review207343
-----------------------------------------------------------
On Aug. 16, 2018, 2:31 p.m., Benjamin Bannier wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/68144/
> -----------------------------------------------------------
>
> (Updated Aug. 16, 2018, 2:31 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/3/
>
>
> Testing
> -------
>
> `make check`
>
> Additional testing with the test case added in https://reviews.apache.org/r/68147/.
>
>
> Thanks,
>
> Benjamin Bannier
>
>
Re: Review Request 68144: Added methods to remove resource providers
from provider manager.
Posted by Chun-Hung Hsiao <ch...@apache.org>.
> On Aug. 15, 2018, 8:15 p.m., Chun-Hung Hsiao wrote:
> > src/resource_provider/manager.cpp
> > Lines 617 (patched)
> > <https://reviews.apache.org/r/68144/diff/2/?file=2072997#file2072997line617>
> >
> > Since we log the removing attempt above, should we also log the result as well? Or do you prefer leaving the logging to the caller?
>
> Benjamin Bannier wrote:
> We currently never log success scenarios. I'd leave this as is. We can always come back and add more debugging statements later, but I suspect that would not be _very_ useful as the code here usually does not involve long continuation chains.
>
> Dropping.
>
> Chun-Hung Hsiao wrote:
> Sorry for not being clear. I meant logging for failures. Re-opening this for feedbacks. Please feel free to drop again :)
It seems that we simply return the failure without logging in Mesos:
https://github.com/apache/mesos/blob/master/src/slave/http.cpp#L531
So probably a good idea to log the failure here, on in the `Slave::removeResourceProvider()` function.
- Chun-Hung
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/68144/#review207343
-----------------------------------------------------------
On Aug. 16, 2018, 9:16 p.m., Benjamin Bannier wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/68144/
> -----------------------------------------------------------
>
> (Updated Aug. 16, 2018, 9:16 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/4/
>
>
> Testing
> -------
>
> `make check`
>
> Additional testing with the test case added in https://reviews.apache.org/r/68147/.
>
>
> Thanks,
>
> Benjamin Bannier
>
>
Re: Review Request 68144: Added methods to remove resource providers
from 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/68144/#review207343
-----------------------------------------------------------
Fix it, then Ship it!
src/resource_provider/manager.cpp
Lines 611-612 (patched)
<https://reviews.apache.org/r/68144/#comment290697>
These should be done in the agent since it knows about the task resources.
src/resource_provider/manager.cpp
Lines 617 (patched)
<https://reviews.apache.org/r/68144/#comment290699>
Since we log the removing attempt above, should we also log the result as well? Or do you prefer leaving the logging to the caller?
src/resource_provider/manager.cpp
Lines 625-626 (patched)
<https://reviews.apache.org/r/68144/#comment290698>
Just for consistency. Anaig, no strong preference.
```
ResourceProviderMessage::Remove remove{resourceProviderId};
```
- Chun-Hung Hsiao
On Aug. 15, 2018, 1: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, 1: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
>
>
Re: Review Request 68144: Added methods to remove resource providers
from provider manager.
Posted by Benjamin Bannier <be...@mesosphere.io>.
-----------------------------------------------------------
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.
Changes
-------
Rebased.
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 (updated)
-----
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/
Changes: https://reviews.apache.org/r/68144/diff/1-2/
Testing
-------
`make check`
Additional testing with the test case added in https://reviews.apache.org/r/68147/.
Thanks,
Benjamin Bannier