Re: Review Request 71952: Set resource limits when updating executor container.

[Qian Zhang]

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

(Updated March 16, 2020, 5:12 p.m.)


Review request for mesos, Andrei Budnik and Greg Mann.


Changes
---

Addressed review comments.


Bugs: MESOS-10050
https://issues.apache.org/jira/browse/MESOS-10050


Repository: mesos


Description
---

Set resource limits when updating executor container.


Diffs (updated)
-

  src/slave/slave.hpp 03279db082bdba23dbfeb2d93081a908e609aec2 
  src/slave/slave.cpp a914de4bae5630126ae26b9e7dee8c8784ce1ee0 


Diff: https://reviews.apache.org/r/71952/diff/8/

Changes: https://reviews.apache.org/r/71952/diff/7-8/


Testing
---

sudo make check


Thanks,

Qian Zhang

Re: Review Request 71952: Set resource limits when updating executor container.

[Greg Mann]

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


Fix it, then Ship it!





src/slave/slave.hpp
Lines 782-784 (original), 782-785 (patched)


At this point, I think a comment for this helper would be useful, to 
explain why we have both `taskInfos` and `tasks` arguments. Maybe something 
like:

```
// This function is used to compute limits for executors
// before they are launched as well as when updating
// running executors, so we must accept both `TaskInfo`
// and `Task` types to handle both cases.
```



src/slave/slave.cpp
Line 9989 (original), 10049-10050 (patched)


Let's add a `CHECK_NOTNULL(task);` here.


- Greg Mann


On March 9, 2020, 2:08 p.m., Qian Zhang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/71952/
> ---
> 
> (Updated March 9, 2020, 2:08 p.m.)
> 
> 
> Review request for mesos, Andrei Budnik and Greg Mann.
> 
> 
> Bugs: MESOS-10050
> https://issues.apache.org/jira/browse/MESOS-10050
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Set resource limits when updating executor container.
> 
> 
> Diffs
> -
> 
>   src/slave/slave.hpp 03279db082bdba23dbfeb2d93081a908e609aec2 
>   src/slave/slave.cpp a914de4bae5630126ae26b9e7dee8c8784ce1ee0 
> 
> 
> Diff: https://reviews.apache.org/r/71952/diff/7/
> 
> 
> Testing
> ---
> 
> sudo make check
> 
> 
> Thanks,
> 
> Qian Zhang
> 
>

Re: Review Request 71952: Set resource limits when updating executor container.

[Qian Zhang]

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

(Updated March 9, 2020, 10:08 p.m.)


Review request for mesos, Andrei Budnik and Greg Mann.


Changes
---

Replaced `Executor::resourceLimits` with `Slave::computeExecutorLimits()`.


Bugs: MESOS-10050
https://issues.apache.org/jira/browse/MESOS-10050


Repository: mesos


Description
---

Set resource limits when updating executor container.


Diffs (updated)
-

  src/slave/slave.hpp 03279db082bdba23dbfeb2d93081a908e609aec2 
  src/slave/slave.cpp a914de4bae5630126ae26b9e7dee8c8784ce1ee0 


Diff: https://reviews.apache.org/r/71952/diff/7/

Changes: https://reviews.apache.org/r/71952/diff/6-7/


Testing
---

sudo make check


Thanks,

Qian Zhang

Re: Review Request 71952: Set resource limits when updating executor container.

[Greg Mann]

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




src/common/protobuf_utils.cpp
Lines 398 (patched)


Could you move this to the patch where you add the limits field to the 
`Task` message?



src/slave/slave.cpp
Lines 11198 (patched)


Seems like you could just change this entire function into a helper which 
we use in two places?

```
google::protobuf::Map computeExecutorLimits(
const std::vector& tasks,
const Resources& executorResources,
bool enableCfs);
```


- Greg Mann


On Feb. 26, 2020, 12:25 p.m., Qian Zhang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/71952/
> ---
> 
> (Updated Feb. 26, 2020, 12:25 p.m.)
> 
> 
> Review request for mesos, Andrei Budnik and Greg Mann.
> 
> 
> Bugs: MESOS-10050
> https://issues.apache.org/jira/browse/MESOS-10050
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Set resource limits when updating executor container.
> 
> 
> Diffs
> -
> 
>   src/common/protobuf_utils.cpp 7fe4a44b1e7ded998dffb0490c1d61ced697ebd5 
>   src/slave/slave.hpp 03279db082bdba23dbfeb2d93081a908e609aec2 
>   src/slave/slave.cpp cce275a504effae7a6b71dd333ce8a300d1ce5be 
> 
> 
> Diff: https://reviews.apache.org/r/71952/diff/6/
> 
> 
> Testing
> ---
> 
> sudo make check
> 
> 
> Thanks,
> 
> Qian Zhang
> 
>

Re: Review Request 71952: Set resource limits when updating executor container.

[Greg Mann]

