> On May 26, 2018, 12:37 a.m., Chun-Hung Hsiao wrote: > > src/tests/resource_provider_manager_tests.cpp > > Lines 846 (patched) > > <https://reviews.apache.org/r/67009/diff/1/?file=2017945#file2017945line846> > > > > Since the purpose of this test and the one below is the registrar, not > > the underlying storage, it seems to me that the in-memory store is > > sufficient for this test (there is no need to create a new > > `mesos::state::Storage` instance), and also makes this test rely on fewer > > prerequisits. WDYT?
On construction the `Registrar` takes ownership of the given `storage`; this means we cannot meaningfully refer to the `storage` after that anymore. With that, it seems to only feasible way to recover the state from that storage afterwards would require reading from the file system which is why I choose a `LevelDBStorage` instead of e.g., a `InMemoryStorage`. Dropping. > On May 26, 2018, 12:37 a.m., Chun-Hung Hsiao wrote: > > src/tests/resource_provider_manager_tests.cpp > > Lines 847 (patched) > > <https://reviews.apache.org/r/67009/diff/1/?file=2017945#file2017945line847> > > > > Is there a reason why `os::getcwd()` is used here instead of > > `sandbox.get()`? Ditto below. Not really, other than ignorance on that value. Thanks for pointing this out, adjusted now. > On May 26, 2018, 12:37 a.m., Chun-Hung Hsiao wrote: > > src/tests/resource_provider_manager_tests.cpp > > Line 844 (original), 853 (patched) > > <https://reviews.apache.org/r/67009/diff/1/?file=2017945#file2017945line853> > > > > Could you briefly explain why you're advocating this pattern? I > > personally prefer the original pattern. Ditto below. We want to perform some checks on the recovered registry below, so we need to bind it to a name. I added a check here as well now. Dropping. > On May 26, 2018, 12:37 a.m., Chun-Hung Hsiao wrote: > > src/tests/resource_provider_manager_tests.cpp > > Lines 883-884 (patched) > > <https://reviews.apache.org/r/67009/diff/1/?file=2017945#file2017945line883> > > > > We could do `ASSERT_SOME_NE(nullptr, registrar);` here and below. Great idea. I ultimately went with `ASSERT_SOME_NE(Owned<Registrar>(nullptr), registrar)`. - Benjamin ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/67009/#review203911 ----------------------------------------------------------- On May 29, 2018, 10:54 a.m., Benjamin Bannier wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/67009/ > ----------------------------------------------------------- > > (Updated May 29, 2018, 10:54 a.m.) > > > Review request for mesos and Chun-Hung Hsiao. > > > Bugs: MESOS-8837 > https://issues.apache.org/jira/browse/MESOS-8837 > > > Repository: mesos > > > Description > ------- > > Added tests of resource provider registrar recovery. > > > Diffs > ----- > > src/tests/resource_provider_manager_tests.cpp > 77a59e4d285b455154990f9b5cf80525f26583c9 > > > Diff: https://reviews.apache.org/r/67009/diff/2/ > > > Testing > ------- > > `make check` > > > Thanks, > > Benjamin Bannier > >
