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



Overall looks good to me. Few fixes here and there. I'll look at the python 
code more thorugoughly next round.


api/src/main/thrift/org/apache/aurora/gen/api.thrift
Lines 203 (patched)
<https://reviews.apache.org/r/66490/#comment281623>

    Can we get away with using a string here instead of a struct?



api/src/main/thrift/org/apache/aurora/gen/api.thrift
Lines 1263 (patched)
<https://reviews.apache.org/r/66490/#comment281621>

    I think the better solution here would be to make Volume.hostPath optional 
and handle hostPath not being set at the Java server side. Maybe have hostPath 
mirror to VolumeSource.hostpath while it is being deprecated.
    
    This can be done somewhere in 
`src/main/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterface.java`



src/main/java/org/apache/aurora/scheduler/mesos/MesosTaskFactory.java
Lines 277 (patched)
<https://reviews.apache.org/r/66490/#comment281624>

    I'm not 100% sure what happens if VolumeType is not set (esp since it's 
optional) here but it may crash the scheduler. There should be a check added to 
`src/main/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterface.java`
    
    That way even if someone uses a custom client to schedule a job on Aurora 
it'll be able to catch this before it's too late.
    
    It would also be great to add tests to 
`src/test/java/org/apache/aurora/scheduler/mesos/MesosTaskFactoryImplTest.java` 
using this new feature.



src/main/java/org/apache/aurora/scheduler/mesos/MesosTaskFactory.java
Lines 284 (patched)
<https://reviews.apache.org/r/66490/#comment281626>

    Can we replace all the iterables in this patch with their Java 8 
counterpart (e.g. streams)?
    
    The project is currently moving in the direction of deprecating the use of 
Guava features that are now replaceable by features in Java 8.


- Renan DelValle


On April 6, 2018, 5:04 p.m., Justin Venus wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/66490/
> -----------------------------------------------------------
> 
> (Updated April 6, 2018, 5:04 p.m.)
> 
> 
> Review request for Aurora, Renan DelValle and Stephan Erb.
> 
> 
> Bugs: AURORA-1983
>     https://issues.apache.org/jira/browse/AURORA-1983
> 
> 
> Repository: aurora
> 
> 
> Description
> -------
> 
> This change implements the docker/volume isolator as described at 
> http://mesos.apache.org/documentation/latest/isolators/docker-volume/.
>  
> * update config thrift tests to check for docker/volume isolator support
> * update mesos task factory to support docker/volume isolator
> * update documentation for docker/volume isolator support
> 
> 
> Diffs
> -----
> 
>   api/src/main/thrift/org/apache/aurora/gen/api.thrift 
> ef754e32172e7490a47a13e7b526f243ffa3efeb 
>   docs/reference/configuration.md d4b869b938105ba301fc88d41019af2f1707f6f4 
>   src/main/java/org/apache/aurora/scheduler/mesos/MesosTaskFactory.java 
> bcb2bbf882f43d813dd26c746d806e78bae6bcf3 
>   src/main/python/apache/aurora/config/schema/base.py 
> a629bcd1261e5959da0a8458a55545d4e2c2a7a5 
>   src/main/python/apache/aurora/config/thrift.py 
> 6d2dde6e964daa68bf6f0e5bbbffecc5bd8c0431 
>   src/test/python/apache/aurora/config/test_thrift.py 
> 8e1d0e177959af12b97bdd1cd47845b72bc12fe1 
> 
> 
> Diff: https://reviews.apache.org/r/66490/diff/4/
> 
> 
> Testing
> -------
> 
> Tests pass locally
> ```sh
> ./gradlew test
> ./pants test src/test/python::
> ```
> 
> I've deployed this code to a test cluster with rexray, dvdcli, and the agents 
> have `docker/volume` isolation enabled.  I am able to exercise mounting an 
> EBS volume as I would expect into a container.
> 
> 
> Thanks,
> 
> Justin Venus
> 
>

Reply via email to