> On Aug. 2, 2019, 9:59 a.m., Chun-Hung Hsiao wrote: > > src/resource_provider/storage/provider.cpp > > Lines 749-768 (patched) > > <https://reviews.apache.org/r/71151/diff/2/?file=2158048#file2158048line749> > > > > ``` > > 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(); > > }); > > }); > > ```
This moves the initial backoff into the function, fixed the call sites for that. > On Aug. 2, 2019, 9:59 a.m., Chun-Hung Hsiao wrote: > > src/resource_provider/storage/provider.cpp > > Lines 756-760 (original), 801-821 (patched) > > <https://reviews.apache.org/r/71151/diff/2/?file=2158048#file2158048line806> > > > > 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? Great suggestion. > On Aug. 2, 2019, 9:59 a.m., Chun-Hung Hsiao wrote: > > src/resource_provider/storage/provider.cpp > > Lines 1869-1886 (original), 1933-1950 (patched) > > <https://reviews.apache.org/r/71151/diff/2/?file=2158048#file2158048line1938> > > > > 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`. While I agree that having less points where we reconcile is better, I feel we shouldn't leave reconciliations like this one to user-specified configs. What if a user sets a `reconciliation_interval_seconds` of `0`? -- we'd still want to reconcile in that case. Dropping for now, but removing mentions of MESOS-9254 from the code base. Please reopen if you feel strongly about this. - Benjamin ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/71151/#review217049 ----------------------------------------------------------- On Aug. 6, 2019, 4:27 p.m., Benjamin Bannier wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/71151/ > ----------------------------------------------------------- > > (Updated Aug. 6, 2019, 4:27 p.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/4/ > > > Testing > ------- > > `make check` > > > Thanks, > > Benjamin Bannier > >
