mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Qian Zhang <zhq527...@gmail.com>
Subject Re: Review Request 71858: Set resource limits when launching executor container.
Date Tue, 03 Mar 2020 07:07:06 GMT


> On Jan. 8, 2020, 5:43 a.m., Greg Mann wrote:
> > src/slave/slave.cpp
> > Lines 3623 (patched)
> > <https://reviews.apache.org/r/71858/diff/4/?file=2191535#file2191535line3628>
> >
> >     Should this be an `Option`? So that we can only set `containerConfig.limits`
when limits have actually been set?
> 
> Qian Zhang wrote:
>     I added a check `if (!executorLimits.empty()) {` before setting `containerConfig.limit`,
HDYT?
> 
> Greg Mann wrote:
>     If we use an option, then the type in the function signature more precisely expresses
the semantics of the function, which improves readability IMO.
> 
> Qian Zhang wrote:
>     I actually thought about it before, but it may make the code a bit more complicated.
Here is the code where we call `launchExecutor()`:
>     ```
>         defer(
>               self(),
>               &Self::launchExecutor,
>               lambda::_1,
>               frameworkId,
>               executorInfo_,
>               executorLimits,
>               taskGroup.isNone() ? task.get() : Option<TaskInfo>::none()));
>     ```
>     
>     The type of the variable `executorLimits` is `google::protobuf::Map` rather than
`Option<google::protobuf::Map>`. So if we change the type of the parameter `executorLimits`
of the `launchExecutor` method to `Option<google::protobuf::Map>`, its `isSome()` will
actually always be true since a map `executorLimits` will always be passed to it, that means
checking `executorLimits.isSome()` in `launchExecutor` is actually redundant. To make it not
redundant, I may need to change the type of the variable `executorLimits` from `google::protobuf::Map`
to `Option<google::protobuf::Map>`in the caller's code and define another local variable
of type `google::protobuf::Map` and set `executorLimits` to that variable when we need to
set executor's resource limits, I think it is bit more complicated than the current code in
this patch.
>     
>     Another option would be set the default value of the `executorLimits` parameter to
an empty map (i.e. `{}`), like: 
>     ```
>     const google::protobuf::Map<string, Value::Scalar>& executorLimits = {},
>     ```
>     Does it help?
> 
> Qian Zhang wrote:
>     The reason that I did not set the default value of the `executorLimits` parameter
to `{}` is, the variable `executorLimits` in the caller side is `{}` by default :-)
> 
> Greg Mann wrote:
>     > To make it not redundant, I may need to change the type of the variable executorLimits
from google::protobuf::Map to Option\<google::protobuf::Map>in the caller's code and
define another local variable of type google::protobuf::Map and set executorLimits to that
variable when we need to set executor's resource limits
>     
>     Personally, that approach sounds better to me. I might be missing something, but
this seems like a classic use case for the `Option` type.
> 
> Qian Zhang wrote:
>     I think the classic use case for the `Option` type is like:
>     ```
>     Option<int> var;
>     if (...) {
>       var = <An int value>
>     }
>     ```
>     
>     But here with the `Option` type, the code will be something like:
>     ```
>     Option<Map> executorLimits;
>     Map executorLimits_;
>     
>     if (executorCpuLimit.isSome()) {
>       executorLimits_.insert({"cpus", executorCpuLimit.get()});
>     }
>     
>     if (executorMemLimit.isSome()) {
>       executorLimits_.insert({"mem", executorMemLimit.get()});
>     }
>     
>     if (!executorLimits_.empty()) {
>      executorLimits = executorLimits_;
>     }
>     
>     ```
>     
>     The above code is a bit redundant to me, I think the code like the below is better:
>     ```
>     Map executorLimits;
>     
>     if (executorCpuLimit.isSome()) {
>       executorLimits.insert({"cpus", executorCpuLimit.get()});
>     }
>     
>     if (executorMemLimit.isSome()) {
>       executorLimits.insert({"mem", executorMemLimit.get()});
>     }
>     ```
> 
> Greg Mann wrote:
>     Ah, this is related to my other comment about refactoring L3140 to L3201 :)
>     
>     If we used an `Option` and refactored as I proposed, and if we will always set the
CPU and mem hard limits together (i.e. never set just CPU hard limits but not mem hard limits),
then we could do something like:
>     
>     ```
>     foreach (const TaskInfo& task, tasks) {
>         if (!task.limits().empty()) {
>           return true;
>         }
>       }
>     
>       return false;
>     };
>     
>     Option<Map<string, Value::Scalar>> executorLimits;
>     if (limitsAreSet(tasks)) {
>       executorLimitsResult = Map<string, Value::Scalar>(
>           {"cpus": Value::Scalar(),
>            "mem":  Value::Scalar()});
>       foreach (const TaskInfo& _task, tasks) {
>         if (_task.limits().count("cpus")) {
>           executorLimitsResult.at("cpus") += _task.limits().at("cpus");
>         } else {
>           Option<Value::Scalar> taskCpus =
>             Resources(_task.resources()).get<Value::Scalar>("cpus");
>           if (taskCpus.isSome()) {
>             executorLimitsResult.at("cpus") += taskCpus.get();
>           }
>         }
>         
>         if (_task.limits().count("mem")) {
>           executorLimitsResult.at("mem") += _task.limits().at("mem");
>         } else {
>           Option<Value::Scalar> taskMem =
>             Resources(_task.resources()).get<Value::Scalar>("mem");
>           if (taskMem.isSome()) {
>             executorLimitsResult.at("mem") += taskMem.get();
>           }
>         }
>       }
>       
>       executorLimits = executorLimitsResult;
>     }
>     
>     // Launch executor with 'executorLimits'...
>     ```
>     
>     WDYT?
> 
> Greg Mann wrote:
>     I still don't understand the argument for making this a raw map rather than an `Option`.
It seems semantically better to me to have a check for `executorLimits.isSome()` within the
body of `launchExecutor()`, rather than a check for `executorLimits.empty()`.

I agree it is better to have a check for `executorLimits.isSome()` in `launchExecutor()` rather
than a check for `executorLimits.empty()`, however that means we need one more variables (`executorLimits_`
in the code below) in `Slave::__run()`, like:
```
Option<Map> executorLimits;
Map executorLimits_;

if (executorCpuLimit.isSome()) {
  executorLimits_.insert({"cpus", executorCpuLimit.get()});
}

if (executorMemLimit.isSome()) {
  executorLimits_.insert({"mem", executorMemLimit.get()});
}

if (!executorLimits_.empty()) {
 executorLimits = executorLimits_;
}
```

In the above code, we have one more variable and we still need to do `executorLimits_.empty()`.
Do we really want to do that?


- Qian


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


On Feb. 25, 2020, 9:46 a.m., Qian Zhang wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/71858/
> -----------------------------------------------------------
> 
> (Updated Feb. 25, 2020, 9:46 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
> -------
> 
> Set resource limits when launching executor container.
> 
> 
> Diffs
> -----
> 
>   src/slave/slave.hpp 03279db082bdba23dbfeb2d93081a908e609aec2 
>   src/slave/slave.cpp cce275a504effae7a6b71dd333ce8a300d1ce5be 
> 
> 
> Diff: https://reviews.apache.org/r/71858/diff/10/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Qian Zhang
> 
>


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