aurora-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Maxim Khutornenko" <ma...@apache.org>
Subject Re: Review Request 28193: Prevent Aurora from creating zero sized Executor tasks.
Date Fri, 21 Nov 2014 20:49:05 GMT


> On Nov. 21, 2014, 6:41 p.m., Maxim Khutornenko wrote:
> > src/main/java/org/apache/aurora/scheduler/configuration/Resources.java, line 196
> > <https://reviews.apache.org/r/28193/diff/2/?file=772028#file772028line196>
> >
> >     This is only used in tests outside of this class. Consider reverting to private.
> 
> Zameer Manji wrote:
>     I think using this constant in tests makes the tests a bit simplier. I have added
a '@VisibleForTesting' annotation to signifiy this.

Using @VisibleForTesting is rather an exception when you want to reuse the complex definition.
You already re-define SOME_EXECUTOR_OVERHEAD for test purposes, why not do the same for NO_EXECUTOR_OVERHEAD?


> On Nov. 21, 2014, 6:41 p.m., Maxim Khutornenko wrote:
> > src/main/java/org/apache/aurora/scheduler/configuration/Resources.java, line 406
> > <https://reviews.apache.org/r/28193/diff/2/?file=772028#file772028line406>
> >
> >     +1 on moving it closer to its only consumer. That's a general guideline we follow
everywhere.
> 
> Zameer Manji wrote:
>     I really think it should belong with the Resources class because it is equally as
useful as .sum in my opinion. If you disagree I will move it closer to the consumer.

You can always move it there when there is a use case. Until then, it's better follow our
style any open up only those things that are used in more than one place.


> On Nov. 21, 2014, 6:41 p.m., Maxim Khutornenko wrote:
> > src/main/java/org/apache/aurora/scheduler/mesos/MesosTaskFactory.java, line 94
> > <https://reviews.apache.org/r/28193/diff/2/?file=772029#file772029line94>
> >
> >     Why not MIN_EXECUTOR_RESOURCES? We normally abstract out from the process framework
concept in the scheduler code.
> 
> Zameer Manji wrote:
>     These minimum values are for thermos. Another executor might require more resources
to function.

Did not we want to eliminate it completely though but Mesos did not let us do that? I suggest
we just use a default and abstract MIN_EXECUTOR_RESOURCES and address the real need to differentiate
when/if it comes up. Also, when https://reviews.apache.org/r/28345/ lands, the 100MB will
become more like 0.5 MB, so it clearly feels like an arbitrary Mesos workaround rather than
a true MIN enforcement.


> On Nov. 21, 2014, 6:41 p.m., Maxim Khutornenko wrote:
> > src/main/java/org/apache/aurora/scheduler/mesos/MesosTaskFactory.java, lines 166-173
> > <https://reviews.apache.org/r/28193/diff/2/?file=772029#file772029line166>
> >
> >     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))?
> 
> Zameer Manji wrote:
>     I would always like to allocate MIN_THERMOS_RESOURCES for the executor. What you
are proposing will make it possible to allocate more CPU or RAM. This is a change in behaviour
from before where we were always allocated a fixed amount for the executor.
>     
>     I can change it to this if you insist but I prefer to allocate a fixed amount for
the executor.

Valid point. Though given the randomness of the applied MIN requirement I am not sure how
important it is. I would go with a more readable and simple approach here. Your call.


- Maxim


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


On Nov. 21, 2014, 8:34 p.m., Zameer Manji wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/28193/
> -----------------------------------------------------------
> 
> (Updated Nov. 21, 2014, 8:34 p.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
> 
>


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