----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/4119/#review5586 -----------------------------------------------------------
Some style nits and suggestions for abstracting some code. This is awesome Flo! Thanks so much! src/Makefile.am <https://reviews.apache.org/r/4119/#comment12101> s/Library for handling/Libraries for performing src/launcher/launcher.cpp <https://reviews.apache.org/r/4119/#comment12102> Throw this one above <sys/...>. We try and organize these in alphabetically for each nesting level, first for the .h's, then the C++ style headers, then the .hpp's, then the local headers (i.e., the ones in quotes). src/launcher/launcher.cpp <https://reviews.apache.org/r/4119/#comment12103> No need to declare these up here, go ahead and put them where they are defined. src/launcher/launcher.cpp <https://reviews.apache.org/r/4119/#comment12104> To be clear, this means the process will be killed. At this point we haven't yet forked, so a user could construct an executor URI that kills the slave. Maybe this means we should actually have forked by this point? Or maybe this means this shouldn't be a CHECK. src/launcher/launcher.cpp <https://reviews.apache.org/r/4119/#comment12106> You should be able to get away with just a string here. src/launcher/launcher.cpp <https://reviews.apache.org/r/4119/#comment12108> It would be cool to see performing an HTTP request get factored out into it's own utility! ;) src/launcher/launcher.cpp <https://reviews.apache.org/r/4119/#comment12109> We use spaces between 'if' and '('. src/launcher/launcher.cpp <https://reviews.apache.org/r/4119/#comment12111> Indent +2 only please. src/launcher/launcher.cpp <https://reviews.apache.org/r/4119/#comment12112> Again, these fatal errors are before we have forked the executor ... this is really a bug on our part. src/launcher/launcher.cpp <https://reviews.apache.org/r/4119/#comment12113> Kill newline. - Benjamin On 2012-03-01 02:48:20, Florian Leibert wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/4119/ > ----------------------------------------------------------- > > (Updated 2012-03-01 02:48:20) > > > Review request for mesos and Benjamin Hindman. > > > Summary > ------- > > Launcher can now retrieve an executor specified as a HTTP Url. > Added dependencies of libcurl, ssl, crypto & z to the Makefile. > > > Diffs > ----- > > src/Makefile.am 5ed41bf > src/launcher/launcher.cpp 4422224 > > Diff: https://reviews.apache.org/r/4119/diff > > > Testing > ------- > > > Thanks, > > Florian > >
