Re: Review Request 41305: MESOS-1718: move getExecutorInfo from slave to master.

2016-02-17 Thread Klaus Ma 


> On Jan. 22, 2016, 5:37 a.m., Joseph Wu wrote:
> > General questions:
> > 
> > - Besides removing the possibility for overcommit, does this change give us 
> > anything else?
> > - What happens if a master sends this modified `TaskInfo` to an agent that 
> > does not have this chain of patches?
> > - Have you considered completely converting a `CommandInfo` to 
> > `ExecutorInfo`?  If we do that, it may be possible to remove all the other 
> > places with special logic for command tasks.
> 
> Klaus Ma wrote:
> * It's more for evicting executor by master, there's several EPICs about 
> sharing resources between frameworks; and all of them need the feature to let 
> resources owner get resouces back: MESOS-4303 (reclaim/preemption in Mesos), 
> "Optimsitic Offer Phase 2", "Borrow/Lend policy on Quota" (draft idea in my 
> mind)
> 
> * Good point! If did not get required info, e.g. `launch_dir`, we should 
> keep the same behaviour as before: let salve trace the executor info.
> 
> * Yes, I'm used to add `taskCommand` into `ExecutorInfo`; but when launch 
> docker executor, it need task info which is different with normal executor. 
> In this patch, I'd like to focus on moving the executor info into master.

@Joseph, any more comments?


- Klaus


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


On Feb. 18, 2016, 7:45 a.m., Klaus Ma wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/41305/
> ---
> 
> (Updated Feb. 18, 2016, 7:45 a.m.)
> 
> 
> Review request for mesos, Ben Mahler and Joseph Wu.
> 
> 
> Bugs: MESOS-1718
> https://issues.apache.org/jira/browse/MESOS-1718
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> MESOS-1718: move getExecutorInfo from slave to master
> 
> 
> Diffs
> -
> 
>   src/master/constants.hpp 2c3299bc1aaa0888f7e47a71965c56ada8ecc21f 
>   src/master/master.hpp 2f2ad2ada508e1923bf995ab124367a3b082b572 
>   src/master/master.cpp e1ca81dab85a7ab1391eca0d6bd995548bf79c22 
>   src/slave/constants.hpp bcbb1401aa8f9f04c4f9256bb4f560e18d8994e0 
>   src/slave/slave.hpp ced835dec797bcc5640422468487a4289a737e38 
>   src/slave/slave.cpp f0be0d5bf4b853952caf06e2e262c2903d79ead3 
> 
> Diff: https://reviews.apache.org/r/41305/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Klaus Ma
> 
>

Re: Review Request 41305: MESOS-1718: move getExecutorInfo from slave to master.

2016-02-17 Thread Klaus Ma 

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

(Updated Feb. 18, 2016, 7:45 a.m.)


Review request for mesos, Ben Mahler and Joseph Wu.


Changes
---

rebase for code conflict


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


Repository: mesos


Description
---

MESOS-1718: move getExecutorInfo from slave to master


Diffs (updated)
-

  src/master/constants.hpp 2c3299bc1aaa0888f7e47a71965c56ada8ecc21f 
  src/master/master.hpp 2f2ad2ada508e1923bf995ab124367a3b082b572 
  src/master/master.cpp e1ca81dab85a7ab1391eca0d6bd995548bf79c22 
  src/slave/constants.hpp bcbb1401aa8f9f04c4f9256bb4f560e18d8994e0 
  src/slave/slave.hpp ced835dec797bcc5640422468487a4289a737e38 
  src/slave/slave.cpp f0be0d5bf4b853952caf06e2e262c2903d79ead3 

Diff: https://reviews.apache.org/r/41305/diff/


Testing
---


Thanks,

Klaus Ma

Re: Review Request 41305: MESOS-1718: move getExecutorInfo from slave to master.

2016-02-12 Thread Klaus Ma 


