----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/35755/#review90423 -----------------------------------------------------------
src/launcher/fetcher.cpp (line 160) <https://reviews.apache.org/r/35755/#comment143465> See https://reviews.apache.org/r/36189, I added a "mode" to 'strings::trim' so that you can use it just for prefix or suffix trims too! Thus: string trimmedSourceUri = strings::trim(sourceUri, strings::PREFIX); Also, a pattern which we've used in the code base quite a bit is to "replace" 'sourceUri' by naming the parameter '_sourceUri' and then doing: string sourceUri = strings::trim(_sourceUri, strings::PREFIX); Alternatively, we've taken 'sourceUri' the parameter by value and then just done: sourceUri = strings::trim(sourceUri, strings::PREFIX); src/launcher/fetcher.cpp <https://reviews.apache.org/r/35755/#comment143466> We've used two newlines between top-level definitions to more easily contrast the definitions (here and the rest of this review please). src/tests/fetcher_tests.cpp (line 281) <https://reviews.apache.org/r/35755/#comment143468> What's the benefit of adding the new route? It seems like the extra test can still use the existing route (unless I'm missing something) and I'd rather not add it and leave people trying to understand why it was added). src/tests/fetcher_tests.cpp (line 292) <https://reviews.apache.org/r/35755/#comment143467> It's not clear to me what adding the process name does here? Perhaps I'm missing something, but I'd rather not have people reading the test trying to understand why we do it here but not other places unless it's actually necessary, and if it is, perhaps it needs a comment because it's not obvious to me! - Benjamin Hindman On June 30, 2015, 11:24 p.m., Artem Harutyunyan wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/35755/ > ----------------------------------------------------------- > > (Updated June 30, 2015, 11:24 p.m.) > > > Review request for mesos and Joris Van Remoortere. > > > Bugs: MESOS-2862 > https://issues.apache.org/jira/browse/MESOS-2862 > > > Repository: mesos > > > Description > ------- > > Fixes MESOS-2862 > > > Diffs > ----- > > src/launcher/fetcher.cpp 8aee4901ec1289f43b5fa6b830c3488815ec24cd > src/tests/fetcher_tests.cpp ae10c420f7dddb8650377c91b5343591e8560392 > > Diff: https://reviews.apache.org/r/35755/diff/ > > > Testing > ------- > > - make check > - added a test case for fetcher > > > Thanks, > > Artem Harutyunyan > >
