You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@aurora.apache.org by Zameer Manji <zm...@twopensource.com> on 2014/11/27 00:02:07 UTC
Review Request 28486: Move sandbox initialization into try block.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/28486/
-----------------------------------------------------------
Review request for Aurora and Kevin Sweeney.
Repository: aurora
Description
-------
Sandbox initialization is an operation that might fail and signal that failure
by raising SanboxInterface.CreationError. By moving this into the try block the
executor will return TASK_FAILED instead of crashing when this error is raised.
Diffs
-----
src/main/python/apache/aurora/executor/aurora_executor.py 79a24855b2a68271b7478395dfdadab8755c3af2
Diff: https://reviews.apache.org/r/28486/diff/
Testing
-------
./pants src/test/python/apache/aurora/executor::
Thanks,
Zameer Manji
Re: Review Request 28486: Move sandbox initialization into try block.
Posted by Aurora ReviewBot <wf...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/28486/#review63157
-----------------------------------------------------------
Master (ae4d70f) is green with this patch.
./build-support/jenkins/build.sh
However, it appears that it might lack test coverage.
I will refresh this build result if you post a review containing "@ReviewBot retry"
- Aurora ReviewBot
On Nov. 26, 2014, 11:02 p.m., Zameer Manji wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/28486/
> -----------------------------------------------------------
>
> (Updated Nov. 26, 2014, 11:02 p.m.)
>
>
> Review request for Aurora and Kevin Sweeney.
>
>
> Repository: aurora
>
>
> Description
> -------
>
> Sandbox initialization is an operation that might fail and signal that failure
> by raising SanboxInterface.CreationError. By moving this into the try block the
> executor will return TASK_FAILED instead of crashing when this error is raised.
>
>
> Diffs
> -----
>
> src/main/python/apache/aurora/executor/aurora_executor.py 79a24855b2a68271b7478395dfdadab8755c3af2
>
> Diff: https://reviews.apache.org/r/28486/diff/
>
>
> Testing
> -------
>
> ./pants src/test/python/apache/aurora/executor::
>
>
> Thanks,
>
> Zameer Manji
>
>
Re: Review Request 28486: Move sandbox initialization into try block.
Posted by Kevin Sweeney <ke...@apache.org>.
> On Dec. 1, 2014, 2:50 p.m., Zameer Manji wrote:
> > src/main/python/apache/aurora/executor/aurora_executor.py, line 131
> > <https://reviews.apache.org/r/28486/diff/3/?file=779344#file779344line131>
> >
> > This makes the code inconsistent with line 141:
> > ````
> > except self._sandbox.Error as e:
> > ````
> >
> > I would prefer to remain consistent with the existing code.
You're accessing a static variable in a non-static context here, effectively introducing a new customization point by allowing an implementation to override the error type. This will burn you if you want to catch this at a higher level and aren't sure which status checker declared the exception type.
- Kevin
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/28486/#review63453
-----------------------------------------------------------
On Dec. 1, 2014, 2:24 p.m., Zameer Manji wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/28486/
> -----------------------------------------------------------
>
> (Updated Dec. 1, 2014, 2:24 p.m.)
>
>
> Review request for Aurora and Kevin Sweeney.
>
>
> Repository: aurora
>
>
> Description
> -------
>
> Sandbox initialization is an operation that might fail and raise an exception.
> By moving this into the try block the executor will return TASK_FAILED instead of crashing when an exception is raised.
>
>
> Diffs
> -----
>
> src/main/python/apache/aurora/executor/aurora_executor.py 79a24855b2a68271b7478395dfdadab8755c3af2
> src/main/python/apache/aurora/executor/common/sandbox.py f47a32b3fefb4a89940b1ddc473b8316ac00df12
> src/test/python/apache/aurora/executor/BUILD 4d43e256ad131223cc1ac36a406d42a979a8a2dd
> src/test/python/apache/aurora/executor/test_aurora_executor.py PRE-CREATION
>
> Diff: https://reviews.apache.org/r/28486/diff/
>
>
> Testing
> -------
>
> ./pants src/test/python/apache/aurora/executor::
>
>
> Thanks,
>
> Zameer Manji
>
>
Re: Review Request 28486: Move sandbox initialization into try block.
Posted by Zameer Manji <zm...@twopensource.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/28486/#review63453
-----------------------------------------------------------
src/main/python/apache/aurora/executor/aurora_executor.py
<https://reviews.apache.org/r/28486/#comment105708>
This makes the code inconsistent with line 141:
````
except self._sandbox.Error as e:
````
I would prefer to remain consistent with the existing code.
- Zameer Manji
On Dec. 1, 2014, 2:24 p.m., Zameer Manji wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/28486/
> -----------------------------------------------------------
>
> (Updated Dec. 1, 2014, 2:24 p.m.)
>
>
> Review request for Aurora and Kevin Sweeney.
>
>
> Repository: aurora
>
>
> Description
> -------
>
> Sandbox initialization is an operation that might fail and raise an exception.
> By moving this into the try block the executor will return TASK_FAILED instead of crashing when an exception is raised.
>
>
> Diffs
> -----
>
> src/main/python/apache/aurora/executor/aurora_executor.py 79a24855b2a68271b7478395dfdadab8755c3af2
> src/main/python/apache/aurora/executor/common/sandbox.py f47a32b3fefb4a89940b1ddc473b8316ac00df12
> src/test/python/apache/aurora/executor/BUILD 4d43e256ad131223cc1ac36a406d42a979a8a2dd
> src/test/python/apache/aurora/executor/test_aurora_executor.py PRE-CREATION
>
> Diff: https://reviews.apache.org/r/28486/diff/
>
>
> Testing
> -------
>
> ./pants src/test/python/apache/aurora/executor::
>
>
> Thanks,
>
> Zameer Manji
>
>
Re: Review Request 28486: Move sandbox initialization into try block.
Posted by Brian Wickman <wi...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/28486/#review63587
-----------------------------------------------------------
src/main/python/apache/aurora/executor/aurora_executor.py
<https://reviews.apache.org/r/28486/#comment105861>
task sanitation shouldn't be the responsibility of the sandbox manager. instead on L99 we should add something like:
try:
mesos_task_instance_from_assigned_task(assigned_task)
except ValueError as e:
self._die(driver, mesos_pb2.TASK_FAILED, 'Invalid task: %s' % e)
return
- Brian Wickman
On Dec. 2, 2014, 2:14 a.m., Zameer Manji wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/28486/
> -----------------------------------------------------------
>
> (Updated Dec. 2, 2014, 2:14 a.m.)
>
>
> Review request for Aurora, Kevin Sweeney and Brian Wickman.
>
>
> Repository: aurora
>
>
> Description
> -------
>
> Sandbox initialization is an operation that might fail and raise an exception.
> By moving this into the try block the executor will return TASK_FAILED instead of crashing when an exception is raised.
>
>
> Diffs
> -----
>
> src/main/python/apache/aurora/executor/aurora_executor.py 79a24855b2a68271b7478395dfdadab8755c3af2
> src/main/python/apache/aurora/executor/common/sandbox.py f47a32b3fefb4a89940b1ddc473b8316ac00df12
> src/test/python/apache/aurora/executor/BUILD 4d43e256ad131223cc1ac36a406d42a979a8a2dd
> src/test/python/apache/aurora/executor/test_aurora_executor.py PRE-CREATION
>
> Diff: https://reviews.apache.org/r/28486/diff/
>
>
> Testing
> -------
>
> ./pants src/test/python/apache/aurora/executor::
>
>
> Thanks,
>
> Zameer Manji
>
>
Re: Review Request 28486: Move sandbox initialization into try block.
Posted by Zameer Manji <zm...@twopensource.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/28486/
-----------------------------------------------------------
(Updated Dec. 1, 2014, 6:14 p.m.)
Review request for Aurora, Kevin Sweeney and Brian Wickman.
Changes
-------
+wickman
Repository: aurora
Description
-------
Sandbox initialization is an operation that might fail and raise an exception.
By moving this into the try block the executor will return TASK_FAILED instead of crashing when an exception is raised.
Diffs
-----
src/main/python/apache/aurora/executor/aurora_executor.py 79a24855b2a68271b7478395dfdadab8755c3af2
src/main/python/apache/aurora/executor/common/sandbox.py f47a32b3fefb4a89940b1ddc473b8316ac00df12
src/test/python/apache/aurora/executor/BUILD 4d43e256ad131223cc1ac36a406d42a979a8a2dd
src/test/python/apache/aurora/executor/test_aurora_executor.py PRE-CREATION
Diff: https://reviews.apache.org/r/28486/diff/
Testing
-------
./pants src/test/python/apache/aurora/executor::
Thanks,
Zameer Manji
Re: Review Request 28486: Move sandbox initialization into try block.
Posted by Aurora ReviewBot <wf...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/28486/#review63458
-----------------------------------------------------------
Ship it!
Master (88baad6) 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. 1, 2014, 11:18 p.m., Zameer Manji wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/28486/
> -----------------------------------------------------------
>
> (Updated Dec. 1, 2014, 11:18 p.m.)
>
>
> Review request for Aurora and Kevin Sweeney.
>
>
> Repository: aurora
>
>
> Description
> -------
>
> Sandbox initialization is an operation that might fail and raise an exception.
> By moving this into the try block the executor will return TASK_FAILED instead of crashing when an exception is raised.
>
>
> Diffs
> -----
>
> src/main/python/apache/aurora/executor/aurora_executor.py 79a24855b2a68271b7478395dfdadab8755c3af2
> src/main/python/apache/aurora/executor/common/sandbox.py f47a32b3fefb4a89940b1ddc473b8316ac00df12
> src/test/python/apache/aurora/executor/BUILD 4d43e256ad131223cc1ac36a406d42a979a8a2dd
> src/test/python/apache/aurora/executor/test_aurora_executor.py PRE-CREATION
>
> Diff: https://reviews.apache.org/r/28486/diff/
>
>
> Testing
> -------
>
> ./pants src/test/python/apache/aurora/executor::
>
>
> Thanks,
>
> Zameer Manji
>
>
Re: Review Request 28486: Move sandbox initialization into try block.
Posted by Kevin Sweeney <ke...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/28486/#review63484
-----------------------------------------------------------
Ship it!
Looks good to me, please have wickman take a look as well.
- Kevin Sweeney
On Dec. 1, 2014, 3:18 p.m., Zameer Manji wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/28486/
> -----------------------------------------------------------
>
> (Updated Dec. 1, 2014, 3:18 p.m.)
>
>
> Review request for Aurora and Kevin Sweeney.
>
>
> Repository: aurora
>
>
> Description
> -------
>
> Sandbox initialization is an operation that might fail and raise an exception.
> By moving this into the try block the executor will return TASK_FAILED instead of crashing when an exception is raised.
>
>
> Diffs
> -----
>
> src/main/python/apache/aurora/executor/aurora_executor.py 79a24855b2a68271b7478395dfdadab8755c3af2
> src/main/python/apache/aurora/executor/common/sandbox.py f47a32b3fefb4a89940b1ddc473b8316ac00df12
> src/test/python/apache/aurora/executor/BUILD 4d43e256ad131223cc1ac36a406d42a979a8a2dd
> src/test/python/apache/aurora/executor/test_aurora_executor.py PRE-CREATION
>
> Diff: https://reviews.apache.org/r/28486/diff/
>
>
> Testing
> -------
>
> ./pants src/test/python/apache/aurora/executor::
>
>
> Thanks,
>
> Zameer Manji
>
>
Re: Review Request 28486: Move sandbox initialization into try block.
Posted by Zameer Manji <zm...@twopensource.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/28486/
-----------------------------------------------------------
(Updated Dec. 1, 2014, 3:18 p.m.)
Review request for Aurora and Kevin Sweeney.
Repository: aurora
Description
-------
Sandbox initialization is an operation that might fail and raise an exception.
By moving this into the try block the executor will return TASK_FAILED instead of crashing when an exception is raised.
Diffs (updated)
-----
src/main/python/apache/aurora/executor/aurora_executor.py 79a24855b2a68271b7478395dfdadab8755c3af2
src/main/python/apache/aurora/executor/common/sandbox.py f47a32b3fefb4a89940b1ddc473b8316ac00df12
src/test/python/apache/aurora/executor/BUILD 4d43e256ad131223cc1ac36a406d42a979a8a2dd
src/test/python/apache/aurora/executor/test_aurora_executor.py PRE-CREATION
Diff: https://reviews.apache.org/r/28486/diff/
Testing
-------
./pants src/test/python/apache/aurora/executor::
Thanks,
Zameer Manji
Re: Review Request 28486: Move sandbox initialization into try block.
Posted by Zameer Manji <zm...@twopensource.com>.
> On Dec. 1, 2014, 2:34 p.m., Joshua Cohen wrote:
> > src/main/python/apache/aurora/executor/aurora_executor.py, line 131
> > <https://reviews.apache.org/r/28486/diff/2-3/?file=777033#file777033line131>
> >
> > Outside of tests, what's actually raising this?
This exception allows for other implementations of sandbox providers to raise an exception to indicate failure when creating the sandbox object. The default implementation that is provided can't fail but other implementations can.
- Zameer
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/28486/#review63447
-----------------------------------------------------------
On Dec. 1, 2014, 2:24 p.m., Zameer Manji wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/28486/
> -----------------------------------------------------------
>
> (Updated Dec. 1, 2014, 2:24 p.m.)
>
>
> Review request for Aurora and Kevin Sweeney.
>
>
> Repository: aurora
>
>
> Description
> -------
>
> Sandbox initialization is an operation that might fail and raise an exception.
> By moving this into the try block the executor will return TASK_FAILED instead of crashing when an exception is raised.
>
>
> Diffs
> -----
>
> src/main/python/apache/aurora/executor/aurora_executor.py 79a24855b2a68271b7478395dfdadab8755c3af2
> src/main/python/apache/aurora/executor/common/sandbox.py f47a32b3fefb4a89940b1ddc473b8316ac00df12
> src/test/python/apache/aurora/executor/BUILD 4d43e256ad131223cc1ac36a406d42a979a8a2dd
> src/test/python/apache/aurora/executor/test_aurora_executor.py PRE-CREATION
>
> Diff: https://reviews.apache.org/r/28486/diff/
>
>
> Testing
> -------
>
> ./pants src/test/python/apache/aurora/executor::
>
>
> Thanks,
>
> Zameer Manji
>
>
Re: Review Request 28486: Move sandbox initialization into try block.
Posted by Joshua Cohen <jc...@twopensource.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/28486/#review63447
-----------------------------------------------------------
src/main/python/apache/aurora/executor/aurora_executor.py
<https://reviews.apache.org/r/28486/#comment105704>
Outside of tests, what's actually raising this?
- Joshua Cohen
On Dec. 1, 2014, 10:24 p.m., Zameer Manji wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/28486/
> -----------------------------------------------------------
>
> (Updated Dec. 1, 2014, 10:24 p.m.)
>
>
> Review request for Aurora and Kevin Sweeney.
>
>
> Repository: aurora
>
>
> Description
> -------
>
> Sandbox initialization is an operation that might fail and raise an exception.
> By moving this into the try block the executor will return TASK_FAILED instead of crashing when an exception is raised.
>
>
> Diffs
> -----
>
> src/main/python/apache/aurora/executor/aurora_executor.py 79a24855b2a68271b7478395dfdadab8755c3af2
> src/main/python/apache/aurora/executor/common/sandbox.py f47a32b3fefb4a89940b1ddc473b8316ac00df12
> src/test/python/apache/aurora/executor/BUILD 4d43e256ad131223cc1ac36a406d42a979a8a2dd
> src/test/python/apache/aurora/executor/test_aurora_executor.py PRE-CREATION
>
> Diff: https://reviews.apache.org/r/28486/diff/
>
>
> Testing
> -------
>
> ./pants src/test/python/apache/aurora/executor::
>
>
> Thanks,
>
> Zameer Manji
>
>
Re: Review Request 28486: Move sandbox initialization into try block.
Posted by Joshua Cohen <jc...@twopensource.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/28486/#review63450
-----------------------------------------------------------
Ship it!
Ship It!
- Joshua Cohen
On Dec. 1, 2014, 10:24 p.m., Zameer Manji wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/28486/
> -----------------------------------------------------------
>
> (Updated Dec. 1, 2014, 10:24 p.m.)
>
>
> Review request for Aurora and Kevin Sweeney.
>
>
> Repository: aurora
>
>
> Description
> -------
>
> Sandbox initialization is an operation that might fail and raise an exception.
> By moving this into the try block the executor will return TASK_FAILED instead of crashing when an exception is raised.
>
>
> Diffs
> -----
>
> src/main/python/apache/aurora/executor/aurora_executor.py 79a24855b2a68271b7478395dfdadab8755c3af2
> src/main/python/apache/aurora/executor/common/sandbox.py f47a32b3fefb4a89940b1ddc473b8316ac00df12
> src/test/python/apache/aurora/executor/BUILD 4d43e256ad131223cc1ac36a406d42a979a8a2dd
> src/test/python/apache/aurora/executor/test_aurora_executor.py PRE-CREATION
>
> Diff: https://reviews.apache.org/r/28486/diff/
>
>
> Testing
> -------
>
> ./pants src/test/python/apache/aurora/executor::
>
>
> Thanks,
>
> Zameer Manji
>
>
Re: Review Request 28486: Move sandbox initialization into try block.
Posted by Aurora ReviewBot <wf...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/28486/#review63448
-----------------------------------------------------------
Ship it!
Master (88baad6) 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. 1, 2014, 10:24 p.m., Zameer Manji wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/28486/
> -----------------------------------------------------------
>
> (Updated Dec. 1, 2014, 10:24 p.m.)
>
>
> Review request for Aurora and Kevin Sweeney.
>
>
> Repository: aurora
>
>
> Description
> -------
>
> Sandbox initialization is an operation that might fail and raise an exception.
> By moving this into the try block the executor will return TASK_FAILED instead of crashing when an exception is raised.
>
>
> Diffs
> -----
>
> src/main/python/apache/aurora/executor/aurora_executor.py 79a24855b2a68271b7478395dfdadab8755c3af2
> src/main/python/apache/aurora/executor/common/sandbox.py f47a32b3fefb4a89940b1ddc473b8316ac00df12
> src/test/python/apache/aurora/executor/BUILD 4d43e256ad131223cc1ac36a406d42a979a8a2dd
> src/test/python/apache/aurora/executor/test_aurora_executor.py PRE-CREATION
>
> Diff: https://reviews.apache.org/r/28486/diff/
>
>
> Testing
> -------
>
> ./pants src/test/python/apache/aurora/executor::
>
>
> Thanks,
>
> Zameer Manji
>
>
Re: Review Request 28486: Move sandbox initialization into try block.
Posted by Kevin Sweeney <ke...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/28486/#review63452
-----------------------------------------------------------
src/main/python/apache/aurora/executor/aurora_executor.py
<https://reviews.apache.org/r/28486/#comment105707>
rather than `self._sandbox_provider.Error` here use the concrete error class, i.e. `SandboxProvider.Error`.
- Kevin Sweeney
On Dec. 1, 2014, 2:24 p.m., Zameer Manji wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/28486/
> -----------------------------------------------------------
>
> (Updated Dec. 1, 2014, 2:24 p.m.)
>
>
> Review request for Aurora and Kevin Sweeney.
>
>
> Repository: aurora
>
>
> Description
> -------
>
> Sandbox initialization is an operation that might fail and raise an exception.
> By moving this into the try block the executor will return TASK_FAILED instead of crashing when an exception is raised.
>
>
> Diffs
> -----
>
> src/main/python/apache/aurora/executor/aurora_executor.py 79a24855b2a68271b7478395dfdadab8755c3af2
> src/main/python/apache/aurora/executor/common/sandbox.py f47a32b3fefb4a89940b1ddc473b8316ac00df12
> src/test/python/apache/aurora/executor/BUILD 4d43e256ad131223cc1ac36a406d42a979a8a2dd
> src/test/python/apache/aurora/executor/test_aurora_executor.py PRE-CREATION
>
> Diff: https://reviews.apache.org/r/28486/diff/
>
>
> Testing
> -------
>
> ./pants src/test/python/apache/aurora/executor::
>
>
> Thanks,
>
> Zameer Manji
>
>
Re: Review Request 28486: Move sandbox initialization into try block.
Posted by Zameer Manji <zm...@twopensource.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/28486/
-----------------------------------------------------------
(Updated Dec. 1, 2014, 2:24 p.m.)
Review request for Aurora and Kevin Sweeney.
Repository: aurora
Description
-------
Sandbox initialization is an operation that might fail and raise an exception.
By moving this into the try block the executor will return TASK_FAILED instead of crashing when an exception is raised.
Diffs (updated)
-----
src/main/python/apache/aurora/executor/aurora_executor.py 79a24855b2a68271b7478395dfdadab8755c3af2
src/main/python/apache/aurora/executor/common/sandbox.py f47a32b3fefb4a89940b1ddc473b8316ac00df12
src/test/python/apache/aurora/executor/BUILD 4d43e256ad131223cc1ac36a406d42a979a8a2dd
src/test/python/apache/aurora/executor/test_aurora_executor.py PRE-CREATION
Diff: https://reviews.apache.org/r/28486/diff/
Testing
-------
./pants src/test/python/apache/aurora/executor::
Thanks,
Zameer Manji
Re: Review Request 28486: Move sandbox initialization into try block.
Posted by Aurora ReviewBot <wf...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/28486/#review63160
-----------------------------------------------------------
Master (ae4d70f) is green with this patch.
./build-support/jenkins/build.sh
However, it appears that it might lack test coverage.
I will refresh this build result if you post a review containing "@ReviewBot retry"
- Aurora ReviewBot
On Nov. 26, 2014, 11:48 p.m., Zameer Manji wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/28486/
> -----------------------------------------------------------
>
> (Updated Nov. 26, 2014, 11:48 p.m.)
>
>
> Review request for Aurora and Kevin Sweeney.
>
>
> Repository: aurora
>
>
> Description
> -------
>
> Sandbox initialization is an operation that might fail and raise an exception.
> By moving this into the try block the executor will return TASK_FAILED instead of crashing when an exception is raised.
>
>
> Diffs
> -----
>
> src/main/python/apache/aurora/executor/aurora_executor.py 79a24855b2a68271b7478395dfdadab8755c3af2
>
> Diff: https://reviews.apache.org/r/28486/diff/
>
>
> Testing
> -------
>
> ./pants src/test/python/apache/aurora/executor::
>
>
> Thanks,
>
> Zameer Manji
>
>
Re: Review Request 28486: Move sandbox initialization into try block.
Posted by Joshua Cohen <jc...@twopensource.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/28486/#review63417
-----------------------------------------------------------
Can you add test coverage?
- Joshua Cohen
On Nov. 26, 2014, 11:48 p.m., Zameer Manji wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/28486/
> -----------------------------------------------------------
>
> (Updated Nov. 26, 2014, 11:48 p.m.)
>
>
> Review request for Aurora and Kevin Sweeney.
>
>
> Repository: aurora
>
>
> Description
> -------
>
> Sandbox initialization is an operation that might fail and raise an exception.
> By moving this into the try block the executor will return TASK_FAILED instead of crashing when an exception is raised.
>
>
> Diffs
> -----
>
> src/main/python/apache/aurora/executor/aurora_executor.py 79a24855b2a68271b7478395dfdadab8755c3af2
>
> Diff: https://reviews.apache.org/r/28486/diff/
>
>
> Testing
> -------
>
> ./pants src/test/python/apache/aurora/executor::
>
>
> Thanks,
>
> Zameer Manji
>
>
Re: Review Request 28486: Move sandbox initialization into try block.
Posted by Kevin Sweeney <ke...@apache.org>.
> On Dec. 1, 2014, 11:40 a.m., Kevin Sweeney wrote:
> > src/main/python/apache/aurora/executor/aurora_executor.py, line 132
> > <https://reviews.apache.org/r/28486/diff/2/?file=777033#file777033line132>
> >
> > Should an uncaught Exception be TASK_LOST? presumably specific exceptions from user input can be TASK_FAILED.
> >
> > Is there a shallower exception class that you're certain comes from invalid user input?
>
> Zameer Manji wrote:
> The sandbox provider could be doing *anything* so any exception there I think is failure to start the task. I agree that having this wide type is bad but I am unsure on how to deal with this better.
Improve the contract of StatusChecker so that user-generated exceptions throw a specific type. IMO there should be a single except Exception block at the top of the application that causes TASK_LOST - TASK_FAILED should come with a useful cause.
- Kevin
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/28486/#review63421
-----------------------------------------------------------
On Nov. 26, 2014, 3:48 p.m., Zameer Manji wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/28486/
> -----------------------------------------------------------
>
> (Updated Nov. 26, 2014, 3:48 p.m.)
>
>
> Review request for Aurora and Kevin Sweeney.
>
>
> Repository: aurora
>
>
> Description
> -------
>
> Sandbox initialization is an operation that might fail and raise an exception.
> By moving this into the try block the executor will return TASK_FAILED instead of crashing when an exception is raised.
>
>
> Diffs
> -----
>
> src/main/python/apache/aurora/executor/aurora_executor.py 79a24855b2a68271b7478395dfdadab8755c3af2
>
> Diff: https://reviews.apache.org/r/28486/diff/
>
>
> Testing
> -------
>
> ./pants src/test/python/apache/aurora/executor::
>
>
> Thanks,
>
> Zameer Manji
>
>
Re: Review Request 28486: Move sandbox initialization into try block.
Posted by Zameer Manji <zm...@twopensource.com>.
> On Dec. 1, 2014, 11:40 a.m., Kevin Sweeney wrote:
> > src/main/python/apache/aurora/executor/aurora_executor.py, line 132
> > <https://reviews.apache.org/r/28486/diff/2/?file=777033#file777033line132>
> >
> > Should an uncaught Exception be TASK_LOST? presumably specific exceptions from user input can be TASK_FAILED.
> >
> > Is there a shallower exception class that you're certain comes from invalid user input?
The sandbox provider could be doing *anything* so any exception there I think is failure to start the task. I agree that having this wide type is bad but I am unsure on how to deal with this better.
- Zameer
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/28486/#review63421
-----------------------------------------------------------
On Nov. 26, 2014, 3:48 p.m., Zameer Manji wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/28486/
> -----------------------------------------------------------
>
> (Updated Nov. 26, 2014, 3:48 p.m.)
>
>
> Review request for Aurora and Kevin Sweeney.
>
>
> Repository: aurora
>
>
> Description
> -------
>
> Sandbox initialization is an operation that might fail and raise an exception.
> By moving this into the try block the executor will return TASK_FAILED instead of crashing when an exception is raised.
>
>
> Diffs
> -----
>
> src/main/python/apache/aurora/executor/aurora_executor.py 79a24855b2a68271b7478395dfdadab8755c3af2
>
> Diff: https://reviews.apache.org/r/28486/diff/
>
>
> Testing
> -------
>
> ./pants src/test/python/apache/aurora/executor::
>
>
> Thanks,
>
> Zameer Manji
>
>
Re: Review Request 28486: Move sandbox initialization into try block.
Posted by Kevin Sweeney <ke...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/28486/#review63421
-----------------------------------------------------------
src/main/python/apache/aurora/executor/aurora_executor.py
<https://reviews.apache.org/r/28486/#comment105670>
Should an uncaught Exception be TASK_LOST? presumably specific exceptions from user input can be TASK_FAILED.
Is there a shallower exception class that you're certain comes from invalid user input?
- Kevin Sweeney
On Nov. 26, 2014, 3:48 p.m., Zameer Manji wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/28486/
> -----------------------------------------------------------
>
> (Updated Nov. 26, 2014, 3:48 p.m.)
>
>
> Review request for Aurora and Kevin Sweeney.
>
>
> Repository: aurora
>
>
> Description
> -------
>
> Sandbox initialization is an operation that might fail and raise an exception.
> By moving this into the try block the executor will return TASK_FAILED instead of crashing when an exception is raised.
>
>
> Diffs
> -----
>
> src/main/python/apache/aurora/executor/aurora_executor.py 79a24855b2a68271b7478395dfdadab8755c3af2
>
> Diff: https://reviews.apache.org/r/28486/diff/
>
>
> Testing
> -------
>
> ./pants src/test/python/apache/aurora/executor::
>
>
> Thanks,
>
> Zameer Manji
>
>
Re: Review Request 28486: Move sandbox initialization into try block.
Posted by Zameer Manji <zm...@twopensource.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/28486/
-----------------------------------------------------------
(Updated Nov. 26, 2014, 3:48 p.m.)
Review request for Aurora and Kevin Sweeney.
Repository: aurora
Description (updated)
-------
Sandbox initialization is an operation that might fail and raise an exception.
By moving this into the try block the executor will return TASK_FAILED instead of crashing when an exception is raised.
Diffs (updated)
-----
src/main/python/apache/aurora/executor/aurora_executor.py 79a24855b2a68271b7478395dfdadab8755c3af2
Diff: https://reviews.apache.org/r/28486/diff/
Testing
-------
./pants src/test/python/apache/aurora/executor::
Thanks,
Zameer Manji