----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/56537/#review165920 -----------------------------------------------------------
Fix it, then Ship it! src/tests/master_tests.cpp (line 6406) <https://reviews.apache.org/r/56537/#comment237764> `s/slave/agent/` src/tests/master_tests.cpp (line 6416) <https://reviews.apache.org/r/56537/#comment237773> Should we make explicit that this future will be ready, e.g., by awaiting it right before we await `offers`? src/tests/master_tests.cpp (line 6428) <https://reviews.apache.org/r/56537/#comment237767> I believe this needs to be an assertion, e.g., ASSERT_FALSE(offers->empty()); src/tests/master_tests.cpp (lines 6433 - 6435) <https://reviews.apache.org/r/56537/#comment237772> Since we are working on the default-constructed fields, `MergeFrom` is identical to `CopyFrom` here, right? src/tests/master_tests.cpp (line 6438) <https://reviews.apache.org/r/56537/#comment237770> You can drop `Times(1)` here as it is the default. src/tests/master_tests.cpp (lines 6450 - 6452) <https://reviews.apache.org/r/56537/#comment237765> `s/status.get()./status->/g' src/tests/master_tests.cpp (lines 6455 - 6456) <https://reviews.apache.org/r/56537/#comment237774> We can make explicit that we keep the same role and avoid a hardcoded value by inverting setting `roles` and clearing `role`, e.g., frameworkInfo.add_roles(frameworkInfo.role()); frameworkInfo.clear_role(); src/tests/master_tests.cpp (lines 6482 - 6489) <https://reviews.apache.org/r/56537/#comment237768> Seems like just using driver2.reconcileTasks({status.get()}); would be identical, but hardcode less information. src/tests/master_tests.cpp (line 6492) <https://reviews.apache.org/r/56537/#comment237766> `explicitReconciliation->state()` src/tests/master_tests.cpp (lines 6494 - 6495) <https://reviews.apache.org/r/56537/#comment237769> `driver1` would be inactive as soon as `registered2` above is ready. We could move this block just after that to create a bracket around the second registration, EXPECT_CALL(exec, shutdown(_)); driver2.start(); AWAIT_READY(registered2); driver1.stop(); driver1.join(); I believe stopping `driver1` here makes sure that `exec` will be around long enough so it actually can see its `shutdown` call allowing us to expect exactly one call (`.Times(1)` is the default for `EXPECT_CALL`). - Benjamin Bannier On Feb. 14, 2017, 2:49 a.m., Jay Guo wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/56537/ > ----------------------------------------------------------- > > (Updated Feb. 14, 2017, 2:49 a.m.) > > > Review request for mesos, Benjamin Bannier, Benjamin Mahler, Guangya Liu, and > Michael Park. > > > Bugs: MESOS-7035 > https://issues.apache.org/jira/browse/MESOS-7035 > > > Repository: mesos > > > Description > ------- > > A framework can be upgraded to be MULTI_ROLE capable even with task > running, as long as it does not change role while upgrading. > > > Diffs > ----- > > src/tests/master_tests.cpp 9a7a953adbbbed0b37082bb0166d03b1388c80ae > > Diff: https://reviews.apache.org/r/56537/diff/ > > > Testing > ------- > > make check > > > Thanks, > > Jay Guo > >