> On Jan. 22, 2016, 5:37 a.m., Joseph Wu wrote:
> > src/master/master.cpp, lines 3770-3779
> > 
> >
> > It should still be invalid to supply both a CommandInfo and 
> > ExecutorInfo in the same TaskInfo.

The check was removed at https://reviews.apache.org/r/41306


- Klaus


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


On Jan. 19, 2016, 10:55 a.m., Klaus Ma wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/41305/
> ---
> 
> (Updated Jan. 19, 2016, 10:55 a.m.)
> 
> 
> Review request for mesos, Ben Mahler, Guangya Liu, Artem Harutyunyan, Ian 
> Downes, Joris Van Remoortere, Joseph Wu, and Vinod Kone.
> 
> 
> Bugs: MESOS-1718
> https://issues.apache.org/jira/browse/MESOS-1718
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> MESOS-1718: move getExecutorInfo from slave to master
> 
> 
> Diffs
> -
> 
>   src/master/constants.hpp 2c3299bc1aaa0888f7e47a71965c56ada8ecc21f 
>   src/master/master.hpp 3a7e18232323a1c051bcc97915484b1195fffe58 
>   src/master/master.cpp 9ee56277c8a472be9e683d5db505becfb5f7c422 
>   src/slave/constants.hpp bcbb1401aa8f9f04c4f9256bb4f560e18d8994e0 
>   src/slave/slave.hpp b7586ce42bfac9d9885a3eb8d82deb94680c236c 
>   src/slave/slave.cpp bb501810d7bb1261ebbbdd147c49948e5a2f8665 
> 
> Diff: https://reviews.apache.org/r/41305/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Klaus Ma
> 
>

Re: Review Request 41305: MESOS-1718: move getExecutorInfo from slave to master.

2016-02-12 Thread Klaus Ma 


> On Jan. 22, 2016, 5:37 a.m., Joseph Wu wrote:
> > src/master/master.cpp, line 2860
> > 
> >
> > `launcher_dir` is an optional field.
> > 
> > Same for the other fields you added (`sandbox_dir`, `switch_user`, 
> > `executor_rootfs).

@Joseph, do you mean we need to check whether the value is set? I think it's 
also related with compatibility with old slave, we should also handle it when 
old slave did not report `slaveInfo`, right?


- Klaus


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


On Feb. 12, 2016, 11:02 p.m., Klaus Ma wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/41305/
> ---
> 
> (Updated Feb. 12, 2016, 11:02 p.m.)
> 
> 
> Review request for mesos, Ben Mahler and Joseph Wu.
> 
> 
> Bugs: MESOS-1718
> https://issues.apache.org/jira/browse/MESOS-1718
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> MESOS-1718: move getExecutorInfo from slave to master
> 
> 
> Diffs
> -
> 
>   src/master/constants.hpp 2c3299bc1aaa0888f7e47a71965c56ada8ecc21f 
>   src/master/master.hpp 3a7e18232323a1c051bcc97915484b1195fffe58 
>   src/master/master.cpp 9ee56277c8a472be9e683d5db505becfb5f7c422 
>   src/slave/constants.hpp bcbb1401aa8f9f04c4f9256bb4f560e18d8994e0 
>   src/slave/slave.hpp b7586ce42bfac9d9885a3eb8d82deb94680c236c 
>   src/slave/slave.cpp bb501810d7bb1261ebbbdd147c49948e5a2f8665 
> 
> Diff: https://reviews.apache.org/r/41305/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Klaus Ma
> 
>

Re: Review Request 41305: MESOS-1718: move getExecutorInfo from slave to master.

2016-01-21 Thread Joseph Wu 

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


General questions:

- Besides removing the possibility for overcommit, does this change give us 
anything else?
- What happens if a master sends this modified `TaskInfo` to an agent that does 
not have this chain of patches?
- Have you considered completely converting a `CommandInfo` to `ExecutorInfo`?  
If we do that, it may be possible to remove all the other places with special 
logic for command tasks.


src/master/master.cpp (line 2860)


`launcher_dir` is an optional field.

Same for the other fields you added (`sandbox_dir`, `switch_user`, 
`executor_rootfs).



