Review Request 34679: Correcting typo in developing-aurora-client.md for readability.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34679/ --- Review request for Aurora. Repository: aurora Description --- Correcting typo in developing-aurora-client.md for readability. Diffs - docs/developing-aurora-client.md 4ddc77f1443e260986db04f2b7ec8dc17e6a7ac1 Diff: https://reviews.apache.org/r/34679/diff/ Testing --- Thanks, Will Swank
Re: Review Request 34484: Daemonize all deadline calls in aurora executor.
On May 21, 2015, 5:54 a.m., Stephan Erb wrote: src/main/python/apache/aurora/executor/aurora_executor.py, line 189 https://reviews.apache.org/r/34484/diff/1/?file=965595#file965595line189 The two `deadline` calls in shutdown are run without `propagate=True`. IIRC we will therefore silently swallow all raised exceptions. Shouldn't we at least log them? Good call, added logging. - Brian --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34484/#review84661 --- On May 26, 2015, 8:43 p.m., Brian Wickman wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34484/ --- (Updated May 26, 2015, 8:43 p.m.) Review request for Aurora, Joe Smith and Vinod Kone. Bugs: AURORA-698 https://issues.apache.org/jira/browse/AURORA-698 Repository: aurora Description --- Daemonize all deadline calls in aurora executor. If we do not daemonize, it's possible for the aurora executor to send TASK_KILLED and then block indefinitely on shutdown. This way the aurora executor process will at least exit, allow the cgroup to tear down all active processes. Diffs - src/main/python/apache/aurora/executor/aurora_executor.py df0df0cba3269a137a370e2b4096fd61fd3af258 Diff: https://reviews.apache.org/r/34484/diff/ Testing --- ./pants test src/test/python/apache/aurora/executor:: Thanks, Brian Wickman
Re: Review Request 34300: Do better sanitation on the client side when encountering unbound pystachio refs
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/34300/#review85258
---
Master (6db13ba) is red with this patch.
./build-support/jenkins/build.sh
Using cached twitter.common.collections-0.3.0.tar.gz
Collecting smmap=0.8.5 (from
gitdb=0.5.1-GitPython==0.3.2.RC1-twitter.checkstyle==0.1.0)
Using cached smmap-0.9.0.tar.gz
Collecting twitter.common.string==0.3.0 (from
twitter.common.process==0.3.0-twitter.common.app==0.3.0-twitter.checkstyle==0.1.0)
Using cached twitter.common.string-0.3.0.tar.gz
Collecting twitter.common.options==0.3.0 (from
twitter.common.log==0.3.0-twitter.common.app==0.3.0-twitter.checkstyle==0.1.0)
Using cached twitter.common.options-0.3.0.tar.gz
Collecting twitter.common.dirutil==0.3.0 (from
twitter.common.log==0.3.0-twitter.common.app==0.3.0-twitter.checkstyle==0.1.0)
Using cached twitter.common.dirutil-0.3.0.tar.gz
Collecting twitter.common.contextutil==0.3.0 (from
twitter.common.util==0.3.0-twitter.common.app==0.3.0-twitter.checkstyle==0.1.0)
Using cached twitter.common.contextutil-0.3.0.tar.gz
Collecting twitter.common.lang==0.3.0 (from
twitter.common.collections==0.3.0-twitter.common.app==0.3.0-twitter.checkstyle==0.1.0)
Using cached twitter.common.lang-0.3.0.tar.gz
Installing collected packages: pyflakes, pep8, smmap, gitdb, GitPython,
twitter.common.lang, twitter.common.string, twitter.common.process,
twitter.common.options, twitter.common.dirutil, twitter.common.log,
twitter.common.contextutil, twitter.common.util, twitter.common.collections,
twitter.common.app, twitter.checkstyle
Running setup.py install for pyflakes
Running setup.py install for pep8
Running setup.py install for smmap
Running setup.py install for gitdb
Running setup.py install for GitPython
Running setup.py install for twitter.common.lang
Running setup.py install for twitter.common.string
Running setup.py install for twitter.common.process
Running setup.py install for twitter.common.options
Running setup.py install for twitter.common.dirutil
Running setup.py install for twitter.common.log
Running setup.py install for twitter.common.contextutil
Running setup.py install for twitter.common.util
Running setup.py install for twitter.common.collections
Running setup.py install for twitter.common.app
Running setup.py install for twitter.checkstyle
Successfully installed GitPython-0.3.2rc1 gitdb-0.6.4 pep8-1.4.5 pyflakes-0.7.2
smmap-0.9.0 twitter.checkstyle-0.1.0 twitter.common.app-0.3.0
twitter.common.collections-0.3.0 twitter.common.contextutil-0.3.0
twitter.common.dirutil-0.3.0 twitter.common.lang-0.3.0 twitter.common.log-0.3.0
twitter.common.options-0.3.0 twitter.common.process-0.3.0
twitter.common.string-0.3.0 twitter.common.util-0.3.0
F401:ERROR src/test/python/apache/aurora/client/test_config.py:019
'temporary_file' imported but unused
|from twitter.common.contextutil import temporary_dir, temporary_file
F401:ERROR src/test/python/apache/aurora/config/test_thrift.py:019 'Map'
imported but unused
|from pystachio import Map, String
F401:ERROR src/test/python/apache/aurora/config/test_thrift.py:019 'String'
imported but unused
|from pystachio import Map, String
I will refresh this build result if you post a review containing @ReviewBot
retry
- Aurora ReviewBot
On May 26, 2015, 9:03 p.m., Brian Wickman wrote:
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/34300/
---
(Updated May 26, 2015, 9:03 p.m.)
Review request for Aurora and Kevin Sweeney.
Bugs: AURORA-739
https://issues.apache.org/jira/browse/AURORA-739
Repository: aurora
Description
---
It's possible to define nested refs that can cause the executor to stack
trace, e.g.
{{derp[{{thermos.ports[http]}}]}} is perfectly valid but crashes the executor.
Diffs
-
src/main/python/apache/aurora/config/__init__.py
dd2f89014a3da730364b14e01c499ac0f2c288c1
src/main/python/apache/aurora/config/schema/base.py
a87524a8b3ad5aa0e337e0a0028cecb85865b4e6
src/main/python/apache/aurora/config/thrift.py
810febb637d168b07c4aea77984e1d1451a39af2
src/main/python/apache/aurora/executor/common/task_info.py
d110faf08135d94d9af95ad74613950c56248c09
src/main/python/apache/thermos/config/dsl.py
0663a9ad8ae194b63ce6d77dabfe65564e2d26dd
src/main/python/apache/thermos/config/loader.py
d77ab9a52b16e9d65acdb95f01fd251ae8ab2b6e
src/test/python/apache/aurora/client/test_config.py
c56779712b91f621261358aa7ebd6c4fc65446a0
src/test/python/apache/aurora/config/test_thrift.py
654c0b5ae82c98db163c7a44301ff6b23e19b211
src/test/python/apache/aurora/executor/common/test_task_info.py
Re: Review Request 34679: Correcting typo in developing-aurora-client.md for readability.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34679/#review85253 --- Ship it! Master (6db13ba) 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 May 26, 2015, 8:31 p.m., Will Swank wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34679/ --- (Updated May 26, 2015, 8:31 p.m.) Review request for Aurora. Repository: aurora Description --- Correcting typo in developing-aurora-client.md for readability. Diffs - docs/developing-aurora-client.md 4ddc77f1443e260986db04f2b7ec8dc17e6a7ac1 Diff: https://reviews.apache.org/r/34679/diff/ Testing --- Thanks, Will Swank
Re: Review Request 34300: Do better sanitation on the client side when encountering unbound pystachio refs
On May 16, 2015, 1:17 a.m., Kevin Sweeney wrote:
src/main/python/apache/aurora/config/schema/base.py, line 98
https://reviews.apache.org/r/34300/diff/1/?file=961840#file961840line98
Would be nice to have a wrapper here like Deprecated(Map(String,
String))
You filed https://github.com/wickman/pystachio/issues/9 over two years ago and
you're still probably right ;)
On May 16, 2015, 1:17 a.m., Kevin Sweeney wrote:
src/main/python/apache/aurora/config/__init__.py, lines 255-257
https://reviews.apache.org/r/34300/diff/1/?file=961839#file961839line255
Delete this property entirely rather than return a dummy value?
Presumably anything accessing still accessing it should AttributeError.
Zameer Manji wrote:
+1, this is something that can be deleted because it only deals with our
code and not configs.
Killed.
On May 16, 2015, 1:17 a.m., Kevin Sweeney wrote:
src/main/python/apache/aurora/config/thrift.py, lines 97-109
https://reviews.apache.org/r/34300/diff/1/?file=961841#file961841line97
Is this still necessary and/or buggy? Reading the comment it seems this
should have prevented seeing this bug.
Not buggy, just insufficient. We allow *some* unbound refs -- specifically
{{thermos.ports}}, {{mesos.task_id}}, and {{mesos.instance}}. The problem is
that if you do something like {{array[{{mesos.instance}}]}}, the only unbound
ref that pystachio sees is {{mesos.instance}} which it's willing to accept.
Once {{mesos.instance}} is bound (in the executor) then we know that
{{array[123]}} is actually an unbound ref. The approach taken in this review
is the simplest way (I could think of at least) to attempt to catch these
iteratively unbound refs, by mimicking what the executor would do but instead
on the client side.
On May 16, 2015, 1:17 a.m., Kevin Sweeney wrote:
src/main/python/apache/thermos/config/loader.py, lines 168-170
https://reviews.apache.org/r/34300/diff/1/?file=961844#file961844line168
This seems like a DRY violation - how do we ensure that any new fields
provided in the thermos namespace will be bound here? Could we iterate over
the available field names here instead?
There are TODOs in the code to move away from pystachio-binding in the executor
and instead doing %%-%% binding a la Aurora. This way we just bind
{{mesos.instance}} to %%instance%%, {{mesos.task_id}} to %%task_id%% and
{{thermos.ports[foo]}} to %%ports[foo]%%. This means that we can definitely
detect unbound refs since we'd require everything to be bound in the client.
We'd then remove pystachio support from the Thermos runner altogether and have
it do simple string replacement instead. Does this seem reasonable?
On May 16, 2015, 1:17 a.m., Kevin Sweeney wrote:
src/test/python/apache/aurora/client/test_config.py, lines 81-85
https://reviews.apache.org/r/34300/diff/1/?file=961845#file961845line81
Seems like there's no reason to write the config out to disk here
versus using a BytesIO, no?
Was just cargo culting. Will use BytesIO instead.
On May 16, 2015, 1:17 a.m., Kevin Sweeney wrote:
src/test/python/apache/aurora/client/test_config.py, lines 93-98
https://reviews.apache.org/r/34300/diff/1/?file=961845#file961845line93
Same here - can we use a BytesIO instead of an actual file?
Done
On May 16, 2015, 1:17 a.m., Kevin Sweeney wrote:
src/test/python/apache/aurora/config/test_thrift.py, line 148
https://reviews.apache.org/r/34300/diff/1/?file=961846#file961846line148
Drop this call entirely? Production code shouldn't reference
.task_links anymore
Done
On May 16, 2015, 1:17 a.m., Kevin Sweeney wrote:
src/test/python/apache/aurora/executor/common/test_task_info.py, line 53
https://reviews.apache.org/r/34300/diff/1/?file=961847#file961847line53
Rather than asserting on the contents of the error message, consider
defining a custom exception type
```
class UnexpectedUnboundRefs(ValueError): pass
```
and expecting that exception type.
done
On May 16, 2015, 1:17 a.m., Kevin Sweeney wrote:
src/test/python/apache/aurora/executor/common/test_task_info.py, line 73
https://reviews.apache.org/r/34300/diff/1/?file=961847#file961847line73
Consider expecting a specialized error type here as well.
done
- Brian
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/34300/#review84010
---
On May 16, 2015, 12:01 a.m., Brian Wickman wrote:
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/34300/
---
(Updated May 16, 2015, 12:01 a.m.)
Review request for Aurora and Kevin Sweeney.
Repository: aurora
Re: Review Request 34484: Daemonize all deadline calls in aurora executor.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34484/#review85256 --- Master (6db13ba) 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 May 26, 2015, 8:43 p.m., Brian Wickman wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34484/ --- (Updated May 26, 2015, 8:43 p.m.) Review request for Aurora, Joe Smith and Vinod Kone. Bugs: AURORA-698 https://issues.apache.org/jira/browse/AURORA-698 Repository: aurora Description --- Daemonize all deadline calls in aurora executor. If we do not daemonize, it's possible for the aurora executor to send TASK_KILLED and then block indefinitely on shutdown. This way the aurora executor process will at least exit, allow the cgroup to tear down all active processes. Diffs - src/main/python/apache/aurora/executor/aurora_executor.py df0df0cba3269a137a370e2b4096fd61fd3af258 Diff: https://reviews.apache.org/r/34484/diff/ Testing --- ./pants test src/test/python/apache/aurora/executor:: Thanks, Brian Wickman
Re: Review Request 34300: Do better sanitation on the client side when encountering unbound pystachio refs
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/34300/
---
(Updated May 26, 2015, 9:03 p.m.)
Review request for Aurora and Kevin Sweeney.
Changes
---
Address Kevin's review feedback.
Bugs: AURORA-739
https://issues.apache.org/jira/browse/AURORA-739
Repository: aurora
Description
---
It's possible to define nested refs that can cause the executor to stack trace,
e.g.
{{derp[{{thermos.ports[http]}}]}} is perfectly valid but crashes the executor.
Diffs (updated)
-
src/main/python/apache/aurora/config/__init__.py
dd2f89014a3da730364b14e01c499ac0f2c288c1
src/main/python/apache/aurora/config/schema/base.py
a87524a8b3ad5aa0e337e0a0028cecb85865b4e6
src/main/python/apache/aurora/config/thrift.py
810febb637d168b07c4aea77984e1d1451a39af2
src/main/python/apache/aurora/executor/common/task_info.py
d110faf08135d94d9af95ad74613950c56248c09
src/main/python/apache/thermos/config/dsl.py
0663a9ad8ae194b63ce6d77dabfe65564e2d26dd
src/main/python/apache/thermos/config/loader.py
d77ab9a52b16e9d65acdb95f01fd251ae8ab2b6e
src/test/python/apache/aurora/client/test_config.py
c56779712b91f621261358aa7ebd6c4fc65446a0
src/test/python/apache/aurora/config/test_thrift.py
654c0b5ae82c98db163c7a44301ff6b23e19b211
src/test/python/apache/aurora/executor/common/test_task_info.py
102ba531aa6c28f2d74bd0d7f1668e5861e3a6b8
Diff: https://reviews.apache.org/r/34300/diff/
Testing
---
Added some regression tests.
Thanks,
Brian Wickman
