----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/65975/#review201629 -----------------------------------------------------------
First round of reviews. I am not a big fan on how reconcilations are modelled here. The counting seems to lead to an incomplete encapsulation of correct behavior. I'd much rather see standard `libprocess` actor semantics and better use of data structures instead. Right now the code appears too complicated and potentially brittle to the touch to me. src/resource_provider/storage/provider.cpp Lines 1069-1109 (patched) <https://reviews.apache.org/r/65975/#comment283061> Could we move this to some named function/lambda? I feel with this change the code here becomes too hard to follow. src/resource_provider/storage/provider.cpp Lines 1092 (patched) <https://reviews.apache.org/r/65975/#comment283062> Could you add a comment why this and the assertion in the continuation below are valid? The guaranteed execution paths of this `loop` with below `defer`s are not obvious to me. Ideally we would not need this variable at all to ensure correct push-pop semantics, but defer to actor behavior and queues. This is hidden under very thick continuation layers at the moment. src/resource_provider/storage/provider.cpp Lines 1105 (patched) <https://reviews.apache.org/r/65975/#comment283063> Please use a temporary, this does not help readability. src/resource_provider/storage/provider.cpp Lines 1284 (patched) <https://reviews.apache.org/r/65975/#comment283066> It would be great if this function would either return a value, or trigger `fatal()` to make sure we do our part to keep the agent up-to-date. - Benjamin Bannier On April 12, 2018, 5:36 a.m., Chun-Hung Hsiao wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/65975/ > ----------------------------------------------------------- > > (Updated April 12, 2018, 5:36 a.m.) > > > Review request for mesos, Benjamin Bannier and Jie Yu. > > > Bugs: MESOS-8492 > https://issues.apache.org/jira/browse/MESOS-8492 > > > Repository: mesos > > > Description > ------- > > The storage pools needs to be reconciled in the following two scenarios: > > 1. When there is a change in the set of known profiles. > 2. When a volume/block of an unknown profile is destroyed, because the > disk space being freed up may belong to a known profile. > > This patch adds a sequence to coordinate the reconciliations for the > above two cases. > > > Diffs > ----- > > src/resource_provider/storage/provider.cpp > a07620d1c4bf618f669575b3e183bf598da905a6 > > > Diff: https://reviews.apache.org/r/65975/diff/4/ > > > Testing > ------- > > sudo make check > > > Thanks, > > Chun-Hung Hsiao > >
