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



src/main/java/org/apache/aurora/scheduler/ResourceSlot.java
<https://reviews.apache.org/r/28193/#comment104640>

    tabbing is off



src/main/java/org/apache/aurora/scheduler/configuration/Resources.java
<https://reviews.apache.org/r/28193/#comment104644>

    This is only used in tests outside of this class. Consider reverting to 
private.



src/main/java/org/apache/aurora/scheduler/configuration/Resources.java
<https://reviews.apache.org/r/28193/#comment104647>

    +1 on moving it closer to its only consumer. That's a general guideline we 
follow everywhere.



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

    Not related to your change but consider renaming it to something different 
(e.g. ExecutorSettings) to avoid naming collision with the thrift object.



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

    "-100MB" looks like negative resource and is confusing.



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

    Why not MIN_EXECUTOR_RESOURCES? We normally abstract out from the process 
framework concept in the scheduler code.



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

    Any justification for the min resources chosen similar to the above?



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

    Convert to // for inline comments.



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

    Unless I am missing something, all you need to do here is to make sure 
neither containerResources nor executorResources is less than min. Can you do 
something like:
    finalTaskResources = Resources.maxElements(containerResources, 
MIN_TASK_RESOURCES);
    
    and replace ".addAllResources(MIN_THERMOS_RESOURCES.toResourceList())" with
    .addAllResources(Resources.maxElements(executorOverhead, 
MIN_THERMOS_RESOURCES))?


- Maxim Khutornenko


On Nov. 21, 2014, 5:01 a.m., Zameer Manji wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/28193/
> -----------------------------------------------------------
> 
> (Updated Nov. 21, 2014, 5:01 a.m.)
> 
> 
> Review request for Aurora, Maxim Khutornenko and Bill Farner.
> 
> 
> Bugs: AURORA-928
>     https://issues.apache.org/jira/browse/AURORA-928
> 
> 
> Repository: aurora
> 
> 
> Description
> -------
> 
> Mesos rejects tasks and executors that are zero sized. This patch 
> reconfigures Aurora to ensure no zero sized tasks and executors are created.
> 
> 
> Diffs
> -----
> 
>   src/main/java/org/apache/aurora/scheduler/ResourceSlot.java 
> ed60447c798a97daceda4a3bba6ee9bcdcaedd0f 
>   src/main/java/org/apache/aurora/scheduler/app/SchedulerMain.java 
> 40b652c679d8e340f585e28cbed066335d9d760d 
>   src/main/java/org/apache/aurora/scheduler/configuration/Resources.java 
> 65c4b526c89a4d5607af4424ebe49bb48e296ae9 
>   src/main/java/org/apache/aurora/scheduler/mesos/MesosTaskFactory.java 
> bb227fd86f7c4c692f6ae2aef1c15a94913354b7 
>   src/test/java/org/apache/aurora/scheduler/app/SchedulerIT.java 
> 899416fceae498353880012b8a93491cff461064 
>   src/test/java/org/apache/aurora/scheduler/configuration/ResourcesTest.java 
> d6febb8998e05257cabe8d193cefa0b6c79f197e 
>   
> src/test/java/org/apache/aurora/scheduler/mesos/MesosTaskFactoryImplTest.java 
> 953c1edb6802d8983ab324aa56361e5c8fbe2e68 
> 
> Diff: https://reviews.apache.org/r/28193/diff/
> 
> 
> Testing
> -------
> 
> ./gradlew clean build -Pq
> 
> 
> Thanks,
> 
> Zameer Manji
> 
>

Reply via email to