----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/71151/#review217049 -----------------------------------------------------------
src/resource_provider/storage/provider.cpp Lines 749-768 (patched) <https://reviews.apache.org/r/71151/#comment304269> ``` loop( self(), std::bind(&process::after, reconciliationInterval), [this](const Nothing&) { // Poll resource provider state ... reconciled = sequence.add(defer(self(), &Self::reconcileResources)); return reconciled.then([](const Nothing&) -> ControlFlow<Nothing> { return Continue(); }); }); ``` src/resource_provider/storage/provider.cpp Lines 756-760 (original), 801-821 (patched) <https://reviews.apache.org/r/71151/#comment304272> How about introduce a `bool alwaysUpdate` parameter in this function: ``` bool shouldUpdate = alwaysUpdate; if (result != totalResources) { LOG(INFO) << "Removing"... ; // Update the resource version... totalResources = result; resourceVersion = id::UUID::random(); checkpointResourceProviderState(); shouldUpdate = true; } if (shouldUpdate) { sendResourceProviderStateUpdate(); } ``` Then, in `reconcileResourceProviderState()`: ``` return reconcileOperationStatuses() .then(defer(self(), &Self::reconcileResources, true)) .then(defer(self(), [this] { statusUpdateManager.resume(); LOG(INFO) << "Resource provider " << info.id() << " is in READY state"; state = READY; return Nothing(); })); ``` So we don't put the state transition logic inside this function? src/resource_provider/storage/provider.cpp Lines 1356 (patched) <https://reviews.apache.org/r/71151/#comment304273> The delay won't guarantee the happens-before order between the first reconciliation and the ones in `watchResources`. How about: ``` reconciled = reconcileResourceProviderState() .onReady(defer(self(), &Self::watchProfiles)) .onReady(defer(self(), &Self::watchResources)) .onFailed(...) .onDiscarded(...); src/resource_provider/storage/provider.cpp Lines 1869-1886 (original), 1933-1950 (patched) <https://reviews.apache.org/r/71151/#comment304274> With periodic reconciliation, this won't be necessary. Removing this would also reduce a source of reconciliation, so the reconciliation can only happen in either `watchProfiles` or `watchResources`. - Chun-Hung Hsiao On July 29, 2019, 8:56 a.m., Benjamin Bannier wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/71151/ > ----------------------------------------------------------- > > (Updated July 29, 2019, 8:56 a.m.) > > > Review request for mesos and Chun-Hung Hsiao. > > > Bugs: MESOS-9254 > https://issues.apache.org/jira/browse/MESOS-9254 > > > Repository: mesos > > > Description > ------- > > Performed periodic storage local provider reconciliations. > > > Diffs > ----- > > src/resource_provider/storage/provider.cpp > 6d632606f411d3ca99d3573a57c9f68b02ba8072 > src/tests/storage_local_resource_provider_tests.cpp > 69b59d48ceefebbb7accefe411c54ac5cecff1c3 > > > Diff: https://reviews.apache.org/r/71151/diff/2/ > > > Testing > ------- > > `make check` > > > Thanks, > > Benjamin Bannier > >
