> On July 31, 2019, 7:23 a.m., Chun-Hung Hsiao wrote: > > src/tests/mesos.hpp > > Lines 3038 (patched) > > <https://reviews.apache.org/r/70728/diff/7/?file=2158025#file2158025line3041> > > > > It seems that I'm wrong about `Self`, which seems to become a type > > template in a class template. However, I've tested that we can simply use > > `MockResourceProviderProcess` (whitout any template argument) within this > > class without introducing a type alias. For example, use > > `&MockResourceProviderProcess::connectDefault`. > > > > That said, please feel free to drop this if you think introducing the > > `Self` type alias make the code more consistent.
You are right, for uses inside this class we can leave off the template args. I have a hunch that at some point I was required to refer to the type from the outside, but I am either wrong or that use case has gone away. Will remove the type alias altogether. > On July 31, 2019, 7:23 a.m., Chun-Hung Hsiao wrote: > > src/tests/mesos.hpp > > Lines 3151 (patched) > > <https://reviews.apache.org/r/70728/diff/7/?file=2158025#file2158025line3155> > > > > This introduces a coupling beween this test and the implementation of > > the JWT secret generator being synchoronous. > > > > How about the following instead: > > ``` > > process::Future<Option<std::string>> token = None(); > > > > #ifdef USE_SSL_SOCKET > > ... > > > > token = secretGenerator.generate(principal) > > .then([](const Secret& secret) -> Option<std::string> { > > return secret.value().data(); > > }); > > > > #endif > > > > // TODO(bbannier): Remove the `shared_ptr` once we get C++14. > > auto detector_ = > > > > std::make_shared<process::Owned<EndpointDetector>>(std::move(detector)); > > > > token.then(defer(self(), [=](const Option<std::string>& token) { > > driver.reset(new Driver( > > std::move(*detector_), > > ..., > > token)); > > > > driver->start(); > > > > return Nothing(); > > })); > > ``` I like this, thanks for the suggestion; adopted. > On July 31, 2019, 7:23 a.m., Chun-Hung Hsiao wrote: > > src/tests/mesos.hpp > > Lines 3302 (patched) > > <https://reviews.apache.org/r/70728/diff/7/?file=2158025#file2158025line3326> > > > > Do we use `FIXME` in the codebase? We don't, this was merely a note to myself which I missed to remove (I use `FIXME` instead of `TODO` precisely since we do not use it in the code and it can easily be greped for). Gone now. > On July 31, 2019, 7:23 a.m., Chun-Hung Hsiao wrote: > > src/tests/mesos.hpp > > Lines 3359 (patched) > > <https://reviews.apache.org/r/70728/diff/7/?file=2158025#file2158025line3383> > > > > No need to make it public anymore. We need to have `process` accessible from the outside so users can interact with the mock. While technically we could make the type alias private I am unsure what that would accomplish but possible future scratched heads. Dropping this, please reopen if I am missing the point. - Benjamin ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/70728/#review216979 ----------------------------------------------------------- On July 31, 2019, 11:31 a.m., Benjamin Bannier wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/70728/ > ----------------------------------------------------------- > > (Updated July 31, 2019, 11:31 a.m.) > > > Review request for mesos, Chun-Hung Hsiao and Jan Schlicht. > > > Bugs: MESOS-9560 > https://issues.apache.org/jira/browse/MESOS-9560 > > > Repository: mesos > > > Description > ------- > > We were passing callbacks into `MockResourceProvider` to the HTTP > driver. Since the lifecycle of the callbacks and the mock provider were > decoupled and these callbacks were binding the mock provider instance > the code was not safe as written as the driver could invoke the callback > after the provider had been destructed. > > This patch makes sure that the callbacks are defered to a process. We > also dispatch a number of other functions which are strongly coupled to > the lifecycle of the provider. We still do not hide the provider away > completely so the provider can be mocked in tests. > > > Diffs > ----- > > src/tests/api_tests.cpp 641eb15153ddb85df322aa6a133ca8e4c6d6a510 > src/tests/master_slave_reconciliation_tests.cpp > 7b6ac50adacc8416b91c0dde55ff7ba46a20515c > src/tests/master_tests.cpp b9ef13c31a9c3ae16e55d3ae8f9b1538a49cf49a > src/tests/mesos.hpp 4612e2e3d2bd32590248df58b546de8756636964 > src/tests/operation_reconciliation_tests.cpp > eae318da2273faae904f0155e49bb23cdb24f007 > src/tests/resource_provider_manager_tests.cpp > 7d48f18e89f046df6c92e52edeef592acfb13b10 > src/tests/slave_tests.cpp abee107720d6b78bb017d2762431ae36c0679026 > > > Diff: https://reviews.apache.org/r/70728/diff/8/ > > > Testing > ------- > > `make check` > > > Thanks, > > Benjamin Bannier > >
