mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Benjamin Bannier <benjamin.bann...@mesosphere.io>
Subject Re: Review Request 67009: Added tests of resource provider registrar recovery.
Date Tue, 29 May 2018 08:54:35 GMT


> 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
> 
>


Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message