You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@aurora.apache.org by Martin Hrabovcin <mh...@gmail.com> on 2016/01/04 13:25:07 UTC
Re: Review Request 40922: Thermos: Add ability to specify process
outputs destination
-----------------------------------------------------------
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
Posted by John Sirois <js...@apache.org>.
-----------------------------------------------------------
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
Posted by Aurora ReviewBot <wf...@apache.org>.
-----------------------------------------------------------
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
Posted by John Sirois <js...@apache.org>.
> 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 <mh...@gmail.com>
> > 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
Posted by John Sirois <js...@apache.org>.
-----------------------------------------------------------
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 <mh...@gmail.com>
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
Posted by Martin Hrabovcin <mh...@gmail.com>.
-----------------------------------------------------------
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
Posted by Martin Hrabovcin <mh...@gmail.com>.
-----------------------------------------------------------
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
Posted by Aurora ReviewBot <wf...@apache.org>.
-----------------------------------------------------------
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
Posted by Martin Hrabovcin <mh...@gmail.com>.
> 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
>
>
Re: Review Request 40922: Thermos: Add ability to specify process
outputs destination
Posted by John Sirois <jo...@gmail.com>.
-----------------------------------------------------------
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
>
>
Re: Review Request 40922: Thermos: Add ability to specify process
outputs destination
Posted by Stephan Erb <st...@dev.static-void.de>.
> On Jan. 6, 2016, 10:02 nachm., Stephan Erb wrote:
> > src/main/python/apache/thermos/core/process.py, line 561
> > <https://reviews.apache.org/r/40922/diff/8/?file=1180836#file1180836line561>
> >
> > 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
Posted by Martin Hrabovcin <mh...@gmail.com>.
> On Jan. 6, 2016, 9:02 p.m., Stephan Erb wrote:
> > src/main/python/apache/thermos/core/process.py, line 561
> > <https://reviews.apache.org/r/40922/diff/8/?file=1180836#file1180836line561>
> >
> > 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
> > <https://reviews.apache.org/r/40922/diff/8/?file=1180837#file1180837line739>
> >
> > 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
Posted by John Sirois <jo...@gmail.com>.
> On Jan. 6, 2016, 2:02 p.m., Stephan Erb wrote:
> > LGTM overall. However, it is probably needed that one of the core comitters takes a closer look as well.
Martin - for the comments marked as issues below, you should at some point click fixed or dropped when you have either fixed the issue or explained why its not one (dropped). It looks like your comments indicate you should drop the 2 below.
- John
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/40922/#review113134
-----------------------------------------------------------
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
>
>
Re: Review Request 40922: Thermos: Add ability to specify process
outputs destination
Posted by Stephan Erb <st...@dev.static-void.de>.
-----------------------------------------------------------
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)
<https://reviews.apache.org/r/40922/#comment173629>
Is this timeout change intentional?
src/main/python/apache/thermos/core/runner.py (line 739)
<https://reviews.apache.org/r/40922/#comment173630>
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
>
>