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