aurora-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "John Sirois" <john.sir...@gmail.com>
Subject Re: Review Request 40922: Thermos: Add ability to specify process outputs destination
Date Thu, 07 Jan 2016 19:27:48 GMT

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


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.


NEWS (line 13)
<https://reviews.apache.org/r/40922/#comment173800>

    Maybe s/has been added to/is configurable for/ - what you have works though.



NEWS (line 14)
<https://reviews.apache.org/r/40922/#comment173798>

    on how to configure...



docs/configuration-reference.md (line 157)
<https://reviews.apache.org/r/40922/#comment173799>

    s/Its/It's/ and ...to set where the process logs should be sent...



docs/configuration-reference.md (line 158)
<https://reviews.apache.org/r/40922/#comment173801>

    Its also possible to specify `console`... is more typical phrasing.



docs/deploying-aurora-scheduler.md (line 178)
<https://reviews.apache.org/r/40922/#comment173805>

    ... in the sandbox.



docs/deploying-aurora-scheduler.md (line 179)
<https://reviews.apache.org/r/40922/#comment173806>

    s/to specify/specifying/ and probably s/logs/log/, also the destination phrasing is a
bit awkward - here an edit suggestion for this line in full:
    allows specifying alternate log file destinations like streamed stdout/stderr or suppression
of all log output.



docs/deploying-aurora-scheduler.md (line 180)
<https://reviews.apache.org/r/40922/#comment173807>

    2 'the's will help here: ...for the entire cluster with the following flag...



src/main/python/apache/aurora/executor/bin/thermos_executor_main.py (line 108)
<https://reviews.apache.org/r/40922/#comment173808>

    "type of logger" worked on the LHS, but for destination and mode, flipping works better:
    `'The logger destination [%s] to use for all processes run by thermos.'`



src/main/python/apache/aurora/executor/bin/thermos_executor_main.py (line 117)
<https://reviews.apache.org/r/40922/#comment173809>

    This reads a bit better:
    `'The logger mode [%s] to use for all processes run by thermos.'`



src/main/python/apache/thermos/core/process.py (line 412)
<https://reviews.apache.org/r/40922/#comment173810>

    s/log_destiniation_resolver/log_destination_resolver/



src/main/python/apache/thermos/core/process.py (line 469)
<https://reviews.apache.org/r/40922/#comment173821>

    You can kill this method now, not used by any subclass and moved to `LogDestinationResolver`
which contains its use.



src/main/python/apache/thermos/core/process.py (line 506)
<https://reviews.apache.org/r/40922/#comment173815>

    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.



src/main/python/apache/thermos/core/process.py (line 565)
<https://reviews.apache.org/r/40922/#comment173819>

    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.



src/main/python/apache/thermos/core/process.py (line 712)
<https://reviews.apache.org/r/40922/#comment173813>

    s/to configure/configuration of/



src/main/python/apache/thermos/core/process.py (line 713)
<https://reviews.apache.org/r/40922/#comment173814>

    when ending a subprocess.



src/main/python/apache/thermos/core/process.py (line 734)
<https://reviews.apache.org/r/40922/#comment173812>

    mimicks the



src/main/python/apache/thermos/runner/thermos_runner.py (line 126)
<https://reviews.apache.org/r/40922/#comment173811>

    Consider `'The logger mode to use...'`



src/test/python/apache/thermos/core/test_process.py (line 326)
<https://reviews.apache.org/r/40922/#comment173827>

    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.



src/test/python/apache/thermos/core/test_process.py (line 400)
<https://reviews.apache.org/r/40922/#comment173824>

    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.



src/test/python/apache/thermos/core/test_process.py (line 402)
<https://reviews.apache.org/r/40922/#comment173826>

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



src/test/python/apache/thermos/core/test_process.py (line 446)
<https://reviews.apache.org/r/40922/#comment173822>

    Bad copy/paste comment - fix or drop.


- John Sirois


On Jan. 4, 2016, 5:25 a.m., Martin Hrabovcin wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/40922/
> -----------------------------------------------------------
> 
> (Updated Jan. 4, 2016, 5:25 a.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