src/master/master.cpp (lines 3770 - 3779)


It should still be invalid to supply both a CommandInfo and ExecutorInfo in 
the same TaskInfo.


- Joseph Wu


On Jan. 18, 2016, 6:55 p.m., Klaus Ma wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/41305/
> ---
> 
> (Updated Jan. 18, 2016, 6:55 p.m.)
> 
> 
> Review request for mesos, Ben Mahler, Guangya Liu, Artem Harutyunyan, Ian 
> Downes, Joris Van Remoortere, Joseph Wu, and Vinod Kone.
> 
> 
> Bugs: MESOS-1718
> https://issues.apache.org/jira/browse/MESOS-1718
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> MESOS-1718: move getExecutorInfo from slave to master
> 
> 
> Diffs
> -
> 
>   src/master/constants.hpp 2c3299bc1aaa0888f7e47a71965c56ada8ecc21f 
>   src/master/master.hpp 3a7e18232323a1c051bcc97915484b1195fffe58 
>   src/master/master.cpp 9ee56277c8a472be9e683d5db505becfb5f7c422 
>   src/slave/constants.hpp bcbb1401aa8f9f04c4f9256bb4f560e18d8994e0 
>   src/slave/slave.hpp b7586ce42bfac9d9885a3eb8d82deb94680c236c 
>   src/slave/slave.cpp bb501810d7bb1261ebbbdd147c49948e5a2f8665 
> 
> Diff: https://reviews.apache.org/r/41305/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Klaus Ma
> 
>

Re: Review Request 41305: MESOS-1718: move getExecutorInfo from slave to master.

2016-01-18 Thread Klaus Ma 

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

(Updated Jan. 19, 2016, 10:55 a.m.)


Review request for mesos, Ben Mahler, Guangya Liu, Artem Harutyunyan, Ian 
Downes, Joris Van Remoortere, Joseph Wu, and Vinod Kone.


Changes
---

rebase for apply conflict


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


Repository: mesos


Description
---

MESOS-1718: move getExecutorInfo from slave to master


Diffs (updated)
-

  src/master/constants.hpp 2c3299bc1aaa0888f7e47a71965c56ada8ecc21f 
  src/master/master.hpp 3a7e18232323a1c051bcc97915484b1195fffe58 
  src/master/master.cpp 9ee56277c8a472be9e683d5db505becfb5f7c422 
  src/slave/constants.hpp bcbb1401aa8f9f04c4f9256bb4f560e18d8994e0 
  src/slave/slave.hpp b7586ce42bfac9d9885a3eb8d82deb94680c236c 
  src/slave/slave.cpp bb501810d7bb1261ebbbdd147c49948e5a2f8665 

Diff: https://reviews.apache.org/r/41305/diff/


Testing
---


Thanks,

Klaus Ma

Re: Review Request 41305: MESOS-1718: move getExecutorInfo from slave to master.

2016-01-15 Thread Klaus Ma 

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

(Updated Jan. 16, 2016, 1:46 p.m.)


Review request for mesos, Ben Mahler, Guangya Liu, Artem Harutyunyan, Ian 
Downes, Joris Van Remoortere, Joseph Wu, and Vinod Kone.


Changes
---

Add period to Summary.


Summary (updated)
-

MESOS-1718: move getExecutorInfo from slave to master.


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


Repository: mesos


Description
---

MESOS-1718: move getExecutorInfo from slave to master


