aurora-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Kevin Sweeney" <kevi...@apache.org>
Subject Re: Review Request 34300: Do better sanitation on the client side when encountering unbound pystachio refs
Date Sat, 16 May 2015 01:17:50 GMT

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


Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message