----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/21734/#review44187 -----------------------------------------------------------
Looks pretty good. Please also add "MESOS-390" to the "Bugs" field, and update the "Testing Done" field with at least 'make check', so we know you built and tested with your changes. As Niklas suggested, we will probably want a unit test for this change, but maybe it makes sense to ask Bernd how it would fit in with his new fetcher tests. src/launcher/fetcher.cpp <https://reviews.apache.org/r/21734/#comment78512> Why use find("://") + 3 if you already know that it starts with "file://"? Can't you just use local.substr(7)? src/launcher/fetcher.cpp <https://reviews.apache.org/r/21734/#comment78514> Can you LOG(ERROR) here too? Is there a reason you need to do this check here rather than rely on the local.find_first_of("/")!=0 below? Is one of these checks better than the other? I would think that startsWith would be able to exit earlier (after 1st char) than find_first_of (after 1st '/'). Perhaps we should change it too. - Adam B On May 21, 2014, 8:29 p.m., Timothy Chen wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/21734/ > ----------------------------------------------------------- > > (Updated May 21, 2014, 8:29 p.m.) > > > Review request for mesos, Adam B and Niklas Nielsen. > > > Repository: mesos-git > > > Description > ------- > > Handling file:// in the fetcher, and also handle case when localhost is used. > > > Diffs > ----- > > src/launcher/fetcher.cpp 8c9e20d > > Diff: https://reviews.apache.org/r/21734/diff/ > > > Testing > ------- > > > Thanks, > > Timothy Chen > >