Diffs
-

  src/master/constants.hpp ebab341e58035d4b579828add752c1ee37efeb95 
  src/master/master.hpp f02d165874fa8023675e545793de699aeecae29b 
  src/master/master.cpp 863a11c82d322f56db1ccf25b73a41304a46 
  src/slave/constants.hpp bcbb1401aa8f9f04c4f9256bb4f560e18d8994e0 
  src/slave/slave.hpp b7586ce42bfac9d9885a3eb8d82deb94680c236c 
  src/slave/slave.cpp 759c8d5b1bfb5ad723aa423e1487998ed62bbc3a 

Diff: https://reviews.apache.org/r/41305/diff/


Testing
---


Thanks,

Klaus Ma

Re: Review Request 41305: MESOS-1718: move getExecutorInfo from slave to master

2016-01-14 Thread Klaus Ma 


> On Dec. 14, 2015, 2:31 p.m., Qian Zhang wrote:
> > src/slave/slave.cpp, line 1370
> > 
> >
> > Before trying to get task's executor via `task.executor()`, suggest to 
> > add a CHECK to ensure the task always has an executor.

It's not necessary, executor is set in master.


- Klaus


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


On Jan. 15, 2016, 1:17 p.m., Klaus Ma wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/41305/
> ---
> 
> (Updated Jan. 15, 2016, 1:17 p.m.)
> 
> 
> Review request for mesos, Ben Mahler, Guangya Liu, Artem Harutyunyan, Ian 
> Downes, Joris Van Remoortere, Joseph Wu, and Vinod Kone.
> 
> 
> Bugs: MESOS-1718
> https://issues.apache.org/jira/browse/MESOS-1718
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> MESOS-1718: move getExecutorInfo from slave to master
> 
> 
> Diffs
> -
> 
>   src/master/constants.hpp ebab341e58035d4b579828add752c1ee37efeb95 
>   src/master/master.hpp f02d165874fa8023675e545793de699aeecae29b 
>   src/master/master.cpp 863a11c82d322f56db1ccf25b73a41304a46 
>   src/slave/constants.hpp bcbb1401aa8f9f04c4f9256bb4f560e18d8994e0 
>   src/slave/slave.hpp b7586ce42bfac9d9885a3eb8d82deb94680c236c 
>   src/slave/slave.cpp 759c8d5b1bfb5ad723aa423e1487998ed62bbc3a 
> 
> Diff: https://reviews.apache.org/r/41305/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Klaus Ma
> 
>

Re: Review Request 41305: MESOS-1718: move getExecutorInfo from slave to master

2016-01-14 Thread Klaus Ma 

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

(Updated Jan. 15, 2016, 1:17 p.m.)


Review request for mesos, Ben Mahler, Guangya Liu, Artem Harutyunyan, Ian 
Downes, Joris Van Remoortere, Joseph Wu, and Vinod Kone.


Changes
---

rebase


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


Repository: mesos


Description
---

MESOS-1718: move getExecutorInfo from slave to master


Diffs (updated)
-

  src/master/constants.hpp ebab341e58035d4b579828add752c1ee37efeb95 
  src/master/master.hpp f02d165874fa8023675e545793de699aeecae29b 
  src/master/master.cpp 863a11c82d322f56db1ccf25b73a41304a46 
  src/slave/constants.hpp bcbb1401aa8f9f04c4f9256bb4f560e18d8994e0 
  src/slave/slave.hpp b7586ce42bfac9d9885a3eb8d82deb94680c236c 
  src/slave/slave.cpp 759c8d5b1bfb5ad723aa423e1487998ed62bbc3a 

Diff: https://reviews.apache.org/r/41305/diff/


Testing
---


Thanks,

Klaus Ma

Re: Review Request 41305: MESOS-1718: move getExecutorInfo from slave to master

2015-12-14 Thread Klaus Ma 


> On Dec. 14, 2015, 10:47 p.m., Guangya Liu wrote:
> > src/master/master.cpp, lines 2829-2845
> > 
> >
> > How does this can resove overcommit issue? I think that the issue is 
> > still there? Another is that can you please add some comments here to 
> > explain the overcommit issue here?

