aurora-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Martin Hrabovcin" <mhrabov...@gmail.com>
Subject Re: Review Request 40922: Thermos: Add ability to specify process outputs destination
Date Fri, 08 Jan 2016 10:39:12 GMT


> On Jan. 7, 2016, 7:27 p.m., John Sirois wrote:
> > Sorry for all the comments late in the feedback loop, I just started looking at
this RB.  Everything LGTM and this is all mainly nitpicks.  I found 1 bug - marked as an issue
- that's not actually exposed but would be good to clean up.  The rest is advisory as you
see fit.

Thanks for the review. I've implemented all suggestions.


> On Jan. 7, 2016, 7:27 p.m., John Sirois wrote:
> > src/main/python/apache/thermos/core/process.py, line 531
> > <https://reviews.apache.org/r/40922/diff/8/?file=1180836#file1180836line531>
> >
> >     os.devnull is just a path string, not a stream; so line 537 below will raise
if I read this correctly.  This suggests another test should be added as well.  Alternatively,
just don;t default these.  FWICT the only caller of the constructor is Process.execute above
and that call always passes stdout and stderr and those are always proper "handlers" with
a write and a close.

Great catch, I've removed defaults.


> On Jan. 7, 2016, 7:27 p.m., John Sirois wrote:
> > src/main/python/apache/thermos/core/process.py, line 472
> > <https://reviews.apache.org/r/40922/diff/8/?file=1180836#file1180836line472>
> >
> >     You can kill this method now, not used by any subclass and moved to `LogDestinationResolver`
which contains its use.

Removed.


> On Jan. 7, 2016, 7:27 p.m., John Sirois wrote:
> > src/main/python/apache/thermos/core/process.py, line 412
> > <https://reviews.apache.org/r/40922/diff/8/?file=1180836#file1180836line412>
> >
> >     s/log_destiniation_resolver/log_destination_resolver/

Fixed


> On Jan. 7, 2016, 7:27 p.m., John Sirois wrote:
> > src/main/python/apache/thermos/core/process.py, line 597
> > <https://reviews.apache.org/r/40922/diff/8/?file=1180836#file1180836line597>
> >
> >     I'd generally try to validate befor setting fields; ie: blow up as early as
possible, but there is no bug doing it this way either and it seems surrounding code uses
this style of field setting, then validating.

I was trying to match style of existing code which does validation after assigning values
so if you're ok with I'd keep it as it is.


> On Jan. 7, 2016, 7:27 p.m., John Sirois wrote:
> > src/test/python/apache/thermos/core/test_process.py, line 402
> > <https://reviews.apache.org/r/40922/diff/8/?file=1180839#file1180839line402>
> >
> >     You could take advantage of [mutable_sys](https://github.com/twitter/commons/blob/master/src/python/twitter/common/contextutil/__init__.py#L168)
here:
> >     ```python
> >     with mutable_sys():
> >       sys.stdout, sys.stderr = stdout, stderr
> >     ```
> >     
> >     That will allow you to drop the output cleanups at the bottom and not be nagged
by the fact they aren't done in a finally block.
> >     
> >     Same goes for `test_log_tee`.

That is very handy helper.


> On Jan. 7, 2016, 7:27 p.m., John Sirois wrote:
> > src/test/python/apache/thermos/core/test_process.py, line 400
> > <https://reviews.apache.org/r/40922/diff/8/?file=1180839#file1180839line400>
> >
> >     Per the comment above, the `mock_` prefixes here are misleading, you might just
want to name these stderr and stdout.  Same for `test_log_tee` below.

Removed `mock_` prefix.


> On Jan. 7, 2016, 7:27 p.m., John Sirois wrote:
> > src/test/python/apache/thermos/core/test_process.py, line 326
> > <https://reviews.apache.org/r/40922/diff/8/?file=1180839#file1180839line326>
> >
> >     I think the preferred style is to replace the comment with a test function,
ie many small, focused test functions.  You can lift the nested assert function and this just
works with little code change otherwise.

I broke single test function to many `test_resolver_*` functions.


- Martin


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/40922/#review113281
-----------------------------------------------------------


On Jan. 4, 2016, 12:25 p.m., Martin Hrabovcin wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/40922/
> -----------------------------------------------------------
> 
> (Updated Jan. 4, 2016, 12:25 p.m.)
> 
> 
> Review request for Aurora and John Sirois.
> 
> 
> Bugs: AURORA-1548
>     https://issues.apache.org/jira/browse/AURORA-1548
> 
> 
> Repository: aurora
> 
> 
> Description
> -------
> 
> This patch will provide way to **optionally** specify running process outputs destination.
Implementation was built on top of https://reviews.apache.org/r/30695/
> 
> **What was changed:**
> 
> New `destination` parameter is available on global cluster level and also on each `Process`
level. Possible options are `file` (default), `stream` to parent process stdout/stderr, `mixed`
will split output to files and stream and finally `none` to discard any logs produced by running
process.
> 
> 
> Diffs
> -----
> 
>   NEWS c0c454d 
>   docs/configuration-reference.md cf63cfa 
>   docs/deploying-aurora-scheduler.md c0988e8 
>   src/main/python/apache/aurora/executor/bin/thermos_executor_main.py 4e9b027 
>   src/main/python/apache/aurora/executor/thermos_task_runner.py c019fc9 
>   src/main/python/apache/thermos/config/schema_base.py 5552108 
>   src/main/python/apache/thermos/core/process.py 8a181b0 
>   src/main/python/apache/thermos/core/runner.py 5623dce 
>   src/main/python/apache/thermos/runner/thermos_runner.py 3dacd45 
>   src/test/python/apache/thermos/core/test_process.py da4c494 
> 
> Diff: https://reviews.apache.org/r/40922/diff/
> 
> 
> Testing
> -------
> 
> Unit test coverage is provided for new functionality.
> 
> I did also manual testing with mesos/docker and I made sure that logs are being written
to expected files and also same output gets to docker daemon.
> 
> 
> Thanks,
> 
> Martin Hrabovcin
> 
>


Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message