> On Feb. 28, 2020, 5:25 p.m., Greg Mann wrote:
> > src/slave/slave.cpp
> > Lines 11198 (patched)
> > 
> >
> > The similarity of this function body to some of the code in 
> > https://reviews.apache.org/r/71858/ is unfortunate... do you think we 
> > should factor it out into some helper which looks through a list of tasks 
> > and calculates the limits and requests? Maybe something like:
> > 
> > ```
> > struct TaskResourceMetadata
> > {
> >   Resources totalTaskResources;
> >   map> totalLimits;
> >   map totalRequests;
> > };
> > 
> > TaskResourceMetadata computeTaskResourceMetadata(vector tasks)
> > {
> >   TaskResourceMetadata result;
> >   std::vector names({"cpus", "mem"});
> > 
> >   foreach (const TaskInfo& _task, tasks) {
> > result.totalTaskResources += _task.resources();
> > 
> > foreach (const std::string _name, names) {
> >   // Lazily initialize requests and limits with `operator[]`.
> >   if (_task.limits().count(_name)) {
> > setLimit(result.totalLimits[_name], _task.limits().at(_name));
> >   } else {
> > Option taskRequest =
> >   Resources(_task.resources()).get(_name);
> > 
> > if (taskRequest.isSome()) {
> >   result.totalRequests[_name] += taskRequest.get();
> > }
> >   }
> > }
> >   }
> > 
> >   return result;
> > }
> > ```
> > 
> > WDYT? Is it worth it?

See my comment below regarding changing this method into a function helper.


- Greg


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


On Feb. 26, 2020, 12:25 p.m., Qian Zhang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/71952/
> ---
> 
> (Updated Feb. 26, 2020, 12:25 p.m.)
> 
> 
> Review request for mesos, Andrei Budnik and Greg Mann.
> 
> 
> Bugs: MESOS-10050
> https://issues.apache.org/jira/browse/MESOS-10050
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Set resource limits when updating executor container.
> 
> 
> Diffs
> -
> 
>   src/common/protobuf_utils.cpp 7fe4a44b1e7ded998dffb0490c1d61ced697ebd5 
>   src/slave/slave.hpp 03279db082bdba23dbfeb2d93081a908e609aec2 
>   src/slave/slave.cpp cce275a504effae7a6b71dd333ce8a300d1ce5be 
> 
> 
> Diff: https://reviews.apache.org/r/71952/diff/6/
> 
> 
> Testing
> ---
> 
> sudo make check
> 
> 
> Thanks,
> 
> Qian Zhang
> 
>

Re: Review Request 71952: Set resource limits when updating executor container.

[Greg Mann]

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




src/slave/slave.cpp
Lines 11198 (patched)


The similarity of this function body to some of the code in 
https://reviews.apache.org/r/71858/ is unfortunate... do you think we should 
factor it out into some helper which looks through a list of tasks and 
calculates the limits and requests? Maybe something like:

```
struct TaskResourceMetadata
{
  Resources totalTaskResources;
  map> totalLimits;
  map totalRequests;
};

TaskResourceMetadata computeTaskResourceMetadata(vector tasks)
{
  TaskResourceMetadata result;
  std::vector names({"cpus", "mem"});

  foreach (const TaskInfo& _task, tasks) {
result.totalTaskResources += _task.resources();

foreach (const std::string _name, names) {
  // Lazily initialize requests and limits with `operator[]`.
  if (_task.limits().count(_name)) {
setLimit(result.totalLimits[_name], _task.limits().at(_name));
  } else {
Option taskRequest =
  Resources(_task.resources()).get(_name);

if (taskRequest.isSome()) {
  result.totalRequests[_name] += taskRequest.get();
}
  }
}
  }

  return result;
}
```

WDYT? Is it worth it?


- Greg Mann


On Feb. 26, 2020, 12:25 p.m., Qian Zhang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/71952/
> ---
> 
> (Updated Feb. 26, 2020, 12:25 p.m.)
> 
> 
> Review request for mesos, Andrei Budnik and Greg Mann.
> 
> 
> Bugs: MESOS-10050
> https://issues.apache.org/jira/browse/MESOS-10050
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Set resource limits when updating executor container.
> 
> 
> Diffs
> -
> 
>   src/common/protobuf_utils.cpp 7fe4a44b1e7ded998dffb0490c1d61ced697ebd5 
>   src/slave/slave.hpp 03279db082bdba23dbfeb2d93081a908e609aec2 
>   src/slave/slave.cpp cce275a504effae7a6b71dd333ce8a300d1ce5be 
> 
> 
> Diff: https://reviews.apache.org/r/71952/diff/6/
> 
> 
> Testing
> ---
> 
> sudo make check
> 
> 
> Thanks,
> 
> Qian Zhang
> 
>

Re: Review Request 71952: Set resource limits when updating executor container.

[Qian Zhang]

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

(Updated Feb. 26, 2020, 8:25 p.m.)


Review request for mesos, Andrei Budnik and Greg Mann.


Summary (updated)
-

Set resource limits when updating executor container.


Bugs: MESOS-10050
https://issues.apache.org/jira/browse/MESOS-10050


Repository: mesos


Description (updated)
---

Set resource limits when updating executor container.


Diffs (updated)
-

  src/common/protobuf_utils.cpp 7fe4a44b1e7ded998dffb0490c1d61ced697ebd5 
  src/slave/slave.hpp 03279db082bdba23dbfeb2d93081a908e609aec2 
  src/slave/slave.cpp cce275a504effae7a6b71dd333ce8a300d1ce5be 


Diff: https://reviews.apache.org/r/71952/diff/6/

Changes: https://reviews.apache.org/r/71952/diff/5-6/


Testing
---

sudo make check


Thanks,

Qian Zhang