> On March 12, 2014, 12:57 p.m., Ben Mahler wrote: > > src/slave/containerizer/mesos_containerizer.cpp, lines 56-61 > > <https://reviews.apache.org/r/18974/diff/4/?file=517556#file517556line56> > > > > 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. > > Dominic Hamon wrote: > If all of 'fetch' were replaced by a series of (well-named) helper > functions then fetch would be much easier to understand. The details of what > each helper does are irrelevant to someone reading the code. As such, this > would be simpler. I think the issue here is that the helper function is not > well-named, and it's only one that has been pulled out. > > The unit tests are testing the function which returns a well-formed and > particular string given the inputs. That's the point of a unit test. Testing > all of fetch is possible, but unhelpful given all the parameters and variants > that would need to be covered. > > Instead, I'd argue that fetch should be a series of calls to helper > functions, each of which should be unit tested. Also, that fetcher should > have its own unit test suite. > > Can fetch() be converted to a static function? Yes. But it should not be > tested in that form. > > Should fetch() have better test coverage? Yes. > > Ben Mahler wrote: > Chatted with Dominic about this. > > We can simplify fetch() by having subprocess take an environment map and > test in the Subprocess tests that the environment is correctly set in the > forked subprocess. > > This way, fetch() only has to correctly construct the environment map > from the commandInfo and other information, no escaping necessary. This > conversion from CommandInfo -> environment map is something we could pull out > as a clear "pure" transformation function, if we'd like to unit test it and > possibly re-use it. > > In general, I would like us to take a step back and understand why code > is complex when we attempt to simplify or when we struggle to test things. > Having the right abstraction boundaries are essential to keeping our code > base clear and maintainable. > > Ben Mahler wrote: > Another example of taking a step back, why are we constructing these > janky "+0", "+1" environment variables in the first place? > > I chatted with idownes and benh about the origins of this. If subprocess > took an environment map, benh proposed that we could avoid conversion > altogether and simply pass the serialized protobufs as environment variables > and de-serialize them in the fetcher. Even better, we could use the > protobuf<->JSON conversion to ensure the command remains human readable! This > will dramatically simplify things.
https://reviews.apache.org/19162 https://reviews.apache.org/19164 - Dominic ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/18974/#review36964 ----------------------------------------------------------- On March 12, 2014, 11:33 a.m., Dominic Hamon wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/18974/ > ----------------------------------------------------------- > > (Updated March 12, 2014, 11:33 a.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 > >
