You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@mesos.apache.org by Florian Leibert <fl...@leibert.de> on 2012/03/01 03:48:20 UTC
Review Request: Adding support to launcher.cpp to retrieve files (executors)
via HTTP
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/4119/
-----------------------------------------------------------
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
Re: Review Request: Adding support to launcher.cpp to retrieve files
(executors) via HTTP
Posted by Florian Leibert <fl...@leibert.de>.
> On 2012-03-02 20:38:28, Benjamin Hindman wrote:
> > src/launcher/launcher.cpp, line 262
> > <https://reviews.apache.org/r/4119/diff/1/?file=86909#file86909line262>
> >
> > Kill newline.
done
> On 2012-03-02 20:38:28, Benjamin Hindman wrote:
> > src/launcher/launcher.cpp, line 227
> > <https://reviews.apache.org/r/4119/diff/1/?file=86909#file86909line227>
> >
> > Again, these fatal errors are before we have forked the executor ... this is really a bug on our part.
it's forked already
> On 2012-03-02 20:38:28, Benjamin Hindman wrote:
> > src/launcher/launcher.cpp, line 218
> > <https://reviews.apache.org/r/4119/diff/1/?file=86909#file86909line218>
> >
> > We use spaces between 'if' and '('.
done
> On 2012-03-02 20:38:28, Benjamin Hindman wrote:
> > src/launcher/launcher.cpp, line 219
> > <https://reviews.apache.org/r/4119/diff/1/?file=86909#file86909line219>
> >
> > Indent +2 only please.
done
> On 2012-03-02 20:38:28, Benjamin Hindman wrote:
> > src/launcher/launcher.cpp, line 214
> > <https://reviews.apache.org/r/4119/diff/1/?file=86909#file86909line214>
> >
> > It would be cool to see performing an HTTP request get factored out into it's own utility! ;)
done
> On 2012-03-02 20:38:28, Benjamin Hindman wrote:
> > src/launcher/launcher.cpp, lines 209-210
> > <https://reviews.apache.org/r/4119/diff/1/?file=86909#file86909line209>
> >
> > You should be able to get away with just a string here.
done
> On 2012-03-02 20:38:28, Benjamin Hindman wrote:
> > src/launcher/launcher.cpp, line 206
> > <https://reviews.apache.org/r/4119/diff/1/?file=86909#file86909line206>
> >
> > 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.
it's forked already actually (discussed this yesterday)
> On 2012-03-02 20:38:28, Benjamin Hindman wrote:
> > src/launcher/launcher.cpp, lines 199-200
> > <https://reviews.apache.org/r/4119/diff/1/?file=86909#file86909line199>
> >
> > No need to declare these up here, go ahead and put them where they are defined.
done
> On 2012-03-02 20:38:28, Benjamin Hindman wrote:
> > src/launcher/launcher.cpp, line 32
> > <https://reviews.apache.org/r/4119/diff/1/?file=86909#file86909line32>
> >
> > 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).
done
> On 2012-03-02 20:38:28, Benjamin Hindman wrote:
> > src/Makefile.am, line 246
> > <https://reviews.apache.org/r/4119/diff/1/?file=86908#file86908line246>
> >
> > s/Library for handling/Libraries for performing
done
- Florian
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/4119/#review5586
-----------------------------------------------------------
On 2012-03-06 15:58:46, Florian Leibert wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/4119/
> -----------------------------------------------------------
>
> (Updated 2012-03-06 15:58:46)
>
>
> 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/launcher/launcher.cpp 9bbae7e
> src/Makefile.am 1d108ab
> src/common/utils.hpp 5a88f17
>
> Diff: https://reviews.apache.org/r/4119/diff
>
>
> Testing
> -------
>
>
> Thanks,
>
> Florian
>
>
Re: Review Request: Adding support to launcher.cpp to retrieve files
(executors) via HTTP
Posted by Benjamin Hindman <be...@berkeley.edu>.
-----------------------------------------------------------
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
>
>
Re: Review Request: Adding support to launcher.cpp to retrieve files
(executors) via HTTP
Posted by Florian Leibert <fl...@leibert.de>.
> On 2012-03-02 14:09:15, Charles Reiss wrote:
> > src/launcher/launcher.cpp, line 212
> > <https://reviews.apache.org/r/4119/diff/1/?file=86909#file86909line212>
> >
> > Check if this fails?
using the os::open wrapper now - in case an error happens the launcher would die.
> On 2012-03-02 14:09:15, Charles Reiss wrote:
> > src/launcher/launcher.cpp, line 222
> > <https://reviews.apache.org/r/4119/diff/1/?file=86909#file86909line222>
> >
> > Do you have some reason to believe that if this fails (e.g. connection closed before response is complete), then CURLINFO_RESPONSE_CODE won't be 200?
checking now the actual curl error code, i.e. not just the http status code.
> On 2012-03-02 14:09:15, Charles Reiss wrote:
> > src/launcher/launcher.cpp, line 236
> > <https://reviews.apache.org/r/4119/diff/1/?file=86909#file86909line236>
> >
> > Check return value? (e.g. out of disk space/quota).
done
- Florian
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/4119/#review5539
-----------------------------------------------------------
On 2012-03-06 15:58:46, Florian Leibert wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/4119/
> -----------------------------------------------------------
>
> (Updated 2012-03-06 15:58:46)
>
>
> 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/launcher/launcher.cpp 9bbae7e
> src/Makefile.am 1d108ab
> src/common/utils.hpp 5a88f17
>
> Diff: https://reviews.apache.org/r/4119/diff
>
>
> Testing
> -------
>
>
> Thanks,
>
> Florian
>
>
Re: Review Request: Adding support to launcher.cpp to retrieve files
(executors) via HTTP
Posted by Charles Reiss <wo...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/4119/#review5539
-----------------------------------------------------------
src/launcher/launcher.cpp
<https://reviews.apache.org/r/4119/#comment11962>
Check if this fails?
src/launcher/launcher.cpp
<https://reviews.apache.org/r/4119/#comment11964>
Do you have some reason to believe that if this fails (e.g. connection closed before response is complete), then CURLINFO_RESPONSE_CODE won't be 200?
src/launcher/launcher.cpp
<https://reviews.apache.org/r/4119/#comment11966>
Check return value? (e.g. out of disk space/quota).
- Charles
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
>
>
Re: Review Request: Adding support to launcher.cpp to retrieve files
(executors) via HTTP
Posted by Benjamin Hindman <be...@berkeley.edu>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/4119/#review6010
-----------------------------------------------------------
Ship it!
I'll get this committed, thanks Flo!
- Benjamin
On 2012-03-06 16:14:45, Florian Leibert wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/4119/
> -----------------------------------------------------------
>
> (Updated 2012-03-06 16:14:45)
>
>
> Review request for mesos and Benjamin Hindman.
>
>
> Summary
> -------
>
> Launcher can now retrieve an executor specified as a HTTP/FTP Url.
> Added dependencies of libcurl, ssl, crypto & z to the Makefile.
>
>
> Diffs
> -----
>
> src/launcher/launcher.cpp 9bbae7e
> src/Makefile.am 1d108ab
> src/common/utils.hpp 5a88f17
>
> Diff: https://reviews.apache.org/r/4119/diff
>
>
> Testing
> -------
>
>
> Thanks,
>
> Florian
>
>
Re: Review Request: Adding support to launcher.cpp to retrieve files
(executors) via HTTP
Posted by Florian Leibert <fl...@leibert.de>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/4119/
-----------------------------------------------------------
(Updated 2012-03-06 16:14:45.598280)
Review request for mesos and Benjamin Hindman.
Changes
-------
Addressed Charles' fclose issue
Summary
-------
Launcher can now retrieve an executor specified as a HTTP/FTP Url.
Added dependencies of libcurl, ssl, crypto & z to the Makefile.
Diffs (updated)
-----
src/launcher/launcher.cpp 9bbae7e
src/Makefile.am 1d108ab
src/common/utils.hpp 5a88f17
Diff: https://reviews.apache.org/r/4119/diff
Testing
-------
Thanks,
Florian
Re: Review Request: Adding support to launcher.cpp to retrieve files
(executors) via HTTP
Posted by Florian Leibert <fl...@leibert.de>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/4119/
-----------------------------------------------------------
(Updated 2012-03-06 16:09:47.630929)
Review request for mesos and Benjamin Hindman.
Summary (updated)
-------
Launcher can now retrieve an executor specified as a HTTP/FTP Url.
Added dependencies of libcurl, ssl, crypto & z to the Makefile.
Diffs
-----
src/launcher/launcher.cpp 9bbae7e
src/Makefile.am 1d108ab
src/common/utils.hpp 5a88f17
Diff: https://reviews.apache.org/r/4119/diff
Testing
-------
Thanks,
Florian
Re: Review Request: Adding support to launcher.cpp to retrieve files
(executors) via HTTP
Posted by Florian Leibert <fl...@leibert.de>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/4119/
-----------------------------------------------------------
(Updated 2012-03-06 15:58:46.087041)
Review request for mesos and Benjamin Hindman.
Changes
-------
Refactored the code into utils.
Added FTP / FTPs handling.
Summary
-------
Launcher can now retrieve an executor specified as a HTTP Url.
Added dependencies of libcurl, ssl, crypto & z to the Makefile.
Diffs (updated)
-----
src/launcher/launcher.cpp 9bbae7e
src/Makefile.am 1d108ab
src/common/utils.hpp 5a88f17
Diff: https://reviews.apache.org/r/4119/diff
Testing
-------
Thanks,
Florian