----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/71858/#review219154 -----------------------------------------------------------
src/slave/slave.cpp Lines 3115 (patched) <https://reviews.apache.org/r/71858/#comment307257> How about s/value/delta/ to make it more explicit that the argument will be added to the existing limit? src/slave/slave.cpp Lines 3116 (patched) <https://reviews.apache.org/r/71858/#comment307256> Are we using `-1` for this? I thought the plan was to use `INFINITY`? src/slave/slave.cpp Lines 3134 (patched) <https://reviews.apache.org/r/71858/#comment307261> I would recommend s/tasksResources/totalTaskResources/ to improve readability. src/slave/slave.cpp Lines 3135-3136 (patched) <https://reviews.apache.org/r/71858/#comment307259> Could you be consistent with the naming here, either do "executorCpuLimit/executorCpuRequest" or "cpuLimit/cpuRequest"? src/slave/slave.cpp Lines 3137 (patched) <https://reviews.apache.org/r/71858/#comment307260> Our style guide prefers trailing underscores in cases like this, could you use `task_` instead of `_task`? src/slave/slave.cpp Lines 3140-3201 (patched) <https://reviews.apache.org/r/71858/#comment307264> I think maybe this logic could be easier to read if we do something like: ``` auto limitsAreSet = [](const vector<TaskInfo>& tasks) { foreach (const TaskInfo& task, tasks) { if (!task.limits().empty()) { return true; } } return false; }; Option<Map<string, Value::Scalar>> executorLimits; if (limitsAreSet(tasks)) { executorLimits = Map<string, Value::Scalar>(); foreach (const TaskInfo& task, tasks) { // Add up task limits/requests here. } } ``` What do you think? src/slave/slave.cpp Lines 3623 (patched) <https://reviews.apache.org/r/71858/#comment307263> Should this be an `Option`? So that we can only set `containerConfig.limits` when limits have actually been set? - Greg Mann On Dec. 31, 2019, 9:08 a.m., Qian Zhang wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/71858/ > ----------------------------------------------------------- > > (Updated Dec. 31, 2019, 9:08 a.m.) > > > Review request for mesos, Andrei Budnik and Greg Mann. > > > Bugs: MESOS-10046 > https://issues.apache.org/jira/browse/MESOS-10046 > > > Repository: mesos > > > Description > ------- > > WIP: Set resource limits when launching executor container. > > > Diffs > ----- > > src/slave/slave.hpp 77b5bc0082c6bb73fbd48a2ebe812629921645cb > src/slave/slave.cpp 3839a120446339fea8aa857f431a2dba28ed4002 > > > Diff: https://reviews.apache.org/r/71858/diff/4/ > > > Testing > ------- > > > Thanks, > > Qian Zhang > >
