----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/55381/#review162351 -----------------------------------------------------------
Fix it, then Ship it! Thanks, looks good as far as validation is concerned. Seems we'll need a more comprehensive upgrade test to ensure a framework with running tasks can do the upgrade and continue to function normally, as well as launch new tasks in the new model. Just some minor comments below, I'll make the updates and get this committed. src/tests/master_validation_tests.cpp (lines 2589 - 2590) <https://reviews.apache.org/r/55381/#comment233698> Both empty would signify a change of roles since an empty `roles` indicates no roles rather than the default role of `"*"`. This test appears to be covering the case where they are both set to the same non-default value (so I'll update the comment), do you want to add another test that covers the `"*`" case or is the implementation agnostic to this case? src/tests/master_validation_tests.cpp (line 2602) <https://reviews.apache.org/r/55381/#comment233699> How about using Duration here? e.g. ``` frameworkInfo.set_failover_timeout(Weeks(1).secs()); ``` src/tests/master_validation_tests.cpp (lines 2618 - 2619) <https://reviews.apache.org/r/55381/#comment233700> How about a `driver.stop(failover=true); driver.join()` here to be more explicit? src/tests/master_validation_tests.cpp (lines 2642 - 2643) <https://reviews.apache.org/r/55381/#comment233701> How about a stop and join here to be consistent with other tests that register a scheduler driver above? - Benjamin Mahler On Jan. 18, 2017, 3:26 p.m., Benjamin Bannier wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/55381/ > ----------------------------------------------------------- > > (Updated Jan. 18, 2017, 3:26 p.m.) > > > Review request for mesos, Benjamin Mahler, Jay Guo, and Guangya Liu. > > > Bugs: MESOS-6900 > https://issues.apache.org/jira/browse/MESOS-6900 > > > Repository: mesos > > > Description > ------- > > See summary. > > > Diffs > ----- > > src/tests/master_validation_tests.cpp > c092362152e1fe8a6b615c2eda171d852c1bbd86 > > Diff: https://reviews.apache.org/r/55381/diff/ > > > Testing > ------- > > make check (OS X) > > > Thanks, > > Benjamin Bannier > >
