Re: Review Request 44486: Exposing ports to shell health checkers

2016-03-08 Thread Zameer Manji
Currently the health checker is just launched by the executor blindly. This proposal now plumbs information about the task to the subprocess. We already have a mechanism to do this, it's the DSL provided to users when they compose their processes. In that case we compose a command line with

Re: Review Request 44486: Exposing ports to shell health checkers

2016-03-08 Thread Bill Farner
> On March 8, 2016, 11:25 a.m., Zameer Manji wrote: > > The code for this approach looks fine to me, but I'm not sure if this > > approach is the way to go. > > > > Why can't the command for the health checker include > > '{{thermos.ports[http]}}' and we can resolve that value before

Re: Review Request 44486: Exposing ports to shell health checkers

2016-03-08 Thread Dmitriy Shirchenko
https://docs.python.org/2/library/subprocess.html#frequently-used-arguments Ok, sorry, so what exactly are you proposing I do instead of simply passing in environment variables? What themos code? What do we replace subprocess with? I'm new to this code base so I'm still figuring things out so

Re: Review Request 44486: Exposing ports to shell health checkers

2016-03-08 Thread Zameer Manji
> On March 8, 2016, 11:25 a.m., Zameer Manji wrote: > > The code for this approach looks fine to me, but I'm not sure if this > > approach is the way to go. > > > > Why can't the command for the health checker include > > '{{thermos.ports[http]}}' and we can resolve that value before

Re: Review Request 44486: Exposing ports to shell health checkers

2016-03-08 Thread Dmitriy Shirchenko
> On March 8, 2016, 7:25 p.m., Zameer Manji wrote: > > The code for this approach looks fine to me, but I'm not sure if this > > approach is the way to go. > > > > Why can't the command for the health checker include > > '{{thermos.ports[http]}}' and we can resolve that value before launching

Re: Review Request 44486: Exposing ports to shell health checkers

2016-03-08 Thread Zameer Manji
> On March 8, 2016, 11:25 a.m., Zameer Manji wrote: > > The code for this approach looks fine to me, but I'm not sure if this > > approach is the way to go. > > > > Why can't the command for the health checker include > > '{{thermos.ports[http]}}' and we can resolve that value before

Re: Review Request 44486: Exposing ports to shell health checkers

2016-03-08 Thread Dmitriy Shirchenko
> On March 8, 2016, 7:25 p.m., Zameer Manji wrote: > > The code for this approach looks fine to me, but I'm not sure if this > > approach is the way to go. > > > > Why can't the command for the health checker include > > '{{thermos.ports[http]}}' and we can resolve that value before launching

Re: Review Request 44486: Exposing ports to shell health checkers

2016-03-08 Thread Joshua Cohen
> On March 8, 2016, 7:25 p.m., Zameer Manji wrote: > > The code for this approach looks fine to me, but I'm not sure if this > > approach is the way to go. > > > > Why can't the command for the health checker include > > '{{thermos.ports[http]}}' and we can resolve that value before launching

Re: Review Request 44486: Exposing ports to shell health checkers

2016-03-08 Thread Zameer Manji
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/44486/#review122583 --- The code for this approach looks fine to me, but I'm not sure if

Re: Review Request 44486: Exposing ports to shell health checkers

2016-03-08 Thread Aurora ReviewBot
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/44486/#review122581 --- Master (26efe55) is red with this patch.

Re: Review Request 44486: Exposing ports to shell health checkers

2016-03-08 Thread Dmitriy Shirchenko
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/44486/ --- (Updated March 8, 2016, 6:32 p.m.) Review request for Aurora, John Sirois,

Re: Review Request 44486: Exposing ports to shell health checkers

2016-03-08 Thread John Sirois
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/44486/#review122537 --- Fix it, then Ship it! LGTM - thanks for the doc additions.

Re: Review Request 44486: Exposing ports to shell health checkers

2016-03-07 Thread Aurora ReviewBot
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/44486/#review122469 --- Ship it! Master (a91a759) is green with this patch.

Re: Review Request 44486: Exposing ports to shell health checkers

2016-03-07 Thread Dmitriy Shirchenko
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/44486/ --- (Updated March 8, 2016, 2:10 a.m.) Review request for Aurora, John Sirois,

Re: Review Request 44486: Exposing ports to shell health checkers

2016-03-07 Thread Dmitriy Shirchenko
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/44486/ --- (Updated March 8, 2016, 2:08 a.m.) Review request for Aurora, John Sirois,

Re: Review Request 44486: Exposing ports to shell health checkers

2016-03-07 Thread John Sirois
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/44486/#review122462 --- This code lgtm, but adding some docs that define what a shell

Re: Review Request 44486: Exposing ports to shell health checkers

2016-03-07 Thread Dmitriy Shirchenko
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/44486/ --- (Updated March 8, 2016, 1:42 a.m.) Review request for Aurora, John Sirois,