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


Mostly style and configuration ergonomics remaining.  Once these are addressed 
my final pass before giving a ship will be assessing for complete test coverage.


docs/configuration-reference.md
<https://reviews.apache.org/r/28920/#comment112074>

    Flag as experimental somehow, here and in other new doc portions.



examples/vagrant/upstart/aurora-scheduler.conf
<https://reviews.apache.org/r/28920/#comment112075>

    remove extra newline



examples/vagrant/upstart/mesos-slave.conf
<https://reviews.apache.org/r/28920/#comment112076>

    Is this afflicted by MESOS-2215?  I'm worried (read: certain) people will 
copy/paste the example here, and likely be set up for failure.  I don't have an 
answer, curious what your thoughts are.



src/main/java/org/apache/aurora/scheduler/app/SchedulerMain.java
<https://reviews.apache.org/r/28920/#comment112077>

    The Args system handles multi-valued args, comma-separated.  Change to 
`Arg<List<String>>` and this will Just Work.



src/main/java/org/apache/aurora/scheduler/app/SchedulerMain.java
<https://reviews.apache.org/r/28920/#comment112107>

    Should this instead be a list of allowed `ContainerType`s?  If we want this 
to support arbitrary container types, seems like a good litmus test for success 
is absense of the string 'docker' from code and comments.  Of course, there may 
be special cases that dictate otherwise.



src/main/java/org/apache/aurora/scheduler/base/CommandUtil.java
<https://reviews.apache.org/r/28920/#comment112088>

    Please grep for 'wrapper' to make sure that code, comments, and docs refer 
to this as executor resources.



src/main/java/org/apache/aurora/scheduler/base/CommandUtil.java
<https://reviews.apache.org/r/28920/#comment112084>

    This one is not optional.



src/main/java/org/apache/aurora/scheduler/base/CommandUtil.java
<https://reviews.apache.org/r/28920/#comment112083>

    Feel free to omit "A[n] optional" from all of these, as the type 
self-documents.



src/main/java/org/apache/aurora/scheduler/base/CommandUtil.java
<https://reviews.apache.org/r/28920/#comment112078>

    blank line between wrapped method signature and body.



src/main/java/org/apache/aurora/scheduler/base/CommandUtil.java
<https://reviews.apache.org/r/28920/#comment112079>

    space-pad the colon:
    
        for (String resource : splitResources) {



src/main/java/org/apache/aurora/scheduler/base/CommandUtil.java
<https://reviews.apache.org/r/28920/#comment112089>

    shell=true is default, is there a reason you want to explicitly set it?  
I'm not against it, just want to be on the same page.



src/main/java/org/apache/aurora/scheduler/configuration/ConfigurationManager.java
<https://reviews.apache.org/r/28920/#comment112115>

    I would love to see another check for whether the scheduler is configured 
to allow docker mounts.  In fact, i'm not sure if `allow_docker_mounts` (or its 
replacement, based on another comment) should be plumbed anywhere else from 
here.
    
    Reasoning here is that only the cluster administrator should need to know 
what's on the slave command line.