The executor resources are cut from task's resource when add it into 
`TaskInfo`. Sure, I'll add some comments here.


- Klaus


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


On Dec. 12, 2015, 5:54 p.m., Klaus Ma wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/41305/
> ---
> 
> (Updated Dec. 12, 2015, 5:54 p.m.)
> 
> 
> Review request for mesos, Ben Mahler, Guangya Liu, Artem Harutyunyan, Ian 
> Downes, Joris Van Remoortere, Joseph Wu, and Vinod Kone.
> 
> 
> Bugs: MESOS-1718
> https://issues.apache.org/jira/browse/MESOS-1718
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> MESOS-1718: move getExecutorInfo from slave to master
> 
> 
> Diffs
> -
> 
>   src/master/constants.hpp cc38dfcf57ecbc8555379084acbf2bfa4b3fc759 
>   src/master/master.hpp 9aa548aa6e159046c94e4ec96f631ea8b3bfd5d8 
>   src/master/master.cpp 0d1482279c68f2a4a27dabaf28774769a5d515c4 
>   src/slave/constants.hpp bcbb1401aa8f9f04c4f9256bb4f560e18d8994e0 
>   src/slave/slave.hpp b7586ce42bfac9d9885a3eb8d82deb94680c236c 
>   src/slave/slave.cpp 9bd86e122c070cd072a54d4de8097f844bd95bb0 
> 
> Diff: https://reviews.apache.org/r/41305/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Klaus Ma
> 
>

Re: Review Request 41305: MESOS-1718: move getExecutorInfo from slave to master

2015-12-14 Thread Klaus Ma 


> On Dec. 14, 2015, 2:31 p.m., Qian Zhang wrote:
> > src/master/master.cpp, line 3556
> > 
> >
> > Just wondering why we want to do `task.resources() - 
> > executor.resources())` here? I think in `validateResourceUsage()`, we will 
> > validate whether the task's resources + executor's resources can be 
> > contained by the offered resources, if here we substract executor's 
> > resources from task's resources, then we will only validate whether task's 
> > resources can be contained by the offered resources in 
> > `validateResourceUsage()` which seems not correct to me.

I do not want to overcommit the command line tasks; for example, if there're 
only 2 CPU, how may tasks (~1 CPU per task) should framework run?


- Klaus


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


On Dec. 12, 2015, 5:54 p.m., Klaus Ma wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/41305/
> ---
> 
> (Updated Dec. 12, 2015, 5:54 p.m.)
> 
> 
> Review request for mesos, Ben Mahler, Guangya Liu, Artem Harutyunyan, Ian 
> Downes, Joris Van Remoortere, Joseph Wu, and Vinod Kone.
> 
> 
> Bugs: MESOS-1718
> https://issues.apache.org/jira/browse/MESOS-1718
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> MESOS-1718: move getExecutorInfo from slave to master
> 
> 
> Diffs
> -
> 
>   src/master/constants.hpp cc38dfcf57ecbc8555379084acbf2bfa4b3fc759 
>   src/master/master.hpp 9aa548aa6e159046c94e4ec96f631ea8b3bfd5d8 
>   src/master/master.cpp 0d1482279c68f2a4a27dabaf28774769a5d515c4 
>   src/slave/constants.hpp bcbb1401aa8f9f04c4f9256bb4f560e18d8994e0 
>   src/slave/slave.hpp b7586ce42bfac9d9885a3eb8d82deb94680c236c 
>   src/slave/slave.cpp 9bd86e122c070cd072a54d4de8097f844bd95bb0 
> 
> Diff: https://reviews.apache.org/r/41305/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Klaus Ma
> 
>

Re: Review Request 41305: MESOS-1718: move getExecutorInfo from slave to master

2015-12-14 Thread Klaus Ma 


> On Dec. 14, 2015, 2:31 p.m., Qian Zhang wrote:
> > src/master/master.cpp, line 2830
> > 
> >
> > So if task's resources has no cpu, then we will not add cpu to 
> > executor's resources, right? But I think we should always add 
> > DEFAULT_EXECUTOR_CPUS to executor's resources in this case.

