----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/56941/#review166458 -----------------------------------------------------------
Looks good overall, left some suggestions below. Main thing is I think we can simplify the test a bit if we don't need the mock executor and test containerizer. Also, seems these types of tests should be collected in a file called upgrade_tests.cpp so that we can more easily migrate them towards an approach that actually spins up different versions of components. src/tests/master_tests.cpp (lines 6690 - 6695) <https://reviews.apache.org/r/56941/#comment238406> I would suggest moving this into a file called upgrade_tests.cpp with a comment at the top like the following: ``` // This file contains upgrade integration tests. Note that tests // in this file can only "spoof" an old version of a component, // since these run against the current version of the repository. // The long term plan for integration tests is to checkout // different releases of the repository and run components from // different releases against each other. ``` That way we can hopefully collect all of the upgrade tests together, and ensure that these tests are migrated to tests that actually spin up different versions. src/tests/master_tests.cpp (lines 6690 - 6694) <https://reviews.apache.org/r/56941/#comment238413> We should clarify here that we start a regular agent to get a task launched, then we restart the master and when the agent re-registers we strip the allocation info in order to spoof an old (non-MULTI_ROLE agent). src/tests/master_tests.cpp (lines 6704 - 6705) <https://reviews.apache.org/r/56941/#comment238407> Why did you need a mock executor and test containerizer? Seems to me this can be simplified by just running a sleep task with no mocking? E.g. ``` TaskInfo task = createTask(offers1.get()[0], "sleep 1000"); ``` You can find examples with: ``` $ grep -R -i sleep src/test ``` src/tests/master_tests.cpp (line 6783) <https://reviews.apache.org/r/56941/#comment238410> Is this flaky? Looking at the agent source, I believe this has to be `slaveFlags.registration_backoff_factor * 2`, no? src/tests/master_tests.cpp (lines 6796 - 6798) <https://reviews.apache.org/r/56941/#comment238412> You can wrap your loops like this if it fits in 80 characters: ``` foreach (ExecutorInfo& executorInfo, *strippedReregisterSlaveMessage.mutable_executor_infos()) { ``` src/tests/master_tests.cpp (line 6834) <https://reviews.apache.org/r/56941/#comment238408> Can you do a sweep for `s/.get()./->/` ? src/tests/master_tests.cpp (line 6838) <https://reviews.apache.org/r/56941/#comment238409> Feel free to use `.at("frameworks")` to avoid mutation here, ditto below for "tasks" and "executors" - Benjamin Mahler On Feb. 22, 2017, 5:43 p.m., Jay Guo wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/56941/ > ----------------------------------------------------------- > > (Updated Feb. 22, 2017, 5:43 p.m.) > > > Review request for mesos, Benjamin Bannier, Benjamin Mahler, Guangya Liu, and > Michael Park. > > > Bugs: MESOS-7063 > https://issues.apache.org/jira/browse/MESOS-7063 > > > Repository: mesos > > > Description > ------- > > When a non-MULTI_ROLE agent with running tasks re-registers, master > should inject allocation roles for tasks/executors sent during > re-registration. > > > Diffs > ----- > > src/tests/master_tests.cpp 357a9a47ef87be6cbc4256d8afabe63cd41b1ea1 > > Diff: https://reviews.apache.org/r/56941/diff/ > > > Testing > ------- > > make check > > > Thanks, > > Jay Guo > >
