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