The purpose of this code is not to overcommit resources; for example, only CPU 
or memory in cluster, master can not assign more resources to it. It may 
exhaust slave's resource if lots of executor in slave.


- Klaus


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


On Dec. 12, 2015, 5:54 p.m., Klaus Ma wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/41305/
> ---
> 
> (Updated Dec. 12, 2015, 5:54 p.m.)
> 
> 
> Review request for mesos, Ben Mahler, Guangya Liu, Artem Harutyunyan, Ian 
> Downes, Joris Van Remoortere, Joseph Wu, and Vinod Kone.
> 
> 
> Bugs: MESOS-1718
> https://issues.apache.org/jira/browse/MESOS-1718
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> MESOS-1718: move getExecutorInfo from slave to master
> 
> 
> Diffs
> -
> 
>   src/master/constants.hpp cc38dfcf57ecbc8555379084acbf2bfa4b3fc759 
>   src/master/master.hpp 9aa548aa6e159046c94e4ec96f631ea8b3bfd5d8 
>   src/master/master.cpp 0d1482279c68f2a4a27dabaf28774769a5d515c4 
>   src/slave/constants.hpp bcbb1401aa8f9f04c4f9256bb4f560e18d8994e0 
>   src/slave/slave.hpp b7586ce42bfac9d9885a3eb8d82deb94680c236c 
>   src/slave/slave.cpp 9bd86e122c070cd072a54d4de8097f844bd95bb0 
> 
> Diff: https://reviews.apache.org/r/41305/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Klaus Ma
> 
>

Re: Review Request 41305: MESOS-1718: move getExecutorInfo from slave to master

2015-12-14 Thread Klaus Ma 


> On Dec. 14, 2015, 2:31 p.m., Qian Zhang wrote:
> > src/master/master.cpp, line 2830
> > 
> >
> > So if task's resources has no cpu, then we will not add cpu to 
> > executor's resources, right? But I think we should always add 
> > DEFAULT_EXECUTOR_CPUS to executor's resources in this case.
> 
> Klaus Ma wrote:
> The purpose of this code is not to overcommit resources; for example, 
> only CPU or memory in cluster, master can not assign more resources to it. It 
> may exhaust slave's resource if lots of executor in slave.
> 
> Qian Zhang wrote:
> So that means, it is possible to launch a command executor with no cpu 
> and no memory? But when a command executor is running in an agent, it will 
> indeed consume cpu and memory in that agent, but those cpu and memory will 
> not be tracked by allocator for that agent with the code you wrote here, is 
> that a resource leak in that agent? I think this will actually cause the 
> resource overcommit in the agent.

There's a JIRA (MESOS-1807) on executor's resources; will publish the behavior 
change to dev@ and add validation in MESOS-1807.


- Klaus


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


On Dec. 12, 2015, 5:54 p.m., Klaus Ma wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/41305/
> ---
> 
> (Updated Dec. 12, 2015, 5:54 p.m.)
> 
> 
> Review request for mesos, Ben Mahler, Guangya Liu, Artem Harutyunyan, Ian 
> Downes, Joris Van Remoortere, Joseph Wu, and Vinod Kone.
> 
> 
> Bugs: MESOS-1718
> https://issues.apache.org/jira/browse/MESOS-1718
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> MESOS-1718: move getExecutorInfo from slave to master
> 
> 
> Diffs
> -
> 
>   src/master/constants.hpp cc38dfcf57ecbc8555379084acbf2bfa4b3fc759 
>   src/master/master.hpp 9aa548aa6e159046c94e4ec96f631ea8b3bfd5d8 
>   src/master/master.cpp 0d1482279c68f2a4a27dabaf28774769a5d515c4 
>   src/slave/constants.hpp bcbb1401aa8f9f04c4f9256bb4f560e18d8994e0 
>   src/slave/slave.hpp b7586ce42bfac9d9885a3eb8d82deb94680c236c 
>   src/slave/slave.cpp 9bd86e122c070cd072a54d4de8097f844bd95bb0 
> 
> Diff: https://reviews.apache.org/r/41305/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Klaus Ma
> 
>

