-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/49218/#review139588
-----------------------------------------------------------




api/src/main/thrift/org/apache/aurora/gen/api.thrift (line 145)
<https://reviews.apache.org/r/49218/#comment204835>

    typo



api/src/main/thrift/org/apache/aurora/gen/api.thrift (line 146)
<https://reviews.apache.org/r/49218/#comment204836>

    The name appears to be too generic for its highly specialized use-case. 
Perhaps something along the lines of the "MesosFetcherUri" should be a better 
fit here?



src/main/java/org/apache/aurora/scheduler/mesos/MesosTaskFactory.java 
<https://reviews.apache.org/r/49218/#comment204837>

    Please revert, there should be a newline after the args spillover.



src/main/java/org/apache/aurora/scheduler/mesos/MesosTaskFactory.java (line 265)
<https://reviews.apache.org/r/49218/#comment204839>

    Favor java8 stream manipulations over Guava's where possible.
    
    Also, formatting is off, should be:
    ```
    Iterable<Protos.CommandInfo.URI> uris = Iterables.transform(
        task.getTask().getUris(),
        u -> Protos.CommandInfo.URI.newBuilder()
            .setValue(u.getValue())
            .setExecutable(false)
            .setExtract(u.isExtract())
            .setCache(u.isCache()).build());
    ```



src/main/java/org/apache/aurora/scheduler/mesos/MesosTaskFactory.java (line 267)
<https://reviews.apache.org/r/49218/#comment204840>

    This seems like a candidate for a command line flag documenting the direct 
security implications of changing the value here.
    
    Also, along the lines of Joshua's comments, suggest having another command 
line flag turning the feature off entirely (default) with a check in 
ConfigurationManager rejecting any tasks with fetcher URIs set.



src/main/java/org/apache/aurora/scheduler/storage/db/TaskConfigMapper.java 
(lines 155 - 157)
<https://reviews.apache.org/r/49218/#comment204843>

    Fits on a single line.



src/main/resources/org/apache/aurora/scheduler/storage/db/TaskConfigMapper.xml 
(line 406)
<https://reviews.apache.org/r/49218/#comment204847>

    Please, add/modify AbstractTaskStoreTest to test this codepath.


- Maxim Khutornenko


On June 24, 2016, 11:01 p.m., Renan DelValle wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/49218/
> -----------------------------------------------------------
> 
> (Updated June 24, 2016, 11:01 p.m.)
> 
> 
> Review request for Aurora.
> 
> 
> Repository: aurora
> 
> 
> Description
> -------
> 
> Adding a URIs field to TaskConfig inside the ThriftAPI so that users are able 
> to specify resources they wish to download into the sandbox per job.
> 
> 
> Diffs
> -----
> 
>   api/src/main/thrift/org/apache/aurora/gen/api.thrift 
> 3e6daf444453dd563dd7a2d494cc95e9a0aba0b6 
>   src/main/java/org/apache/aurora/scheduler/base/TaskTestUtil.java 
> 4089b79da8079243703eead884e80bcf736f8b29 
>   src/main/java/org/apache/aurora/scheduler/mesos/MesosTaskFactory.java 
> 3b01801d929dd61ee989495bf38af8f03e9f5ad4 
>   src/main/java/org/apache/aurora/scheduler/storage/db/TaskConfigManager.java 
> c76164292cf62d2181374c09f8bf6d8d3358e982 
>   src/main/java/org/apache/aurora/scheduler/storage/db/TaskConfigMapper.java 
> 571201094c1e576e496495a01cb83f6c57decfa8 
>   
> src/main/java/org/apache/aurora/scheduler/storage/db/migration/V007_CreateURIsTable.java
>  PRE-CREATION 
>   
> src/main/java/org/apache/aurora/scheduler/storage/db/views/DbTaskConfig.java 
> a90cb00e240df25dce6d55728859768e22d741a6 
>   
> src/main/resources/org/apache/aurora/scheduler/storage/db/TaskConfigMapper.xml
>  2c8af8b88e41b3b381cac831fd43b1057e4df0aa 
>   src/main/resources/org/apache/aurora/scheduler/storage/db/schema.sql 
> 5069bedc08bb7111d0e0f101c8a2c81495b97bc9 
>   
> src/test/java/org/apache/aurora/scheduler/mesos/MesosTaskFactoryImplTest.java 
> 58785bfa37ff214f26e9f94d836e6df40e411c3b 
>   src/test/java/org/apache/aurora/scheduler/thrift/Fixtures.java 
> a883b0e33bfec1d14e6fe4ee8ed2200d93acaeec 
> 
> Diff: https://reviews.apache.org/r/49218/diff/
> 
> 
> Testing
> -------
> 
> ./gradlew build -Pq
> ./build-support/jenkins/build.sh
> bash src/test/sh/org/apache/aurora/e2e/test_end_to_end.sh
> 
> 
> Thanks,
> 
> Renan DelValle
> 
>

Reply via email to