> On April 18, 2018, 5:03 a.m., Chun-Hung Hsiao wrote: > > src/slave/slave.hpp > > Line 815 (original), 819 (patched) > > <https://reviews.apache.org/r/66308/diff/3/?file=1994903#file1994903line819> > > > > This is inconsistent with the existing codebase. Could you justify why > > favoring `unique_ptr` over `Owned`?
I went for a `unique_ptr` while developing because it is much easier to use it correctly (see e.g., MESOS-5122). I changed the code to used `Owned` now for consistency. > On April 18, 2018, 5:03 a.m., Chun-Hung Hsiao wrote: > > src/slave/slave.cpp > > Line 4606 (original), 4601-4616 (patched) > > <https://reviews.apache.org/r/66308/diff/3/?file=1994904#file1994904line4610> > > > > 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. I like that suggestion, adopted. > On April 18, 2018, 5:03 a.m., Chun-Hung Hsiao wrote: > > src/slave/slave.cpp > > Lines 8177-8179 (patched) > > <https://reviews.apache.org/r/66308/diff/3/?file=1994904#file1994904line8188> > > > > Let's check that all resources are agent default resources. I have added an assertion to that effect here. > On April 18, 2018, 5:03 a.m., Chun-Hung Hsiao wrote: > > src/slave/slave.cpp > > Lines 8822 (patched) > > <https://reviews.apache.org/r/66308/diff/3/?file=1994904#file1994904line8833> > > > > `flags` is not used in this function. It will be used in https://reviews.apache.org/r/66310/ and I added it here already. > On April 18, 2018, 5:03 a.m., Chun-Hung Hsiao wrote: > > src/slave/slave.cpp > > Lines 8829 (patched) > > <https://reviews.apache.org/r/66308/diff/3/?file=1994904#file1994904line8840> > > > > Could you elaborate on this? Or make it a TODO if the comment is too > > complicated to be added ;) Ups, I did remove this in https://reviews.apache.org/r/66310/, and hadn't realized it was still _here_. Fixed the artifact here now. > On April 18, 2018, 5:03 a.m., Chun-Hung Hsiao wrote: > > src/slave/slave.cpp > > Lines 8833-8840 (patched) > > <https://reviews.apache.org/r/66308/diff/3/?file=1994904#file1994904line8844> > > > > 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 wrote: > Some typo: > "sech as" -> "such as" > "return 404 if a route is added" -> "return 404 if a route is *not* there" I changed the code to add the route statically on agent initialization, but not servicing it until a resource provider manager has be created. This seems to make sense as while the route is owned by the agent (and should have a corresponding lifetime), the agent's ability to serve it depends on whether the rp manager is available. Does that capture what you had in mind? - Benjamin ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/66308/#review201377 ----------------------------------------------------------- On April 18, 2018, 4:28 p.m., Benjamin Bannier wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/66308/ > ----------------------------------------------------------- > > (Updated April 18, 2018, 4:28 p.m.) > > > Review request for mesos, Chun-Hung Hsiao, 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/4/ > > > Testing > ------- > > `make check` > > > Thanks, > > Benjamin Bannier > >
