This is an automatically generated e-mail. To reply, visit:

Great patch overall!  Mostly documentation and style nits, but also opening 
some questions.


    For this doc and others, i found the docs in .md to be more informative, 
please lift them here.  e.g., this was more helpful: `The path inside the 
container where the mount will be created.`.


    This should be camelCase, ditto below.


    This may be my docker ignorance showing, but can you expand on this doc?  
It's not blatantly obvious what this is for and how it differs from 


    What are the implications of this?  Does it mean a docker image must be 
pre-loaded on the host for it to be used?  Can we cope with the user specifying 
a bad path?  If not, what's the fallout - TASK_FAILED?


    Perhaps this should be called `imageName`?  Without help in the name, i 
could be convinced this was a path or URI.
    That said, what is this for?  As a user, can i incorrectly specify/format 
this string, or is it for my own purposes?


    s/A set of z/Z/
    As a convention, when type information is already expressed in a 
signature/declaration, tend away from repeating type information in the doc.


    rephrase suggestion:
    `*Note: the only container type currently supported is "docker"*`
    (italicized, on its own line at the top)


    It would be really useful to see a working example of these configuration 
parameters in concert.  I suggest you go ahead and wire it up in the upstart 
configs we have, and link to them from this doc.
    Taking it a step further, it would be awesome if this was exercised in 


    linkify docker containerizer to 


    s/aurora/mesos/?  IIUC it's the slave that does the copy.


    From reading the code, it appears this additional path is needed for them 
both to be available in the container.  If so, would it be easier to just 
accept an arbitrary number of additional assets to copy into the sandbox?  I 
would find that more generalized, and easier to understand.
    If you go with the above, i _think_ you can also safely nuke the extra args 


    Style nit: we usually leave one blank line between a wrapped method 
signature and the body, for better visual separation.


    I applied your patch and removed these PMD exclusions without any issue.  
Are they needed?
    As a general practice - we avoid decorating the code with hints to code 
quality checkers.  This could vary from making the code appease the checker to 
disabling the rule.


    It's good form for this type of sanitization to happen here, but at minimum 
the same sanitization must be done in `ConfigurationManager` to give the user a 
good message and avoid accepting the bad config.


    Declare these as Strings, do the Optional.of(X) where used.




    typo: specified


    Per previous comment, when the field is made camelCase, this jshint 
directive can go away.


    Whenever possible, use `ImmutableSet.of` rather than `Sets.newHashset`.


    should be +4 indent


    Pre-existing DRY violation getting worse here - initialize `config` to the 
common value in a `@Before` method.

- Bill Farner

On Jan. 6, 2015, 11:32 p.m., Steve Niemitz wrote:
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/28920/
> -----------------------------------------------------------
> (Updated Jan. 6, 2015, 11:32 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/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