aurora-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Joshua Cohen" <jco...@twopensource.com>
Subject Re: Review Request 28920: Add support for docker containers to aurora
Date Mon, 05 Jan 2015 20:41:43 GMT

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


Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message