> On Jan. 13, 2015, 11:25 a.m., Timothy Chen wrote: > > src/launcher/fetcher.cpp, line 285 > > <https://reviews.apache.org/r/29809/diff/2/?file=818721#file818721line285> > > > > So I think the reason I think the method looks like it needed to be > > refactored, it's just that it seems very cluttered overall. > > > > I downloaded your patches and went through this file myself, and did > > most spacing cleanups: > > > > https://gist.github.com/tnachen/e97cb242d31fb2f281e4 > > > > Mostly the following: > > > > - Leave spaces before returns and after some multiple line block > > - Extra space in the switch statement, following the same style > > src/log/leveldb.cpp > > - Remove extra LOG(ERROR), since we're already logging in LOG(ERROR) in > > the download code > > - Reduce number of parameters passed to download > > - Some small refactoring to reduce extra code > > > > Also I cleaned up some existing wierd formatting in the logging. > > > > If you like what I did then take the patch from the gist and apply a > > new review. > > Bernd Mathiske wrote: > Good stuff! Thanks, Tim! I am going to incorporate your changes. On this > trajectory, I found additional opportunities to furthr clean up and simplify > the code. So I will work these out, too. Stay tuned!
Tim, after applying your patch, I found more opportunity to clean up the code and went for it. I think I have addressed all 5 points you made, but the affected method now looks quite different in its gestalt, having been split up into subroutines. Also there is a new way of dealing with URIs and actions that is way simpler, but changes the code structure in this place and others. - Bernd ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/29809/#review67915 ----------------------------------------------------------- On Jan. 13, 2015, 5:27 a.m., Bernd Mathiske wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/29809/ > ----------------------------------------------------------- > > (Updated Jan. 13, 2015, 5:27 a.m.) > > > Review request for mesos, Benjamin Hindman and Timothy Chen. > > > Bugs: MESOS-2069 > https://issues.apache.org/jira/browse/MESOS-2069 > > > Repository: mesos-git > > > Description > ------- > > Enhances the fetcher program (launcher/fetcher.cpp) with cache "actions" and > an additional parameter in FetcherInfo: the cache filenames to be used for > these actions. > > > Diffs > ----- > > include/mesos/fetcher/fetcher.proto > facb87b92bf3194516f636dcc348e136af537721 > src/launcher/fetcher.cpp fed0105946da579a38357a30e7ae56e646e05b89 > src/slave/containerizer/fetcher.hpp > 1db0eaf002c8d0eaf4e0391858e61e0912b35829 > src/slave/containerizer/fetcher.cpp > 5993670f7899233efa1e6acef4b0c7856e32f748 > > Diff: https://reviews.apache.org/r/29809/diff/ > > > Testing > ------- > > make check. > > For now it is OK if only the FETCH action works, which implements the legacy > behavior without any caching. > > The new functionality will be tested in the context of MESOS-2074. > > > Thanks, > > Bernd Mathiske > >
