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