----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/50199/#review142913 -----------------------------------------------------------
src/launcher/fetcher.cpp (line 430) <https://reviews.apache.org/r/50199/#comment208584> Do we really want to check and fail on this condition? In the previous iteration you would have skipped creating the directory in that case. Also judging from the code `` const Option<string> cacheDirectory = fetcherInfo.get().has_cache_directory() ? Option<string>::some(fetcherInfo.get().cache_directory()) : Option<string>::none();``` below, it seems it is valid that there is no cache_directory. src/launcher/fetcher.cpp (line 432) <https://reviews.apache.org/r/50199/#comment208583> This comment should move one line down or? (as you now have the create logic in the loop as well). src/launcher/fetcher.cpp (line 441) <https://reviews.apache.org/r/50199/#comment208582> task's user? I am not a native speaker but 'as the task user' sounds strange to me, feel free to drop if correct. src/launcher/fetcher.cpp (line 504) <https://reviews.apache.org/r/50199/#comment208581> Does the name `result` make sense at this point? Maybe something like `cacheDirectory`? src/launcher/fetcher.cpp (line 508) <https://reviews.apache.org/r/50199/#comment208585> I personally would still use this cacheDirectory as parameter to createCacheDirectory for the following reason: we need to extract it anyhow and otherwise we do that at two different places. Feel free to drop if you have a different opinion. - Joerg Schad On July 19, 2016, 11:10 p.m., Greg Mann wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/50199/ > ----------------------------------------------------------- > > (Updated July 19, 2016, 11:10 p.m.) > > > Review request for mesos, Gilbert Song, Jie Yu, and Joerg Schad. > > > Bugs: MESOS-5845 > https://issues.apache.org/jira/browse/MESOS-5845 > > > Repository: mesos > > > Description > ------- > > The fetcher's launcher creates the fetcher cache > directory for each user immediately before an artifact > is fetched. In order to allow this directory to be > created by a different user than the user doing the > fetching, this patch factors out this directory > creation and places it before all fetches occur. > > > Diffs > ----- > > src/launcher/fetcher.cpp 0539b0182bd4a7178f103dddd1ab4fee8fc79eda > > Diff: https://reviews.apache.org/r/50199/diff/ > > > Testing > ------- > > `sudo make check` on both OSX and CentOS 7 was done at the end of this patch > chain. > > > Thanks, > > Greg Mann > >
