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