> On Aug. 4, 2017, 8:58 a.m., Benjamin Bannier wrote: > > src/tests/containerizer/posix_rlimits_isolator_tests.cpp > > Lines 380-381 (patched) > > <https://reviews.apache.org/r/61387/diff/2/?file=1789190#file1789190line380> > > > > Indent by two spaces less.
The pattern in this file is to use 4 spaces, see the following tests (some of them added by you): https://github.com/apache/mesos/blob/cdb90b91ce8ce02d6163e5e2ee5b46fb797b1dee/src/tests/containerizer/posix_rlimits_isolator_tests.cpp#L70-L73 https://github.com/apache/mesos/blob/cdb90b91ce8ce02d6163e5e2ee5b46fb797b1dee/src/tests/containerizer/posix_rlimits_isolator_tests.cpp#L147-L149 https://github.com/apache/mesos/blob/cdb90b91ce8ce02d6163e5e2ee5b46fb797b1dee/src/tests/containerizer/posix_rlimits_isolator_tests.cpp#L227-L229 https://github.com/apache/mesos/blob/cdb90b91ce8ce02d6163e5e2ee5b46fb797b1dee/src/tests/containerizer/posix_rlimits_isolator_tests.cpp#L294-L296 clang-format suggests two spaces, and I agree with you both. So I'm thinking we should probably keep 4 spaces here and then have a different patch cleaning up the whitespaces in all these tests? > On Aug. 4, 2017, 8:58 a.m., Benjamin Bannier wrote: > > src/tests/containerizer/posix_rlimits_isolator_tests.cpp > > Lines 388 (patched) > > <https://reviews.apache.org/r/61387/diff/2/?file=1789190#file1789190line388> > > > > `ASSERT_FALSE(offers->empty())`. I agree that your proposal is more readable. Yet the other tests in this file and a LOT of other tests use `ASSERT_NE(0u, offers->size());`. I'm thinking again that we should keep it like this in this patch and then do a sweeping change? > On Aug. 4, 2017, 8:58 a.m., Benjamin Bannier wrote: > > src/tests/containerizer/posix_rlimits_isolator_tests.cpp > > Lines 392-396 (patched) > > <https://reviews.apache.org/r/61387/diff/2/?file=1789190#file1789190line392> > > > > Currently the declaration and first use are not on the same page for > > me. Let's move this right above its use, > > > > driver.acceptOffers({offer.id()}, {LAUNCH_GROUP(executorInfo, > > taskGroup)}) Moved this down. Now the `taskStatuses` declaration and expectations are kind of away from their use, but I think that's ok. > On Aug. 4, 2017, 8:58 a.m., Benjamin Bannier wrote: > > src/tests/containerizer/posix_rlimits_isolator_tests.cpp > > Lines 449 (patched) > > <https://reviews.apache.org/r/61387/diff/2/?file=1789190#file1789190line449> > > > > It would be nice to avoid hardcoding `4` here, e.g., > > > > foreach (Future<TaskStatus> update, updates) { > > EXPECT_CALL(sched, statusUpdate(&driver, _)) > > .WillOnce(FutureArg<1>(&update)); > > } > > Benjamin Bannier wrote: > Dropped a ref here, > > foreach (Future<TaskStatus>& update, updates) { > EXPECT_CALL(sched, statusUpdate(&driver, _)) > .WillOnce(FutureArg<1>(&update)); > } Done (but with 2 spaces to stay consistent with the rest of the file). > On Aug. 4, 2017, 8:58 a.m., Benjamin Bannier wrote: > > src/tests/containerizer/posix_rlimits_isolator_tests.cpp > > Lines 490 (patched) > > <https://reviews.apache.org/r/61387/diff/2/?file=1789190#file1789190line490> > > > > `s/updates[1]/taskStatus/`. Good catch =). - Gastón ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/61387/#review182189 ----------------------------------------------------------- On Aug. 4, 2017, 9:15 p.m., Gastón Kleiman wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/61387/ > ----------------------------------------------------------- > > (Updated Aug. 4, 2017, 9:15 p.m.) > > > Review request for mesos, Benjamin Bannier and Jie Yu. > > > Bugs: MESOS-7849 > https://issues.apache.org/jira/browse/MESOS-7849 > > > Repository: mesos > > > Description > ------- > > Make the rlimits isolator support nesting. Without this patch rlimits > sets for tasks launched by the DefaultExecutor are silently ignored. > > > Diffs > ----- > > src/slave/containerizer/mesos/isolators/posix/rlimits.hpp > ee36a24ae36179d3ff3141f8c3cdc768b1e399af > src/slave/containerizer/mesos/isolators/posix/rlimits.cpp > 3fc2b3dbd5b382e422c948adae0dc50f4fbcfc49 > src/tests/containerizer/posix_rlimits_isolator_tests.cpp > b7ccc7eeeb7e4d0d8f4fd463d506cfe7157076a4 > > > Diff: https://reviews.apache.org/r/61387/diff/4/ > > > Testing > ------- > > `bin/mesos-tests.sh > --gtest_filter="PosixRLimitsIsolatorTest.NestedContainers" > --gtest_repeat=1000 --gtest_break_on_failure` passed on a machine running > GNU/Linux. > > > Thanks, > > Gastón Kleiman > >
