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


This is 99% just style nits. Unfortunately our Java styleguide isn't published, 
but I'm working on rectifying that!


api/src/main/thrift/org/apache/aurora/gen/api.thrift
<https://reviews.apache.org/r/28920/#comment110279>

    nit: fix indentation, should be 2 spaces, not 4 (same goes for the Mode 
enum below).



examples/jobs/docker/hello_docker.aurora
<https://reviews.apache.org/r/28920/#comment110282>

    indent 2 to be consistent w/ the task below?



examples/jobs/docker/hello_docker.aurora
<https://reviews.apache.org/r/28920/#comment110280>

    put this on a single line?



examples/jobs/docker/hello_docker.aurora
<https://reviews.apache.org/r/28920/#comment110281>

    move to previous line.



examples/vagrant/aurorabuild.sh
<https://reviews.apache.org/r/28920/#comment110283>

    Fix indentation, avoid tabs



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

    Keep indentation consistent with the other args below (indent `help = 
"..."` four spaces).



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

    Insteat of concatenating the strings together just put the help on a 
separate line (applies to all instances of this style below).



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

    Do we need to plumb the wrapper path all the way through to the 
CommandUtil? What if instead of passing both along we figure out the correct 
path to use here at start up and passed that along directly?
    
    I.e. we could do something like...
    
       Optional<String> executorPath = 
Optional.of(THERMOS_EXECUTOR_PATH.get()).or(Optional.of(THERMOS_EXECUTOR_WRAPPER_PATH.get()));
       
       if (!executorPath.isPresent()) {
         throw new IllegalStateException(...);
       }
       
       bind(ExecutorSettings.class).toInstance(new ExecutorSettings(
           executorPath.get(),
           ...));



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

    style nit: method continuation should be formatted like:
    
        public static void create(
            String executorUri,
            String wrapperUri,
            String basePath,
            CommandInfo.Builder builder) {
            
          String uriToAdd;
          ...



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

    This could be inlined into the addVolumes call



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

    Fits on one line (we use 100 chars as the wrap point).



src/main/python/apache/thermos/config/schema_base.py
<https://reviews.apache.org/r/28920/#comment110305>

    2 blank lines between top level constructs (c.f. 
https://www.python.org/dev/peps/pep-0008/#blank-lines).



src/test/java/org/apache/aurora/scheduler/mesos/MesosTaskFactoryImplTest.java
<https://reviews.apache.org/r/28920/#comment110306>

    Fix indentation, should be 2 chars.



src/test/java/org/apache/aurora/scheduler/mesos/MesosTaskFactoryImplTest.java
<https://reviews.apache.org/r/28920/#comment110308>

    one param per line when exceeding line length:
    
        new ExecutorSettings(
            EXECUTOR_PATH,
            ...);



src/test/python/apache/aurora/executor/test_thermos_executor.py
<https://reviews.apache.org/r/28920/#comment110310>

    This seems like a change in semantics... the sandbox_provider previously 
was expected to be a factory function that returned the sandbox, now you're 
passing in the sandbox itself? Why the change?


- Joshua Cohen


On Jan. 5, 2015, 8:25 p.m., Steve Niemitz wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/28920/
> -----------------------------------------------------------
> 
> (Updated Jan. 5, 2015, 8:25 p.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 
> 5665c69cd7b49c3fd7345074c9f16a3b224496ab 
>   docs/configuration-reference.md f3cb257206a194b82fd2045dc20456ee832dbcea 
>   docs/deploying-aurora-scheduler.md 711ae7eda07c2c1735601c265c06a88c1862cce7 
>   examples/jobs/docker/hello_docker.aurora PRE-CREATION 
>   examples/vagrant/aurorabuild.sh 69983d0140b76c6869cd04e55d760f3e3a1e4262 
>   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/async/TaskScheduler.java 
> ead9d28100673440168a32d114ecaa15874978a6 
>   src/main/java/org/apache/aurora/scheduler/base/CommandUtil.java 
> d885b224ec5a1d529347d84e03ba98ab6734a126 
>   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 
> 9df9b4b79c0c7d29c5088409bf15c0d32a621df0 
>   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/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