----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/42921/#review116829 -----------------------------------------------------------
Looks great, just some minor comments below. src/tests/master_tests.cpp (line 3993) <https://reviews.apache.org/r/42921/#comment177933> In general we'll add a newline between the end of your comment and the start of the TODO: ``` // Action to enqueue all received offers from SchedulerDriver.resourceOffers() // into a libprocess queue. Currently, this is only used in the // MaxCompletedTasksPerFrameworkFlag test below. // // TODO(klueska): Move this into src/tests/mesos.hpp if we decide it is more // generally useful. ``` Just to make it a bit easier to read when there are multiple TODOs, NOTEs, etc. Or it becomes sometimes hard to tell where a TODO ends and a comment starts again (e.g. if the TODO line ends with a period, is the next line part of the TODO or the comment?) But mind just moving this to mesos.hpp? Seems generally useful (at some point we should have a gmock.hpp or gmock/actions.hpp header). src/tests/master_tests.cpp (line 3998) <https://reviews.apache.org/r/42921/#comment177934> Sometimes we'll use single letter names, but it's more typical that we don't abbreviate here: s/o/offer/ src/tests/master_tests.cpp (line 4038) <https://reviews.apache.org/r/42921/#comment177935> Usually the type is omitted from the name, e.g. schedRegistered instead of schedRegisteredFuture. So the following would be more consistent with our naming style: ``` s/offersQueue/offers/ ``` - Ben Mahler On Jan. 28, 2016, 10:16 p.m., Kevin Klues wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/42921/ > ----------------------------------------------------------- > > (Updated Jan. 28, 2016, 10:16 p.m.) > > > Review request for mesos, Anand Mazumdar and Ben Mahler. > > > Bugs: MESOS-4518 > https://issues.apache.org/jira/browse/MESOS-4518 > > > Repository: mesos > > > Description > ------- > > Previously, this test was calling EXPECT_CALL() on > SchedulerDriver.resourceOffers() after the scheduler driver itself had > already been started. This resulted in flaky test results when offers > came in between schedDriver.start() and the EXPECT_CALL(). > > This commit fixes this test by moving the EXPECT_CALL() above the > schedDriver.start() call. However, because of the nature of this test, > this introduced a few complexities related to processing the incoming offers > in a loop later on. To allow this, we had to introduce a custom gmock > ACTION to enqueue offers in a process::Queue for later processing. In > the future we shoudl consider generalizing this custom ACTION or at > least moving it into src/tests/mesos.hpp for more general use. > > > Diffs > ----- > > src/tests/master_tests.cpp ce6ce25a03cdb0883612fe40b20996ec2e50de40 > > Diff: https://reviews.apache.org/r/42921/diff/ > > > Testing > ------- > > Tested locally on my mac, as well as with running support/docker_build.sh on > a linux machine. > > > Thanks, > > Kevin Klues > >
