----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/30039/#review71181 -----------------------------------------------------------
First pass, looks pretty good. Several minor suggestions, but it's almost shippable. src/launcher/fetcher.cpp <https://reviews.apache.org/r/30039/#comment116841> I think we usually like to keep our `using namespace` declarations above the other `using`s. Is there a reason you had to move it? src/launcher/fetcher.cpp <https://reviews.apache.org/r/30039/#comment116843> Update the comment to match your new parameter names: s/filename/sourcePath/ and s/directory/destinationDirectory/ src/launcher/fetcher.cpp <https://reviews.apache.org/r/30039/#comment116842> s/dest/destination/ src/launcher/fetcher.cpp <https://reviews.apache.org/r/30039/#comment116844> s/dest/destination/ src/launcher/fetcher.cpp <https://reviews.apache.org/r/30039/#comment116847> s/dest/destination/g src/launcher/fetcher.cpp <https://reviews.apache.org/r/30039/#comment116849> Merge these two log lines like you did for hadoop. src/launcher/fetcher.cpp <https://reviews.apache.org/r/30039/#comment116848> 'path' isn't used here anymore, and this check is already done in basename() when generating the destPath. src/launcher/fetcher.cpp <https://reviews.apache.org/r/30039/#comment116850> Weird indentation here. Please fix. src/launcher/fetcher.cpp <https://reviews.apache.org/r/30039/#comment116858> You return work_directory if you extract, but just return 'downloaded' otherwise? What's the logic here? workDir (when extracting) vs. workDir+basename? Does this need a comment? src/launcher/fetcher.cpp <https://reviews.apache.org/r/30039/#comment116860> s/dest/destination/ src/launcher/fetcher.cpp <https://reviews.apache.org/r/30039/#comment116859> s/Fetching/Downloading/? src/launcher/fetcher.cpp <https://reviews.apache.org/r/30039/#comment116851> CHECK_SOME src/launcher/fetcher.cpp <https://reviews.apache.org/r/30039/#comment116852> CHECK_SOME src/launcher/fetcher.cpp <https://reviews.apache.org/r/30039/#comment116853> CHECK_SOME src/launcher/fetcher.cpp <https://reviews.apache.org/r/30039/#comment116856> No need to LOG and EXIT. Just EXIT should be enough. src/launcher/fetcher.cpp <https://reviews.apache.org/r/30039/#comment116855> s/<<"/<< "/ src/launcher/fetcher.cpp <https://reviews.apache.org/r/30039/#comment116857> Remove the LOG(ERROR), and then EXIT with the message inline. Something looks weird about the message string wrapping onto so many lines at the 2-space indent mark. - Adam B On Jan. 25, 2015, 9:21 a.m., Bernd Mathiske wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/30039/ > ----------------------------------------------------------- > > (Updated Jan. 25, 2015, 9:21 a.m.) > > > Review request for mesos, Adam B, Benjamin Hindman, Till Toenshoff, and > Timothy Chen. > > > Bugs: MESOS-2069 > https://issues.apache.org/jira/browse/MESOS-2069 > > > Repository: mesos > > > Description > ------- > > Enables the fetcher cache actions prepared in the preceding patches by > implementing them in the mesos fetcher program. Also general cleanup of this > file, which is necessary to reduce complexity in and better understand the > resulting source code. > > > Diffs > ----- > > src/launcher/fetcher.cpp fed0105946da579a38357a30e7ae56e646e05b89 > > Diff: https://reviews.apache.org/r/30039/diff/ > > > Testing > ------- > > make check > > > Thanks, > > Bernd Mathiske > >
