----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/25035/#review52763 -----------------------------------------------------------
src/master/hierarchical_allocator_process.hpp <https://reviews.apache.org/r/25035/#comment91816> kill trailing white space. we should really have the mesos-style.py catch this. filed: https://issues.apache.org/jira/browse/MESOS-1779 src/master/master.cpp <https://reviews.apache.org/r/25035/#comment91822> We use proper sentences (capitalization, periods etc) for comments. s/check/Check/ s/set/set./ src/master/master.cpp <https://reviews.apache.org/r/25035/#comment91839> How about: ``` Resources executorResources = task.executor.resources(); foreach (const Resource& resource, executorResources) { ..... } Option<double> cpus = executorResources.cpus(); if (cpus.isNone() || cpus.get() < MIN_CPUS) { LOG(WARNING) << ... } Option<Bytes> mem = executorResources.mem(); if (mem.isNone() || mem.get() < MIN_MEM) { LOG(WARNINIG) << ... } ``` src/master/master.cpp <https://reviews.apache.org/r/25035/#comment91823> Why do we need a minimum cpus for the executor? I'm assuming you didn't want to use MIN_CPUS because 0.1 cpus is too much overhead for Spark? 0.01 cpus equates to ~10 shares (1024 * 0.01) which is the minium amount of shares enforced by the cpu isolator; so, let's just change MIN_CPUS to 0.01 and get rid of MIN_CPUS_EXECUTOR. src/master/master.cpp <https://reviews.apache.org/r/25035/#comment91830> s/executor/Executor/ s/should/ should/ src/master/master.cpp <https://reviews.apache.org/r/25035/#comment91817> kill extra white space. src/master/master.cpp <https://reviews.apache.org/r/25035/#comment91818> kill extra white space. src/master/master.cpp <https://reviews.apache.org/r/25035/#comment91833> s/executor/Executor/ s/should/ should/ src/master/master.cpp <https://reviews.apache.org/r/25035/#comment91840> I like these warnings. Are you planning to get this in to 0.20.1 or 0.21.0 ? If the former, can you add this to the list of deprecations in CHANGELOG. src/tests/allocator_tests.cpp <https://reviews.apache.org/r/25035/#comment91842> s/pure cpu/cpu only/ ? s/offered/offered./ Also, this test is doing more than checking resources are offered. It is also testing that task(s) which use cpus only are launched. Can you please add that to the comment? Note that this test and the next one will break once we disallow such launches in the future, but that is good to have. src/tests/allocator_tests.cpp <https://reviews.apache.org/r/25035/#comment91843> Just say: // Start a slave with cpu only resources. src/tests/allocator_tests.cpp <https://reviews.apache.org/r/25035/#comment91847> Capitalize and period. Also, why launch two tasks instead of one? Makes the test a bit complicated and not sure you are gaining much. src/tests/allocator_tests.cpp <https://reviews.apache.org/r/25035/#comment91848> Capitalize and period. src/tests/allocator_tests.cpp <https://reviews.apache.org/r/25035/#comment91849> s/pure memory/memory only/ ? s/offered/offered./ also, see comments on the previous test. src/tests/allocator_tests.cpp <https://reviews.apache.org/r/25035/#comment91850> // Start slave with memory only resources. src/tests/allocator_tests.cpp <https://reviews.apache.org/r/25035/#comment91851> Period at the end. ditto. see comments in the previous test. - Vinod Kone On Sept. 6, 2014, 10:03 p.m., Martin Weindel wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/25035/ > ----------------------------------------------------------- > > (Updated Sept. 6, 2014, 10:03 p.m.) > > > Review request for mesos and Vinod Kone. > > > Bugs: MESOS-1688 > https://issues.apache.org/jira/browse/MESOS-1688 > > > Repository: mesos-git > > > Description > ------- > > As already explained in JIRA MESOS-1688, there are schedulers allocating > memory only for the executor and not for tasks. For tasks only CPU resources > are allocated in this case. > Such a scheduler does not get offered any idle CPUs if the slave has nearly > used up all memory. > This can easily lead to a dead lock (in the application, not in Mesos). > > Simple example: > 1. Scheduler allocates all memory of a slave for an executor > 2. Scheduler launches a task for this executor (allocating 1 CPU) > 3. Task finishes: 1 CPU , 0 MB memory allocatable. > 4. No offers are made, as no memory is left. Scheduler will wait for offers > forever. Dead lock in the application. > > To fix this problem, offers must be made if CPU resources are allocatable > without considering allocatable memory > > > Diffs > ----- > > src/common/resources.cpp edf36b1 > src/master/constants.hpp ce7995b > src/master/constants.cpp faa1503 > src/master/hierarchical_allocator_process.hpp 34f8cd6 > src/master/master.cpp 18464ba > src/tests/allocator_tests.cpp 774528a > > Diff: https://reviews.apache.org/r/25035/diff/ > > > Testing > ------- > > Deployed patched Mesos 0.19.1 on a small cluster with 3 slaves and tested > running multiple parallel Spark jobs in "fine-grained" mode to saturate > allocatable memory. The jobs run fine now. This load always caused a dead > lock in all Spark jobs within one minute with the unpatched Mesos. > > > Thanks, > > Martin Weindel > >
