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