Re: Review Request 40922: Thermos: Add ability to specify process outputs destination

[John Sirois]

> On Jan. 8, 2016, 9:22 a.m., John Sirois wrote:
> > Thanks Martin!  Please mark this review as closed.
> > 
> > Now on master:
> > 
> > git log -1 origin/master
> > commit 024bac9dcb8f37e4b31210e3a0a7aea2345a16ab
> > Author: Martin Hrabovcin 
> > Date:   Fri Jan 8 09:18:11 2016 -0700
> > 
> > Thermos: Add ability to specify process outputs destination
> > 
> > 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.
> > 
> > Testing Done:
> > 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.
> > 
> > Bugs closed: AURORA-1548
> > 
> > Reviewed at https://reviews.apache.org/r/40922/

Never mind the request to close - I was able to do this myself.
Thanks again!


- John


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


On Jan. 8, 2016, 3:54 a.m., Martin Hrabovcin wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/40922/
> ---
> 
> (Updated Jan. 8, 2016, 3:54 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 e2b26d9 
>   docs/configuration-reference.md bea99a7 
>   docs/deploying-aurora-scheduler.md c0988e8 
>   src/main/python/apache/aurora/executor/bin/thermos_executor_main.py cfade22 
>   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
> 
>

Re: Review Request 40922: Thermos: Add ability to specify process outputs destination

[John Sirois]

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


Thanks Martin!  Please mark this review as closed.

Now on master:

git log -1 origin/master
commit 024bac9dcb8f37e4b31210e3a0a7aea2345a16ab
Author: Martin Hrabovcin 
Date:   Fri Jan 8 09:18:11 2016 -0700

Thermos: Add ability to specify process outputs destination

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.

Testing Done:
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.

Bugs closed: AURORA-1548

Reviewed at https://reviews.apache.org/r/40922/

- John Sirois


On Jan. 8, 2016, 3:54 a.m., Martin Hrabovcin wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/40922/
> ---
> 
> (Updated Jan. 8, 2016, 3:54 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 e2b26d9 
>   docs/configuration-reference.md bea99a7 
>   docs/deploying-aurora-scheduler.md c0988e8 
>   src/main/python/apache/aurora/executor/bin/thermos_executor_main.py cfade22 
>   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
> 
>

Re: Review Request 40922: Thermos: Add ability to specify process outputs destination

[John Sirois]

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

Ship it!


Ship It!

- John Sirois


On Jan. 8, 2016, 3:54 a.m., Martin Hrabovcin wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/40922/
> ---
> 
> (Updated Jan. 8, 2016, 3:54 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 e2b26d9 
>   docs/configuration-reference.md bea99a7 
>   docs/deploying-aurora-scheduler.md c0988e8 
>   src/main/python/apache/aurora/executor/bin/thermos_executor_main.py cfade22 
>   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
> 
>

Re: Review Request 40922: Thermos: Add ability to specify process outputs destination

[Aurora ReviewBot]

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

Ship it!


Master (206a48b) is green with this patch.
  ./build-support/jenkins/build.sh

I will refresh this build result if you post a review containing "@ReviewBot 
retry"

- Aurora ReviewBot


On Jan. 8, 2016, 10:54 a.m., Martin Hrabovcin wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/40922/
> ---
> 
> (Updated Jan. 8, 2016, 10:54 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 e2b26d9 
>   docs/configuration-reference.md bea99a7 
>   docs/deploying-aurora-scheduler.md c0988e8 
>   src/main/python/apache/aurora/executor/bin/thermos_executor_main.py cfade22 
>   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
> 
>

Re: Review Request 40922: Thermos: Add ability to specify process outputs destination

[Martin Hrabovcin]

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

(Updated Jan. 8, 2016, 10:54 a.m.)


Review request for Aurora and John Sirois.


Changes
---

Bumping wait value back to 1 second. @ReviewBot retry


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 (updated)
-

  NEWS e2b26d9 
  docs/configuration-reference.md bea99a7 
  docs/deploying-aurora-scheduler.md c0988e8 
  src/main/python/apache/aurora/executor/bin/thermos_executor_main.py cfade22 
  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

Re: Review Request 40922: Thermos: Add ability to specify process outputs destination

[Martin Hrabovcin]

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

(Updated Jan. 8, 2016, 10:43 a.m.)


Review request for Aurora and John Sirois.


Changes
---

This patch addresses John's comments. @ReviewBot retry


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 (updated)
-

  NEWS e2b26d9 
  docs/configuration-reference.md bea99a7 
  docs/deploying-aurora-scheduler.md c0988e8 
  src/main/python/apache/aurora/executor/bin/thermos_executor_main.py cfade22 
  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

Re: Review Request 40922: Thermos: Add ability to specify process outputs destination

[Martin Hrabovcin]

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

Re: Review Request 40922: Thermos: Add ability to specify process outputs destination

[Stephan Erb]

> On Jan. 6, 2016, 10:02 nachm., Stephan Erb wrote:
> > src/main/python/apache/thermos/core/process.py, line 561
> > 
> >
> > Is this timeout change intentional?
> 
> Martin Hrabovcin wrote:
> I've changed this value to .1 because tests were taking significantly 
> longer to execute. From now on every running process logs will be read with 
> that select call. I am happy to revert this to 1 or any other meaningful 
> value.

I believe we should bump that again to prevent busy waiting.


- Stephan


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


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

Re: Review Request 40922: Thermos: Add ability to specify process outputs destination

[Martin Hrabovcin]

> On Jan. 6, 2016, 9:02 p.m., Stephan Erb wrote:
> > src/main/python/apache/thermos/core/process.py, line 561
> > 
> >
> > Is this timeout change intentional?

I've changed this value to .1 because tests were taking significantly longer to 
execute. From now on every running process logs will be read with that select 
call. I am happy to revert this to 1 or any other meaningful value.


> On Jan. 6, 2016, 9:02 p.m., Stephan Erb wrote:
> > src/main/python/apache/thermos/core/runner.py, line 739
> > 
> >
> > Is this path actually needed? Doesn't the command line argument default 
> > to FILE anyway?

Its name of plugin that will process logs that is named `FILE` rather than 
actual filepath. This patch doesn't change how file paths are handled and files 
will be created still in usual places in sandbox.


- Martin


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


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

Re: Review Request 40922: Thermos: Add ability to specify process outputs destination

[Stephan Erb]

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

Ship it!


LGTM overall. However, it is probably needed that one of the core comitters 
takes a closer look as well.


src/main/python/apache/thermos/core/process.py (line 529)


Is this timeout change intentional?



src/main/python/apache/thermos/core/runner.py (line 739)


Is this path actually needed? Doesn't the command line argument default to 
FILE anyway?


- Stephan Erb


On Jan. 4, 2016, 1: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, 1: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
> 
>

Re: Review Request 40922: Thermos: Add ability to specify process outputs destination

[Aurora ReviewBot]

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

Ship it!


Master (8706a78) is green with this patch.
  ./build-support/jenkins/build.sh

I will refresh this build result if you post a review containing "@ReviewBot 
retry"

- Aurora ReviewBot


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

Re: Review Request 40922: Thermos: Add ability to specify process outputs destination

[Martin Hrabovcin]

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


Changes
---

Updated patch addresses @StephanErb comments. @ReviewBot retry


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 (updated)
-

  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

Re: Review Request 40922: Thermos: Add ability to specify process outputs destination

[Martin Hrabovcin]

> On Dec. 25, 2015, 10:10 a.m., Stephan Erb wrote:
> >

I will implemnet all recommended changes.


> On Dec. 25, 2015, 10:10 a.m., Stephan Erb wrote:
> > src/main/python/apache/thermos/core/process.py, line 62
> > 
> >
> > `mixed` does somewhat imply that some bits go there, some other there. 
> > `both` would be more precise. 
> > 
> > However, I can also envision that we might have something like `syslog` 
> > in the future. So, maybe specifying those as a list of applicable options 
> > would be easier to extend? The `none` case would then be an empty list.
> > 
> > What do you think?

I will update this to `both`. Main motivation for this patch was to get 
stdout/stderr to `console` output. I can see the value of different logging 
plugins but I'd leave this tools designed specifically for that purpose. With 
`console` and `file` support you can use `docker` and its logging drivers 
(https://docs.docker.com/engine/reference/logging/overview/) or tools like 
`fluentd` (http://www.fluentd.org/) which can read files, process messages and 
forward them to any output, including syslog.


> On Dec. 25, 2015, 10:10 a.m., Stephan Erb wrote:
> > src/main/python/apache/thermos/core/process.py, line 61
> > 
> >
> > I'd find `console` more obvious than `stream`. What do you think?

I agree! Initially I've googled for common name for stdout/stderr and I've 
found https://en.wikipedia.org/wiki/Standard_streams


- Martin


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


On Dec. 23, 2015, 5:39 p.m., Martin Hrabovcin wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/40922/
> ---
> 
> (Updated Dec. 23, 2015, 5:39 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 ebfc3a6 
>   docs/configuration-reference.md cf63cfa 
>   docs/deploying-aurora-scheduler.md 73b2e04 
>   src/main/python/apache/aurora/executor/bin/thermos_executor_main.py 7b7ef4b 
>   src/main/python/apache/aurora/executor/thermos_task_runner.py 25fcca2 
>   src/main/python/apache/thermos/config/schema_base.py 5552108 
>   src/main/python/apache/thermos/core/process.py 8efdfdc 
>   src/main/python/apache/thermos/core/runner.py 11c06a8 
>   src/main/python/apache/thermos/runner/thermos_runner.py a36bd2a 
>   src/test/python/apache/thermos/core/test_process.py 261371d 
> 
> 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
> 
>

Re: Review Request 40922: Thermos: Add ability to specify process outputs destination

[Stephan Erb]

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



docs/configuration-reference.md (line 158)


This section should also explicitly list the `file` option.



docs/deploying-aurora-scheduler.md (line 177)


This section should link more explicitly to the configuration-reference 
section describing all options.



src/main/python/apache/aurora/executor/bin/thermos_executor_main.py (line 107)


I'd prefer if the default value is specified here . Makes it easier to see 
which values are used when looking at the scheduler command line of an existing 
binary, as it prints the default values when running with `--help`.



src/main/python/apache/thermos/config/schema_base.py (line 72)


`destination` and `mode` could both be tuned independently. I think it make 
sense to specify default values here, so that the user does not has to repeat 
the option he wants to keep at default.



src/main/python/apache/thermos/core/process.py (line 61)


I'd find `console` more obvious than `stream`. What do you think?



src/main/python/apache/thermos/core/process.py (line 62)


`mixed` does somewhat imply that some bits go there, some other there. 
`both` would be more precise. 

However, I can also envision that we might have something like `syslog` in 
the future. So, maybe specifying those as a list of applicable options would be 
easier to extend? The `none` case would then be an empty list.

What do you think?



src/main/python/apache/thermos/core/runner.py (line 731)


Given that you change the default argument, this would no longer be 
necessary.


- Stephan Erb


On Dec. 23, 2015, 6:39 p.m., Martin Hrabovcin wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/40922/
> ---
> 
> (Updated Dec. 23, 2015, 6:39 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 ebfc3a6 
>   docs/configuration-reference.md cf63cfa 
>   docs/deploying-aurora-scheduler.md 73b2e04 
>   src/main/python/apache/aurora/executor/bin/thermos_executor_main.py 7b7ef4b 
>   src/main/python/apache/aurora/executor/thermos_task_runner.py 25fcca2 
>   src/main/python/apache/thermos/config/schema_base.py 5552108 
>   src/main/python/apache/thermos/core/process.py 8efdfdc 
>   src/main/python/apache/thermos/core/runner.py 11c06a8 
>   src/main/python/apache/thermos/runner/thermos_runner.py a36bd2a 
>   src/test/python/apache/thermos/core/test_process.py 261371d 
> 
> 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
> 
>

Re: Review Request 40922: Thermos: Add ability to specify process outputs destination

[Aurora ReviewBot]

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

Ship it!


Master (e75cdba) is green with this patch.
  ./build-support/jenkins/build.sh

I will refresh this build result if you post a review containing "@ReviewBot 
retry"

- Aurora ReviewBot


On Dec. 23, 2015, 9:33 a.m., Martin Hrabovcin wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/40922/
> ---
> 
> (Updated Dec. 23, 2015, 9:33 a.m.)
> 
> 
> Review request for Aurora.
> 
> 
> 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 ebfc3a6 
>   docs/configuration-reference.md cf63cfa 
>   docs/deploying-aurora-scheduler.md 73b2e04 
>   src/main/python/apache/aurora/executor/bin/thermos_executor_main.py 7b7ef4b 
>   src/main/python/apache/aurora/executor/thermos_task_runner.py 25fcca2 
>   src/main/python/apache/thermos/config/schema_base.py 5552108 
>   src/main/python/apache/thermos/core/process.py 8efdfdc 
>   src/main/python/apache/thermos/core/runner.py 11c06a8 
>   src/main/python/apache/thermos/runner/thermos_runner.py a36bd2a 
>   src/test/python/apache/thermos/core/test_process.py 261371d 
> 
> 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
> 
>

Re: Review Request 40922: Thermos: Add ability to specify process outputs destination

[Martin Hrabovcin]

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

(Updated Dec. 23, 2015, 9:33 a.m.)


Review request for Aurora.


Changes
---

Fixed small bug after manual testing and minor docs changes. @ReviewBot retry


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 (updated)
-

  NEWS ebfc3a6 
  docs/configuration-reference.md cf63cfa 
  docs/deploying-aurora-scheduler.md 73b2e04 
  src/main/python/apache/aurora/executor/bin/thermos_executor_main.py 7b7ef4b 
  src/main/python/apache/aurora/executor/thermos_task_runner.py 25fcca2 
  src/main/python/apache/thermos/config/schema_base.py 5552108 
  src/main/python/apache/thermos/core/process.py 8efdfdc 
  src/main/python/apache/thermos/core/runner.py 11c06a8 
  src/main/python/apache/thermos/runner/thermos_runner.py a36bd2a 
  src/test/python/apache/thermos/core/test_process.py 261371d 

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

Re: Review Request 40922: Thermos: Add ability to specify process outputs destination

[Aurora ReviewBot]

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

Ship it!


Master (0813e3a) is green with this patch.
  ./build-support/jenkins/build.sh

I will refresh this build result if you post a review containing "@ReviewBot 
retry"

- Aurora ReviewBot


On Dec. 22, 2015, 4:52 p.m., Martin Hrabovcin wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/40922/
> ---
> 
> (Updated Dec. 22, 2015, 4:52 p.m.)
> 
> 
> Review request for Aurora.
> 
> 
> 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 ebfc3a6 
>   docs/configuration-reference.md cf63cfa 
>   docs/deploying-aurora-scheduler.md 73b2e04 
>   src/main/python/apache/aurora/executor/bin/thermos_executor_main.py 7b7ef4b 
>   src/main/python/apache/aurora/executor/thermos_task_runner.py 25fcca2 
>   src/main/python/apache/thermos/config/schema_base.py 5552108 
>   src/main/python/apache/thermos/core/process.py 8efdfdc 
>   src/main/python/apache/thermos/core/runner.py 11c06a8 
>   src/main/python/apache/thermos/runner/thermos_runner.py a36bd2a 
>   src/test/python/apache/thermos/core/test_process.py 261371d 
> 
> 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
> 
>

Re: Review Request 40922: Thermos: Add ability to specify process outputs destination

[Martin Hrabovcin]

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


This is reworked patch that is more flexible, allows global cluster level 
configuration as well as process level configuration. I was following patterns 
in https://reviews.apache.org/r/30695/ as that patch is related.

@ReviewBot retry

- Martin Hrabovcin


On Dec. 22, 2015, 4:52 p.m., Martin Hrabovcin wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/40922/
> ---
> 
> (Updated Dec. 22, 2015, 4:52 p.m.)
> 
> 
> Review request for Aurora.
> 
> 
> 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 ebfc3a6 
>   docs/configuration-reference.md cf63cfa 
>   docs/deploying-aurora-scheduler.md 73b2e04 
>   src/main/python/apache/aurora/executor/bin/thermos_executor_main.py 7b7ef4b 
>   src/main/python/apache/aurora/executor/thermos_task_runner.py 25fcca2 
>   src/main/python/apache/thermos/config/schema_base.py 5552108 
>   src/main/python/apache/thermos/core/process.py 8efdfdc 
>   src/main/python/apache/thermos/core/runner.py 11c06a8 
>   src/main/python/apache/thermos/runner/thermos_runner.py a36bd2a 
>   src/test/python/apache/thermos/core/test_process.py 261371d 
> 
> 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
> 
>

Re: Review Request 40922: Thermos: Add ability to specify process outputs destination

[Martin Hrabovcin]

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

(Updated Dec. 22, 2015, 4:52 p.m.)


Review request for Aurora.


Changes
---

Updated description to reflect new patch.


Bugs: AURORA-1548
https://issues.apache.org/jira/browse/AURORA-1548


Repository: aurora


Description (updated)
---

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 ebfc3a6 
  docs/configuration-reference.md cf63cfa 
  docs/deploying-aurora-scheduler.md 73b2e04 
  src/main/python/apache/aurora/executor/bin/thermos_executor_main.py 7b7ef4b 
  src/main/python/apache/aurora/executor/thermos_task_runner.py 25fcca2 
  src/main/python/apache/thermos/config/schema_base.py 5552108 
  src/main/python/apache/thermos/core/process.py 8efdfdc 
  src/main/python/apache/thermos/core/runner.py 11c06a8 
  src/main/python/apache/thermos/runner/thermos_runner.py a36bd2a 
  src/test/python/apache/thermos/core/test_process.py 261371d 

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

Re: Review Request 40922: Thermos: Add ability to specify process outputs destination

[Martin Hrabovcin]

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

(Updated Dec. 22, 2015, 4:47 p.m.)


Review request for Aurora.


Summary (updated)
-

Thermos: Add ability to specify process outputs destination


Bugs: AURORA-1548
https://issues.apache.org/jira/browse/AURORA-1548


Repository: aurora


Description
---

This patch will provide way to **optionally** forward process output to stdout 
and stderr along with existing file 
destinations. Its main use is to get logs to docker daemon when using Mesos 
Docker containerizer.

**What was changed:**

New command line option **log_to_std** is added to thermos_executor and 
thermos_runner. If thermos_executor gets this option it will pass it down to 
thermos_runner and runner will pass it down to **Process** class. New class 
**Tee** that mimicks behavior of unix tee utility that allows to split process 
output to file and stdout is core of this patch. subprocess.Popen in Process 
class used be opened with stdout/stderr file parameters. Now it will always get 
openend with subprocess.PIPE for stdout/stderr. Output of these pipes is 
constantly copied to Tee until process exits. If log_to_std is provided Tee 
will split output to expected files and stdout/stderr. Otherwise logs will be 
split to files and /dev/null.


Diffs
-

  NEWS ebfc3a6 
  docs/configuration-reference.md cf63cfa 
  docs/deploying-aurora-scheduler.md 73b2e04 
  src/main/python/apache/aurora/executor/bin/thermos_executor_main.py 7b7ef4b 
  src/main/python/apache/aurora/executor/thermos_task_runner.py 25fcca2 
  src/main/python/apache/thermos/config/schema_base.py 5552108 
  src/main/python/apache/thermos/core/process.py 8efdfdc 
  src/main/python/apache/thermos/core/runner.py 11c06a8 
  src/main/python/apache/thermos/runner/thermos_runner.py a36bd2a 
  src/test/python/apache/thermos/core/test_process.py 261371d 

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