> On May 5, 2020, 12:27 p.m., Andrei Sekretenko wrote: > > src/tests/slave_tests.cpp > > Lines 12227-12228 (patched) > > <https://reviews.apache.org/r/72368/diff/3/?file=2230178#file2230178line12324> > > > > I 'm not sure if these assertions add any value: these properties of V1 > > API are not specific to agent draining and are covered by corresponding > > tests. Maybe we can just remove them to improve readability?
Good call, removed. > On May 5, 2020, 12:27 p.m., Andrei Sekretenko wrote: > > src/tests/slave_tests.cpp > > Lines 12229 (patched) > > <https://reviews.apache.org/r/72368/diff/3/?file=2230178#file2230178line12326> > > > > Given that `drainConfig` in `EXPECT_EQ` below is different from default > > value of `DrainConfig`, isn't this assertion redundant as well? I think it's a bit better to explicitly assert here that the field has been set, since not doing so would require the reader to know something about the field's default value in order to reason about this code. - Greg ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/72368/#review220634 ----------------------------------------------------------- On May 5, 2020, 5:52 p.m., Greg Mann wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/72368/ > ----------------------------------------------------------- > > (Updated May 5, 2020, 5:52 p.m.) > > > Review request for mesos, Andrei Sekretenko and Benjamin Mahler. > > > Bugs: MESOS-10118 > https://issues.apache.org/jira/browse/MESOS-10118 > > > Repository: mesos > > > Description > ------- > > Previously, when the agent had no tasks or operations and > received a `DrainSlaveMessage`, it would checkpoint the > `DrainConfig` to disk, implicitly placing it into a "draining" > state indefinitely. This patch updates the agent's handler to > avoid checkpointing anything to disk in this case. > > The `SlaveTest.DrainInfoInAPIOutputs` test is also removed > and its functionality is moved into the test > `SlaveTest.DrainAgentKillsRunningTask`. The running task in > the latter test allows us to verify agent API outputs both > before and after the task's terminal update is acknowleged. > > > Diffs > ----- > > src/slave/slave.cpp 1a32c813eeacf21b903db3f9602d034e7fb085c0 > src/tests/slave_tests.cpp 5ad04b2fc494cb1120d2dc68a8cd40e5a27c129e > > > Diff: https://reviews.apache.org/r/72368/diff/4/ > > > Testing > ------- > > Ran the test in https://reviews.apache.org/r/72364/ with this patch applied > and verified that the final task reached TASK_RUNNING. > > > Thanks, > > Greg Mann > >
