----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/18974/#review36964 -----------------------------------------------------------
Hey Vinod and Dominic, I have some concerns about this style of change where we're pulling out a one-off helper function. If you remember Vinod, we decided we really want to avoid this pattern as it introduces complexity. The discussion around this pattern first came up when we were thinking about pulling apart the Slave::initialize method into arbitrary helpers. src/slave/containerizer/mesos_containerizer.cpp <https://reviews.apache.org/r/18974/#comment68174> We tend to avoid introducing one-off "helper" functions to pull out arbitrary pieces of unique logic in the code. We're not achieving any simplification here. Instead we're strictly adding complexity. Now we have: (1) More code. (2) Readers of fetch() have to understand what buildCommand() does, and that it doesn't add the fetcher path. (3) Readers of mesos_containerizer.cpp need to understand what buildCommand() is for, who calls it (only 1 use of this function), and why we decided to pull it out (in this case only because we wanted to unit test this particular piece of logic). The unit tests for this are not checking that the command functions correctly, merely the format. So my question here is, rather than pulling out an arbitrary piece of fetch(), can fetch() be converted to a static function that we can test fully using local URIs? It looks like fetch is only using the 'flags' member. This way we can test the fetching in a more complete manner, including the output redirection. src/slave/containerizer/mesos_containerizer.cpp <https://reviews.apache.org/r/18974/#comment68161> Kill std:: qualifier as you did a few lines below. src/slave/containerizer/mesos_containerizer.cpp <https://reviews.apache.org/r/18974/#comment68162> Kill std:: qualifier as you did a few lines below. src/slave/containerizer/mesos_containerizer.cpp <https://reviews.apache.org/r/18974/#comment68163> Kill std:: qualifier as you did a few lines below. src/tests/containerizer_tests.cpp <https://reviews.apache.org/r/18974/#comment68166> Why did you introduce a new file here? Why have both: src/tests/containerizer.cpp src/tests/containerizer_tests.cpp src/tests/containerizer_tests.cpp <https://reviews.apache.org/r/18974/#comment68176> Did you need to use .c_str() and EXPECT_STREQ in these tests? - Ben Mahler On March 12, 2014, 6:33 p.m., Dominic Hamon wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/18974/ > ----------------------------------------------------------- > > (Updated March 12, 2014, 6:33 p.m.) > > > Review request for mesos, Ian Downes and Vinod Kone. > > > Bugs: MESOS-1050 and MESOS-1063 > https://issues.apache.org/jira/browse/MESOS-1050 > https://issues.apache.org/jira/browse/MESOS-1063 > > > Repository: mesos-git > > > Description > ------- > > see summary > > > Diffs > ----- > > src/Makefile.am c18b311bc578e6e56a76be95fbafc8d7d7712787 > src/slave/containerizer/mesos_containerizer.cpp > 9bf9829fb38f17d9a2a0d3ab33d45d34cd5d3ea5 > src/tests/containerizer_tests.cpp PRE-CREATION > > Diff: https://reviews.apache.org/r/18974/diff/ > > > Testing > ------- > > make check > > > Thanks, > > Dominic Hamon > >
