You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@aurora.apache.org by Brian Wickman <wi...@apache.org> on 2015/05/16 02:01:05 UTC
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/
-----------------------------------------------------------
Review request for Aurora and Kevin Sweeney.
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 102ba531aa6c28f2d74bd0d7f1668e5861e3a6b8
Diff: https://reviews.apache.org/r/34300/diff/
Testing
-------
Added some regression tests.
Thanks,
Brian Wickman
Re: Review Request 34300: Do better sanitation on the client side when
encountering unbound pystachio refs
Posted by Aurora ReviewBot <wf...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/34300/#review86028
-----------------------------------------------------------
Ship it!
Master (827b9ab) 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 June 1, 2015, 6:05 p.m., Brian Wickman wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/34300/
> -----------------------------------------------------------
>
> (Updated June 1, 2015, 6:05 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 102ba531aa6c28f2d74bd0d7f1668e5861e3a6b8
>
> Diff: https://reviews.apache.org/r/34300/diff/
>
>
> Testing
> -------
>
> Added some regression tests.
>
>
> Thanks,
>
> Brian Wickman
>
>
Re: Review Request 34300: Do better sanitation on the client side when
encountering unbound pystachio refs
Posted by Brian Wickman <wi...@apache.org>.
> On June 4, 2015, 11:55 p.m., Kevin Sweeney wrote:
> > src/main/python/apache/aurora/config/thrift.py, line 218
> > <https://reviews.apache.org/r/34300/diff/3/?file=975774#file975774line218>
> >
> > Why set this at all? Does the scheduler read this field?
it is not. i will leave it up to AURORA-739 to remove support entirely (or add it back in a sensible way.)
- Brian
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/34300/#review86742
-----------------------------------------------------------
On June 1, 2015, 6:05 p.m., Brian Wickman wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/34300/
> -----------------------------------------------------------
>
> (Updated June 1, 2015, 6:05 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 102ba531aa6c28f2d74bd0d7f1668e5861e3a6b8
>
> Diff: https://reviews.apache.org/r/34300/diff/
>
>
> Testing
> -------
>
> Added some regression tests.
>
>
> Thanks,
>
> Brian Wickman
>
>
Re: Review Request 34300: Do better sanitation on the client side when
encountering unbound pystachio refs
Posted by Kevin Sweeney <ke...@apache.org>.
> On June 4, 2015, 4:55 p.m., Kevin Sweeney wrote:
> > src/main/python/apache/aurora/config/thrift.py, line 218
> > <https://reviews.apache.org/r/34300/diff/3/?file=975774#file975774line218>
> >
> > Why set this at all? Does the scheduler read this field?
>
> Brian Wickman wrote:
> it is not. i will leave it up to AURORA-739 to remove support entirely (or add it back in a sensible way.)
In that case, please drop this set from the client-side.
- Kevin
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/34300/#review86742
-----------------------------------------------------------
On June 1, 2015, 11:05 a.m., Brian Wickman wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/34300/
> -----------------------------------------------------------
>
> (Updated June 1, 2015, 11:05 a.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 102ba531aa6c28f2d74bd0d7f1668e5861e3a6b8
>
> Diff: https://reviews.apache.org/r/34300/diff/
>
>
> Testing
> -------
>
> Added some regression tests.
>
>
> Thanks,
>
> Brian Wickman
>
>
Re: Review Request 34300: Do better sanitation on the client side when
encountering unbound pystachio refs
Posted by Kevin Sweeney <ke...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/34300/#review86742
-----------------------------------------------------------
Ship it!
src/main/python/apache/aurora/config/thrift.py
<https://reviews.apache.org/r/34300/#comment138805>
Why set this at all? Does the scheduler read this field?
- Kevin Sweeney
On June 1, 2015, 11:05 a.m., Brian Wickman wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/34300/
> -----------------------------------------------------------
>
> (Updated June 1, 2015, 11:05 a.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 102ba531aa6c28f2d74bd0d7f1668e5861e3a6b8
>
> Diff: https://reviews.apache.org/r/34300/diff/
>
>
> Testing
> -------
>
> Added some regression tests.
>
>
> Thanks,
>
> Brian Wickman
>
>
Re: Review Request 34300: Do better sanitation on the client side when
encountering unbound pystachio refs
Posted by Joe Smith <ya...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/34300/#review88188
-----------------------------------------------------------
Ship it!
Ship It!
- Joe Smith
On June 1, 2015, 11:05 a.m., Brian Wickman wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/34300/
> -----------------------------------------------------------
>
> (Updated June 1, 2015, 11:05 a.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 102ba531aa6c28f2d74bd0d7f1668e5861e3a6b8
>
> Diff: https://reviews.apache.org/r/34300/diff/
>
>
> Testing
> -------
>
> Added some regression tests.
>
>
> Thanks,
>
> Brian Wickman
>
>
Re: Review Request 34300: Do better sanitation on the client side when
encountering unbound pystachio refs
Posted by Brian Wickman <wi...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/34300/
-----------------------------------------------------------
(Updated June 1, 2015, 6:05 p.m.)
Review request for Aurora and Kevin Sweeney.
Changes
-------
Fix checkstyle violation.
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
Re: Review Request 34300: Do better sanitation on the client side when
encountering unbound pystachio refs
Posted by Aurora ReviewBot <wf...@apache.org>.
-----------------------------------------------------------
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 102ba531aa6c28f2d74bd0d7f1668e5861e3a6b8
>
> Diff: https://reviews.apache.org/r/34300/diff/
>
>
> Testing
> -------
>
> Added some regression tests.
>
>
> Thanks,
>
> Brian Wickman
>
>
Re: Review Request 34300: Do better sanitation on the client side when
encountering unbound pystachio refs
Posted by Brian Wickman <wi...@apache.org>.
-----------------------------------------------------------
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
Re: Review Request 34300: Do better sanitation on the client side when
encountering unbound pystachio refs
Posted by Brian Wickman <wi...@apache.org>.
> 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
>
>
> 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 102ba531aa6c28f2d74bd0d7f1668e5861e3a6b8
>
> Diff: https://reviews.apache.org/r/34300/diff/
>
>
> Testing
> -------
>
> Added some regression tests.
>
>
> Thanks,
>
> Brian Wickman
>
>
Re: Review Request 34300: Do better sanitation on the client side when
encountering unbound pystachio refs
Posted by Zameer Manji <zm...@apache.org>.
> On May 15, 2015, 6:17 p.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.
+1, this is something that can be deleted because it only deals with our code and not configs.
- Zameer
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/34300/#review84010
-----------------------------------------------------------
On May 15, 2015, 5:01 p.m., Brian Wickman wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/34300/
> -----------------------------------------------------------
>
> (Updated May 15, 2015, 5:01 p.m.)
>
>
> Review request for Aurora and Kevin Sweeney.
>
>
> 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 102ba531aa6c28f2d74bd0d7f1668e5861e3a6b8
>
> Diff: https://reviews.apache.org/r/34300/diff/
>
>
> Testing
> -------
>
> Added some regression tests.
>
>
> Thanks,
>
> Brian Wickman
>
>
Re: Review Request 34300: Do better sanitation on the client side when
encountering unbound pystachio refs
Posted by Kevin Sweeney <ke...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/34300/#review84010
-----------------------------------------------------------
src/main/python/apache/aurora/config/__init__.py
<https://reviews.apache.org/r/34300/#comment135134>
Delete this property entirely rather than return a dummy value? Presumably anything accessing still accessing it should AttributeError.
src/main/python/apache/aurora/config/schema/base.py
<https://reviews.apache.org/r/34300/#comment135137>
Would be nice to have a wrapper here like Deprecated(Map(String, String))
src/main/python/apache/aurora/config/thrift.py
<https://reviews.apache.org/r/34300/#comment135136>
Is this still necessary and/or buggy? Reading the comment it seems this should have prevented seeing this bug.
src/main/python/apache/thermos/config/loader.py
<https://reviews.apache.org/r/34300/#comment135139>
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?
src/test/python/apache/aurora/client/test_config.py
<https://reviews.apache.org/r/34300/#comment135140>
Seems like there's no reason to write the config out to disk here versus using a BytesIO, no?
src/test/python/apache/aurora/client/test_config.py
<https://reviews.apache.org/r/34300/#comment135141>
Same here - can we use a BytesIO instead of an actual file?
src/test/python/apache/aurora/config/test_thrift.py
<https://reviews.apache.org/r/34300/#comment135142>
Drop this call entirely? Production code shouldn't reference .task_links anymore
src/test/python/apache/aurora/executor/common/test_task_info.py
<https://reviews.apache.org/r/34300/#comment135143>
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.
src/test/python/apache/aurora/executor/common/test_task_info.py
<https://reviews.apache.org/r/34300/#comment135144>
Consider expecting a specialized error type here as well.
- Kevin Sweeney
On May 15, 2015, 5:01 p.m., Brian Wickman wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/34300/
> -----------------------------------------------------------
>
> (Updated May 15, 2015, 5:01 p.m.)
>
>
> Review request for Aurora and Kevin Sweeney.
>
>
> 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 102ba531aa6c28f2d74bd0d7f1668e5861e3a6b8
>
> Diff: https://reviews.apache.org/r/34300/diff/
>
>
> Testing
> -------
>
> Added some regression tests.
>
>
> Thanks,
>
> Brian Wickman
>
>
Re: Review Request 34300: Do better sanitation on the client side when
encountering unbound pystachio refs
Posted by Aurora ReviewBot <wf...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/34300/#review84021
-----------------------------------------------------------
Ship it!
Master (07ab0bb) 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 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
>
>
> 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 102ba531aa6c28f2d74bd0d7f1668e5861e3a6b8
>
> Diff: https://reviews.apache.org/r/34300/diff/
>
>
> Testing
> -------
>
> Added some regression tests.
>
>
> Thanks,
>
> Brian Wickman
>
>