Re: Review Request 41305: MESOS-1718: move getExecutorInfo from slave to master

2015-12-14 Thread Qian Zhang 


> On Dec. 14, 2015, 2:31 p.m., Qian Zhang wrote:
> > src/master/master.cpp, line 2830
> > 
> >
> > So if task's resources has no cpu, then we will not add cpu to 
> > executor's resources, right? But I think we should always add 
> > DEFAULT_EXECUTOR_CPUS to executor's resources in this case.
> 
> Klaus Ma wrote:
> The purpose of this code is not to overcommit resources; for example, 
> only CPU or memory in cluster, master can not assign more resources to it. It 
> may exhaust slave's resource if lots of executor in slave.

So that means, it is possible to launch a command executor with no cpu and no 
memory? But when a command executor is running in an agent, it will indeed 
consume cpu and memory in that agent, but those cpu and memory will not be 
tracked by allocator for that agent with the code you wrote here, is that a 
resource leak in that agent? I think this will actually cause the resource 
overcommit in the agent.


> On Dec. 14, 2015, 2:31 p.m., Qian Zhang wrote:
> > src/master/master.cpp, line 3556
> > 
> >
> > Just wondering why we want to do `task.resources() - 
> > executor.resources())` here? I think in `validateResourceUsage()`, we will 
> > validate whether the task's resources + executor's resources can be 
> > contained by the offered resources, if here we substract executor's 
> > resources from task's resources, then we will only validate whether task's 
> > resources can be contained by the offered resources in 
> > `validateResourceUsage()` which seems not correct to me.
> 
> Klaus Ma wrote:
> I do not want to overcommit the command line tasks; for example, if 
> there're only 2 CPU, how may tasks (~1 CPU per task) should framework run?

Understand your concern. But what about framework launches a small task (e.g., 
0.1 cpu and 32MB memory)? So if we substract executor's resources from task's 
resources here, that means we will launch a task with empty resources?


- Qian


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


On Dec. 12, 2015, 5:54 p.m., Klaus Ma wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/41305/
> ---
> 
> (Updated Dec. 12, 2015, 5:54 p.m.)
> 
> 
> Review request for mesos, Ben Mahler, Guangya Liu, Artem Harutyunyan, Ian 
> Downes, Joris Van Remoortere, Joseph Wu, and Vinod Kone.
> 
> 
> Bugs: MESOS-1718
> https://issues.apache.org/jira/browse/MESOS-1718
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> MESOS-1718: move getExecutorInfo from slave to master
> 
> 
> Diffs
> -
> 
>   src/master/constants.hpp cc38dfcf57ecbc8555379084acbf2bfa4b3fc759 
>   src/master/master.hpp 9aa548aa6e159046c94e4ec96f631ea8b3bfd5d8 
>   src/master/master.cpp 0d1482279c68f2a4a27dabaf28774769a5d515c4 
>   src/slave/constants.hpp bcbb1401aa8f9f04c4f9256bb4f560e18d8994e0 
>   src/slave/slave.hpp b7586ce42bfac9d9885a3eb8d82deb94680c236c 
>   src/slave/slave.cpp 9bd86e122c070cd072a54d4de8097f844bd95bb0 
> 
> Diff: https://reviews.apache.org/r/41305/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Klaus Ma
> 
>

Re: Review Request 41305: MESOS-1718: move getExecutorInfo from slave to master

2015-12-14 Thread Guangya Liu 

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



src/master/master.cpp (lines 2829 - 2845)


