> 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
> 
>

Reply via email to