> On Feb. 24, 2018, 12:11 a.m., Jie Yu wrote: > > src/resource_provider/storage/provider.cpp > > Line 1304 (original), 1329 (patched) > > <https://reviews.apache.org/r/65594/diff/3/?file=1959556#file1959556line1404> > > > > This flag is a bit confusing. When we will set this flag to `true`? Do > > we need to set it to true when `reconcileStoragePools` is called > > (currently, it's not)? > > > > What if `GetCapacity` reports shrinked capacity? Should we drop > > `CreateVolume` in that case?
Refactored in a followup patch: https://reviews.apache.org/r/65975/ It is fine for `GetCapacity` to shrink the resources since will wait for all pending `CreateVolume` to complete before calling `GetCapacity`. > On Feb. 24, 2018, 12:11 a.m., Jie Yu wrote: > > src/resource_provider/storage/provider.cpp > > Lines 2968-2969 (patched) > > <https://reviews.apache.org/r/65594/diff/3/?file=1959556#file1959556line3083> > > > > Do you need a dispatch here? Removed in https://reviews.apache.org/r/65975/. > On Feb. 24, 2018, 12:11 a.m., Jie Yu wrote: > > src/resource_provider/storage/provider.cpp > > Lines 2970-2971 (patched) > > <https://reviews.apache.org/r/65594/diff/3/?file=1959556#file1959556line3085> > > > > This is a bit confusing to me initially, and a bit hard to follow. > > > > I think the goal here is trying to prevent concurrent reconciliation. > > However, currently in the code, we used two ways to enforce that: > > 1) the loop for detecting profile changes (to prevent two > > reconcileProfiles run concurrently > > 2) operationSequence to prevent reconcileProfiles and > > reconcileStoragePool run concurrently > > > > Maybe we should use a unified way. Refactored in https://reviews.apache.org/r/65975/. > On Feb. 24, 2018, 12:11 a.m., Jie Yu wrote: > > src/resource_provider/storage/provider.cpp > > Lines 3087 (patched) > > <https://reviews.apache.org/r/65594/diff/3/?file=1959556#file1959556line3202> > > > > Do you want to add some CHECK here to check resource `has_disk`, > > `has_source` and `has_profile`? I think `CHECK(resource.disk().source().has_profile())` is sufficient and concise since if either `disk` or `source` is missing, we will get an empty `Resource::DiskInfo::Source`. - Chun-Hung ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/65594/#review197572 ----------------------------------------------------------- On March 8, 2018, 5:04 a.m., Chun-Hung Hsiao wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/65594/ > ----------------------------------------------------------- > > (Updated March 8, 2018, 5:04 a.m.) > > > Review request for mesos, James DeFelice, Jie Yu, and Joseph Wu. > > > Bugs: MESOS-8492 > https://issues.apache.org/jira/browse/MESOS-8492 > > > Repository: mesos > > > Description > ------- > > SLRP now checkpoints profiles associated with storage pools, and does > not depend on the `DiskProfileAdaptor` module to return the set of > previously-known profiles during recovery. > > > Diffs > ----- > > src/resource_provider/state.proto 8577b58b8cdb63b3daddf06ea5d12f80f9d42f2b > src/resource_provider/storage/provider.cpp > 63dde512fd8cc9f68f5f48a96869eb09b23b6f4a > > > Diff: https://reviews.apache.org/r/65594/diff/4/ > > > Testing > ------- > > sudo make check > > > Thanks, > > Chun-Hung Hsiao > >