src/main/java/org/apache/aurora/scheduler/configuration/ConfigurationManager.java
<https://reviews.apache.org/r/28920/#comment112090>

    please break these out and give a separate error message



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

    kick the first arg onto the next line, and use +4 indent for all (so a 
method rename doesn't move these out of alignment).



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

    blank line above



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

    Can you include a comment explaining why this is necessary?  Will be good 
for posterity.



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

    Prefer inserting line breaks to keep the longest continuous statement 
unbroken.  In this case, i would break after the `=` and put the rest on one 
line.



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

    blank line before



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

    blank line before



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

    blank line before



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

    line break after the paren, +4 indent for each arg after


- Bill Farner


On Jan. 13, 2015, 1:11 a.m., Steve Niemitz wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/28920/
> -----------------------------------------------------------
> 
> (Updated Jan. 13, 2015, 1:11 a.m.)
> 
> 
> Review request for Aurora, Jay Buffington, Kevin Sweeney, and Bill Farner.
> 
> 
> Bugs: AURORA-633
>     https://issues.apache.org/jira/browse/AURORA-633
> 
> 
> Repository: aurora
> 
> 
> Description
> -------
> 
> This change adds support for launching docker containers through aurora.  
> These changes are based off of the discussion in 
> https://issues.apache.org/jira/browse/AURORA-633
> 
> As of now, a special thermos_executor.sh script is needed to launch the 
> executor inside docker containers.  A sample aurora file is in 
> examples/jobs/docker.
> 
> In addition, mesos-slave must be run with `--containerizers=docker,mesos`, 
> the example upstart config in examples/vagrant/upstart has been updated to 
> reflect this.
> 
> More information is in subsequent review request comments.
> 
> 
> Diffs
> -----
> 
>   Vagrantfile f8b7db8eebdc6a10989de3bc9a2c3e89ce17f5fc 
>   api/src/main/thrift/org/apache/aurora/gen/api.thrift 
> 08ba1cdf88b712de22c26c04443079282db59ef9 
>   config/legacy_untested_classes.txt 33c1d6eb4ea02e01b7002c2c2bae5a3858c8b0e5 
>   docs/configuration-reference.md f3cb257206a194b82fd2045dc20456ee832dbcea 
>   docs/deploying-aurora-scheduler.md 711ae7eda07c2c1735601c265c06a88c1862cce7 
>   examples/jobs/docker/hello_docker.aurora PRE-CREATION 
>   examples/vagrant/aurorabuild.sh 1e31f21998d02fd69ce0db88e6adb3d32cff67fd 
>   examples/vagrant/provision-dev-cluster.sh 
> 7af4b52a6876268a97630279221bb98d9b04efad 
>   examples/vagrant/upstart/aurora-scheduler.conf 
> 788ec254270bca074dae91829c7f4fccdc8f8bb0 
>   examples/vagrant/upstart/mesos-slave.conf 
> 512ce7ecf34042ed68dda55efb2dd0415f8469db 
>   src/main/java/org/apache/aurora/scheduler/app/SchedulerMain.java 
> 72c7545e7f16549f6a9ccb5fb74a06f154a7ea94 
>   src/main/java/org/apache/aurora/scheduler/async/GcExecutorLauncher.java 
> 5226e3d1b303b1773a057078f2911c5ec2aa97f5 
>   src/main/java/org/apache/aurora/scheduler/base/CommandUtil.java 
> d885b224ec5a1d529347d84e03ba98ab6734a126 
>   
> src/main/java/org/apache/aurora/scheduler/configuration/ConfigurationManager.java
>  01b03508afac37b5a8f0ec5c3da1460695e1ef59 
>   src/main/java/org/apache/aurora/scheduler/mesos/MesosTaskFactory.java 
> 5bf283062c9d119ff91ed45da8b236e36d0fc9aa 
>   src/main/python/apache/aurora/config/thrift.py 
> ba94ac3c0cbaf3c91eb1a1d86a244ed6fa3b649c 
>   src/main/python/apache/aurora/executor/aurora_executor.py 
> 636b23d30a897b557eb8c3f8733c90b23cb807ef 
>   src/main/python/apache/aurora/executor/bin/thermos_executor_main.py 
> f7d8977e42aa56188799400bf8e12a6886fb4a8f 
>   src/main/python/apache/aurora/executor/common/sandbox.py 
> f47a32b3fefb4a89940b1ddc473b8316ac00df12 
>   src/main/python/apache/aurora/executor/thermos_task_runner.py 
> 5e4bd65537d186459003c0b9434f1b769e04f448 
>   src/main/python/apache/thermos/config/schema_base.py 
> f9143cc1b83143d6147f59d90c79435d055d0518 
>   src/main/python/apache/thermos/core/runner.py 
> 8aac6b50c66080abbb5308b367e9f74c487f42e3 
>   src/main/resources/scheduler/assets/configSummary.html 
> 28878908b0c9381e366b71a3135dfc28c542a890 
>   src/main/resources/scheduler/assets/js/services.js 
> b744f375411e09b7f776e4a05ee5961227143439 
>   src/test/java/org/apache/aurora/scheduler/app/SchedulerIT.java 
> 5e54364a49a208bd5f19b9649633dc8feca591e9 
>   src/test/java/org/apache/aurora/scheduler/base/CommandUtilTest.java 
> 876e173ccbac04e4a06a245648c7c6af15eaaa92 
>   
> src/test/java/org/apache/aurora/scheduler/configuration/ConfigurationManagerTest.java
>  dc2cb37adf32df0a6e4c7ee2ba776ba9f1f3c2f8 
>   
> src/test/java/org/apache/aurora/scheduler/mesos/MesosTaskFactoryImplTest.java 
> ddcb511d108220ab5e4efcf3496458f7ab4a20c2 
>   src/test/python/apache/aurora/executor/test_thermos_executor.py 
> 503e62f4cac872b14f6985b5bccc3e4dfcf81789 
> 
> Diff: https://reviews.apache.org/r/28920/diff/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Steve Niemitz
> 
>

Reply via email to