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 31338: Added a command line flag, -global_container_mounts, to allow mounting paths from the slaves into the (docker) containers they run.
Date Tue, 24 Feb 2015 06:01:47 GMT

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



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

    `Arg.<List<String>>create(...)` should let you avoid the cast?
    
    That said, since you're actually splitting the arg values, I think you should be able
to use a Map instead to simplify the parsing logic below a bit.
    
    c.f. https://github.com/apache/incubator-aurora/blob/e6e7e53d92b52d78960824022bef8a0546002180/src/main/java/org/apache/aurora/scheduler/thrift/auth/ThriftAuthModule.java#L43
    
    The accompanying command line format would be comma delimited key=value pairs. You could
still use a delimiter to allow for specifying the (optional) mode.



src/main/java/org/apache/aurora/scheduler/mesos/ExecutorSettings.java
<https://reviews.apache.org/r/31338/#comment120139>

    If we validate in the guice module as suggested below, then perhaps we can inject a parsed
map of host => (path, mode) values (maybe create a small bean to model the mount point).
That way we don't have to re-parse the values in the task factory below.



src/main/java/org/apache/aurora/scheduler/mesos/ExecutorSettings.java
<https://reviews.apache.org/r/31338/#comment120138>

    I think it would make more sense to validate these mounts in the Guice module where the
arg is defined? That way instead of throwing an IllegalArgumentException at runtime, we could
use Guice's addError method to have the problem reported at start up as a normal binding problem.



src/main/java/org/apache/aurora/scheduler/mesos/ExecutorSettings.java
<https://reviews.apache.org/r/31338/#comment120137>

    Extract constants for RW and RO? Maybe a(n Immutable) set? Or perhaps reuse the constants
on Volume used below? Then just ensure the value is a member of the set and we can also just
join the values of the set to generate the error message if necessary.


- Joshua Cohen


On Feb. 24, 2015, 2:07 a.m., Steve Niemitz wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/31338/
> -----------------------------------------------------------
> 
> (Updated Feb. 24, 2015, 2:07 a.m.)
> 
> 
> Review request for Aurora, Jay Buffington and Bill Farner.
> 
> 
> Bugs: AURORA-1107
>     https://issues.apache.org/jira/browse/AURORA-1107
> 
> 
> Repository: aurora
> 
> 
> Description
> -------
> 
> Added a command line flag, -global_container_mounts, to allow mounting paths from the
slaves into the (docker) containers they run.
> 
> This is the first portion of allowing per-job mounts, however, I wanted to get this out
first since more people want it.  I'll implement per-job mounts in a future review.
> 
> 
> Diffs
> -----
> 
>   docs/deploying-aurora-scheduler.md d1123359961fd59ddb8c1a07f80f293bdd46019f 
>   src/jmh/java/org/apache/aurora/benchmark/SchedulingBenchmarks.java bacfbfeb237ecddf82f58679e05be012c5214e61

>   src/main/java/org/apache/aurora/scheduler/app/SchedulerMain.java 24b61c1e4f615295acf28d904588e1512972d3f4

>   src/main/java/org/apache/aurora/scheduler/mesos/ExecutorSettings.java baacb71403d55c5b90fc11cb2a23f552a32e8ba5

>   src/main/java/org/apache/aurora/scheduler/mesos/MesosTaskFactory.java 5340d651b298ec8aa079e73d6d2f652fdf876293

>   src/test/java/org/apache/aurora/scheduler/app/SchedulerIT.java 6575b7d420f17ec68d6e2a83e7b380f684577d4f

>   src/test/java/org/apache/aurora/scheduler/mesos/MesosTaskFactoryImplTest.java 444d6d3fdaf86eb84612f846eaa326eb75c49898

>   src/test/java/org/apache/aurora/scheduler/mesos/TaskExecutors.java efe62ceb502ead88a2f0cd6d09a76664e465d9bc

> 
> Diff: https://reviews.apache.org/r/31338/diff/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Steve Niemitz
> 
>


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