How does this can resove overcommit issue? I think that the issue is still 
there? Another is that can you please add some comments here to explain the 
overcommit issue here?


- Guangya Liu


On Dec. 12, 2015, 9:54 a.m., Klaus Ma wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/41305/
> ---
> 
> (Updated Dec. 12, 2015, 9:54 a.m.)
> 
> 
> Review request for mesos, Ben Mahler, Guangya Liu, Artem Harutyunyan, Ian 
> Downes, Joris Van Remoortere, Joseph Wu, and Vinod Kone.
> 
> 
> Bugs: MESOS-1718
> https://issues.apache.org/jira/browse/MESOS-1718
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> MESOS-1718: move getExecutorInfo from slave to master
> 
> 
> Diffs
> -
> 
>   src/master/constants.hpp cc38dfcf57ecbc8555379084acbf2bfa4b3fc759 
>   src/master/master.hpp 9aa548aa6e159046c94e4ec96f631ea8b3bfd5d8 
>   src/master/master.cpp 0d1482279c68f2a4a27dabaf28774769a5d515c4 
>   src/slave/constants.hpp bcbb1401aa8f9f04c4f9256bb4f560e18d8994e0 
>   src/slave/slave.hpp b7586ce42bfac9d9885a3eb8d82deb94680c236c 
>   src/slave/slave.cpp 9bd86e122c070cd072a54d4de8097f844bd95bb0 
> 
> Diff: https://reviews.apache.org/r/41305/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Klaus Ma
> 
>

Re: Review Request 41305: MESOS-1718: move getExecutorInfo from slave to master

2015-12-13 Thread Qian Zhang 

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



src/master/master.cpp (line 2830)


So if task's resources has no cpu, then we will not add cpu to executor's 
resources, right? But I think we should always add DEFAULT_EXECUTOR_CPUS to 
executor's resources in this case.



src/master/master.cpp (line 2838)


So if task's resources has no mem, then we will not add mem to executor's 
resources, right? But I think we should always add DEFAULT_EXECUTOR_MEM to 
executor's resources in this case.



src/master/master.cpp (line 3556)


Just wondering why we want to do `task.resources() - executor.resources())` 
here? I think in `validateResourceUsage()`, we will validate whether the task's 
resources + executor's resources can be contained by the offered resources, if 
here we substract executor's resources from task's resources, then we will only 
validate whether task's resources can be contained by the offered resources in 
`validateResourceUsage()` which seems not correct to me.



src/slave/slave.cpp (line 1370)


Before trying to get task's executor via `task.executor()`, suggest to add 
a CHECK to ensure the task always has an executor.


- Qian Zhang


On Dec. 12, 2015, 5:54 p.m., Klaus Ma wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/41305/
> ---
> 
> (Updated Dec. 12, 2015, 5:54 p.m.)
> 
> 
> Review request for mesos, Ben Mahler, Guangya Liu, Artem Harutyunyan, Ian 
> Downes, Joris Van Remoortere, Joseph Wu, and Vinod Kone.
> 
> 
> Bugs: MESOS-1718
> https://issues.apache.org/jira/browse/MESOS-1718
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> MESOS-1718: move getExecutorInfo from slave to master
> 
> 
> Diffs
> -
> 
>   src/master/constants.hpp cc38dfcf57ecbc8555379084acbf2bfa4b3fc759 
>   src/master/master.hpp 9aa548aa6e159046c94e4ec96f631ea8b3bfd5d8 
>   src/master/master.cpp 0d1482279c68f2a4a27dabaf28774769a5d515c4 
>   src/slave/constants.hpp bcbb1401aa8f9f04c4f9256bb4f560e18d8994e0 
>   src/slave/slave.hpp b7586ce42bfac9d9885a3eb8d82deb94680c236c 
>   src/slave/slave.cpp 9bd86e122c070cd072a54d4de8097f844bd95bb0 
> 
> Diff: https://reviews.apache.org/r/41305/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Klaus Ma
> 
>