Re: Review Request 52470: Filled missing executor info in tasks when `LAUNCH_GROUP`.

2016-10-14 Thread Vinod Kone 

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


Ship it!




Ship It!

- Vinod Kone


On Oct. 13, 2016, 6:36 a.m., haosdent huang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52470/
> ---
> 
> (Updated Oct. 13, 2016, 6:36 a.m.)
> 
> 
> Review request for mesos, Anand Mazumdar and Vinod Kone.
> 
> 
> Bugs: MESOS-6283
> https://issues.apache.org/jira/browse/MESOS-6283
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This fixed the navigate error in Web UI because Web UI uses the
> executor id of the task to search the corresponding sandbox directory.
> Web UI uses the task id as the executor id if the executor id of the
> task is empty when searching the sandbox directory. It works fine when
> tasks are launched by `CommandExecutor` because the executor id of the
> task is equal to the task id in this case. However, when tasks are
> launched by `DefaultExecutor`, the executor id of the task is defined
> in the framework side and may different with the task id. So we need to
> fill the `ExecutorInfo` of the `TaskInfo` when `LAUNCH_GROUP` to avoid
> the Web UI uses incorrect executor id to search sandbox directory.
> 
> 
> Diffs
> -
> 
>   include/mesos/mesos.proto 05988d4a5cc387f6eafe6453170bcc8764ee1247 
>   include/mesos/v1/mesos.proto 08a536cf345663f81d03e53625c640f946ce5079 
>   src/master/master.hpp 43518b9bf1bfaa54e26acc7f2e70c4161c667a84 
>   src/master/master.cpp 7ef898781ce5c2349ffeaa3ce43e68dede19c852 
>   src/master/validation.cpp e5da3c9bf0a1042b42522f1ab74ce798fbb1738d 
>   src/tests/default_executor_tests.cpp 
> 9e0fd678a7f5e9c2288fbb11a60cf6f339efa24f 
>   src/tests/master_validation_tests.cpp 
> 99e350e0587e73e9ee25ef20dd369cd146bd446a 
> 
> Diff: https://reviews.apache.org/r/52470/diff/
> 
> 
> Testing
> ---
> 
> Add new test case `DefaultExecutorTest.ROOT_TaskUsesExecutor`.
> 
> 
> Thanks,
> 
> haosdent huang
> 
>

Re: Review Request 52470: Filled missing executor info in tasks when `LAUNCH_GROUP`.

2016-10-13 Thread haosdent huang 

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

(Updated Oct. 13, 2016, 6:36 a.m.)


Review request for mesos, Anand Mazumdar and Vinod Kone.


Changes
---

Address @vinodkone's comments.


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


Repository: mesos


Description
---

This fixed the navigate error in Web UI because Web UI uses the
executor id of the task to search the corresponding sandbox directory.
Web UI uses the task id as the executor id if the executor id of the
task is empty when searching the sandbox directory. It works fine when
tasks are launched by `CommandExecutor` because the executor id of the
task is equal to the task id in this case. However, when tasks are
launched by `DefaultExecutor`, the executor id of the task is defined
in the framework side and may different with the task id. So we need to
fill the `ExecutorInfo` of the `TaskInfo` when `LAUNCH_GROUP` to avoid
the Web UI uses incorrect executor id to search sandbox directory.


Diffs (updated)
-

  include/mesos/mesos.proto 05988d4a5cc387f6eafe6453170bcc8764ee1247 
  include/mesos/v1/mesos.proto 08a536cf345663f81d03e53625c640f946ce5079 
  src/master/master.hpp 43518b9bf1bfaa54e26acc7f2e70c4161c667a84 
  src/master/master.cpp 7ef898781ce5c2349ffeaa3ce43e68dede19c852 
  src/master/validation.cpp e5da3c9bf0a1042b42522f1ab74ce798fbb1738d 
  src/tests/default_executor_tests.cpp 9e0fd678a7f5e9c2288fbb11a60cf6f339efa24f 
  src/tests/master_validation_tests.cpp 
99e350e0587e73e9ee25ef20dd369cd146bd446a 

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


Testing
---

Add new test case `DefaultExecutorTest.ROOT_TaskUsesExecutor`.


Thanks,

haosdent huang

Re: Review Request 52470: Filled missing executor info in tasks when `LAUNCH_GROUP`.

2016-10-12 Thread Vinod Kone 

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




include/mesos/mesos.proto 


2) `TaskInfo.executor` need not be set. If set, it should match 
`LaunchGroup.executor`.


- Vinod Kone


On Oct. 12, 2016, 5:18 p.m., haosdent huang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52470/
> ---
> 
> (Updated Oct. 12, 2016, 5:18 p.m.)
> 
> 
> Review request for mesos, Anand Mazumdar and Vinod Kone.
> 
> 
> Bugs: MESOS-6283
> https://issues.apache.org/jira/browse/MESOS-6283
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This fixed the navigate error in Web UI because Web UI uses the
> executor id of the task to search the corresponding sandbox directory.
> Web UI uses the task id as the executor id if the executor id of the
> task is empty when searching the sandbox directory. It works fine when
> tasks are launched by `CommandExecutor` because the executor id of the
> task is equal to the task id in this case. However, when tasks are
> launched by `DefaultExecutor`, the executor id of the task is defined
> in the framework side and may different with the task id. So we need to
> fill the `ExecutorInfo` of the `TaskInfo` when `LAUNCH_GROUP` to avoid
> the Web UI uses incorrect executor id to search sandbox directory.
> 
> 
> Diffs
> -
> 
>   include/mesos/mesos.proto 7d0201ead69c816b9d51f1a41cd64a8f922b069d 
>   include/mesos/v1/mesos.proto 301d1aedca7ac0729229ba9b05e5bf2b79f286a5 
>   src/master/master.hpp 43518b9bf1bfaa54e26acc7f2e70c4161c667a84 
>   src/master/master.cpp ad8993ae43e2508a3777f4062acaca1177ac77f2 
>   src/master/validation.cpp e5da3c9bf0a1042b42522f1ab74ce798fbb1738d 
>   src/tests/default_executor_tests.cpp 
> 9e0fd678a7f5e9c2288fbb11a60cf6f339efa24f 
>   src/tests/master_validation_tests.cpp 
> 99e350e0587e73e9ee25ef20dd369cd146bd446a 
> 
> Diff: https://reviews.apache.org/r/52470/diff/
> 
> 
> Testing
> ---
> 
> Add new test case `DefaultExecutorTest.ROOT_TaskUsesExecutor`.
> 
> 
> Thanks,
> 
> haosdent huang
> 
>

Re: Review Request 52470: Filled missing executor info in tasks when `LAUNCH_GROUP`.

2016-10-12 Thread Vinod Kone 

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




src/master/validation.cpp (line 1115)


need to add a validation here to ensure taskInfo.executor() is same as 
executor for all tasks in the task group.



src/tests/master_validation_tests.cpp 


Keep this test and modify it to make sure the operation is rejected 
ifExecutorinfo inside TaskInfo doesn't match the top level Executorinfo.


- Vinod Kone


On Oct. 12, 2016, 5:18 p.m., haosdent huang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52470/
> ---
> 
> (Updated Oct. 12, 2016, 5:18 p.m.)
> 
> 
> Review request for mesos, Anand Mazumdar and Vinod Kone.
> 
> 
> Bugs: MESOS-6283
> https://issues.apache.org/jira/browse/MESOS-6283
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This fixed the navigate error in Web UI because Web UI uses the
> executor id of the task to search the corresponding sandbox directory.
> Web UI uses the task id as the executor id if the executor id of the
> task is empty when searching the sandbox directory. It works fine when
> tasks are launched by `CommandExecutor` because the executor id of the
> task is equal to the task id in this case. However, when tasks are
> launched by `DefaultExecutor`, the executor id of the task is defined
> in the framework side and may different with the task id. So we need to
> fill the `ExecutorInfo` of the `TaskInfo` when `LAUNCH_GROUP` to avoid
> the Web UI uses incorrect executor id to search sandbox directory.
> 
> 
> Diffs
> -
> 
>   include/mesos/mesos.proto 7d0201ead69c816b9d51f1a41cd64a8f922b069d 
>   include/mesos/v1/mesos.proto 301d1aedca7ac0729229ba9b05e5bf2b79f286a5 
>   src/master/master.hpp 43518b9bf1bfaa54e26acc7f2e70c4161c667a84 
>   src/master/master.cpp ad8993ae43e2508a3777f4062acaca1177ac77f2 
>   src/master/validation.cpp e5da3c9bf0a1042b42522f1ab74ce798fbb1738d 
>   src/tests/default_executor_tests.cpp 
> 9e0fd678a7f5e9c2288fbb11a60cf6f339efa24f 
>   src/tests/master_validation_tests.cpp 
> 99e350e0587e73e9ee25ef20dd369cd146bd446a 
> 
> Diff: https://reviews.apache.org/r/52470/diff/
> 
> 
> Testing
> ---
> 
> Add new test case `DefaultExecutorTest.ROOT_TaskUsesExecutor`.
> 
> 
> Thanks,
> 
> haosdent huang
> 
>

Re: Review Request 52470: Filled missing executor info in tasks when `LAUNCH_GROUP`.

2016-10-12 Thread haosdent huang 

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

(Updated Oct. 12, 2016, 5:18 p.m.)


Review request for mesos, Anand Mazumdar and Vinod Kone.


Changes
---

Fix failed test cases.


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


Repository: mesos


Description
---

This fixed the navigate error in Web UI because Web UI uses the
executor id of the task to search the corresponding sandbox directory.
Web UI uses the task id as the executor id if the executor id of the
task is empty when searching the sandbox directory. It works fine when
tasks are launched by `CommandExecutor` because the executor id of the
task is equal to the task id in this case. However, when tasks are
launched by `DefaultExecutor`, the executor id of the task is defined
in the framework side and may different with the task id. So we need to
fill the `ExecutorInfo` of the `TaskInfo` when `LAUNCH_GROUP` to avoid
the Web UI uses incorrect executor id to search sandbox directory.


Diffs
-

  include/mesos/mesos.proto 7d0201ead69c816b9d51f1a41cd64a8f922b069d 
  include/mesos/v1/mesos.proto 301d1aedca7ac0729229ba9b05e5bf2b79f286a5 
  src/master/master.hpp 43518b9bf1bfaa54e26acc7f2e70c4161c667a84 
  src/master/master.cpp ad8993ae43e2508a3777f4062acaca1177ac77f2 
  src/master/validation.cpp e5da3c9bf0a1042b42522f1ab74ce798fbb1738d 
  src/tests/default_executor_tests.cpp 9e0fd678a7f5e9c2288fbb11a60cf6f339efa24f 
  src/tests/master_validation_tests.cpp 
99e350e0587e73e9ee25ef20dd369cd146bd446a 

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


Testing (updated)
---

Add new test case `DefaultExecutorTest.ROOT_TaskUsesExecutor`.


Thanks,

haosdent huang

Re: Review Request 52470: Filled missing executor info in tasks when `LAUNCH_GROUP`.

2016-10-12 Thread haosdent huang 

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

(Updated Oct. 12, 2016, 5:17 p.m.)


Review request for mesos, Anand Mazumdar and Vinod Kone.


Changes
---

Rebase.


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


Repository: mesos


Description
---

This fixed the navigate error in Web UI because Web UI uses the
executor id of the task to search the corresponding sandbox directory.
Web UI uses the task id as the executor id if the executor id of the
task is empty when searching the sandbox directory. It works fine when
tasks are launched by `CommandExecutor` because the executor id of the
task is equal to the task id in this case. However, when tasks are
launched by `DefaultExecutor`, the executor id of the task is defined
in the framework side and may different with the task id. So we need to
fill the `ExecutorInfo` of the `TaskInfo` when `LAUNCH_GROUP` to avoid
the Web UI uses incorrect executor id to search sandbox directory.


Diffs (updated)
-

  include/mesos/mesos.proto 7d0201ead69c816b9d51f1a41cd64a8f922b069d 
  include/mesos/v1/mesos.proto 301d1aedca7ac0729229ba9b05e5bf2b79f286a5 
  src/master/master.hpp 43518b9bf1bfaa54e26acc7f2e70c4161c667a84 
  src/master/master.cpp ad8993ae43e2508a3777f4062acaca1177ac77f2 
  src/master/validation.cpp e5da3c9bf0a1042b42522f1ab74ce798fbb1738d 
  src/tests/default_executor_tests.cpp 9e0fd678a7f5e9c2288fbb11a60cf6f339efa24f 
  src/tests/master_validation_tests.cpp 
99e350e0587e73e9ee25ef20dd369cd146bd446a 

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


Testing
---


Thanks,

haosdent huang

Re: Review Request 52470: Filled missing executor info in tasks when `LAUNCH_GROUP`.

2016-10-12 Thread haosdent huang 


> On Oct. 12, 2016, 12:08 a.m., Vinod Kone wrote:
> > src/master/master.cpp, lines 3529-3555
> > 
> >
> > I think we might want to mutate TaskInfo here so that authorization can 
> > use that information.
> > 
> > More importantly, we want pending tasks to have ExecutorInfo for the 
> > operator endpoints right? This is one of the main reasons we want to mutate 
> > TaskInfo right?
> > 
> > 
> > ```
> > const RepeatedPtrField& tasks = [&]() {
> >   if (operation.type() == Offer::Operation::LAUNCH) {
> > return operation.launch().task_infos();
> >   } else if (operation.type() == Offer::Operation::LAUNCH_GROUP) {
> > // We mutate TaskInfo to include ExecutorInfo to make it easy
> > // for operator API and WebUI to get access to the corresponding
> > // executor for task group tasks.
> > for (int i = 0; i < operation.launch_group().tasks().size(); ++i) {
> >   TaskInfo* task = 
> > operation.launch_group().mutable_task_group()->mutable_tasks(i);
> >   if (!task->has_executor()) {
> > 
> > task->mutable_executor()->CopyFrom(operation.launch_group().executor());
> >   }
> > }
> > 
> > return operation.launch_group().task_group().tasks();
> >   }
> > UNREACHABLE();
> >   }();
> > ```

Now I mutate this at the entrypoint of `accept` because of `_accpet` need to 
access the `TaskInfo` with `ExecutorInfo` (Call `Master::addTask`) as well.


- haosdent


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


On Oct. 12, 2016, 9:29 a.m., haosdent huang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52470/
> ---
> 
> (Updated Oct. 12, 2016, 9:29 a.m.)
> 
> 
> Review request for mesos, Anand Mazumdar and Vinod Kone.
> 
> 
> Bugs: MESOS-6283
> https://issues.apache.org/jira/browse/MESOS-6283
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This fixed the navigate error in Web UI because Web UI uses the
> executor id of the task to search the corresponding sandbox directory.
> Web UI uses the task id as the executor id if the executor id of the
> task is empty when searching the sandbox directory. It works fine when
> tasks are launched by `CommandExecutor` because the executor id of the
> task is equal to the task id in this case. However, when tasks are
> launched by `DefaultExecutor`, the executor id of the task is defined
> in the framework side and may different with the task id. So we need to
> fill the `ExecutorInfo` of the `TaskInfo` when `LAUNCH_GROUP` to avoid
> the Web UI uses incorrect executor id to search sandbox directory.
> 
> 
> Diffs
> -
> 
>   include/mesos/mesos.proto 7d0201ead69c816b9d51f1a41cd64a8f922b069d 
>   include/mesos/v1/mesos.proto 301d1aedca7ac0729229ba9b05e5bf2b79f286a5 
>   src/master/master.hpp 43518b9bf1bfaa54e26acc7f2e70c4161c667a84 
>   src/master/master.cpp ad8993ae43e2508a3777f4062acaca1177ac77f2 
>   src/master/validation.cpp e5da3c9bf0a1042b42522f1ab74ce798fbb1738d 
> 
> Diff: https://reviews.apache.org/r/52470/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> haosdent huang
> 
>

Re: Review Request 52470: Filled missing executor info in tasks when `LAUNCH_GROUP`.

2016-10-12 Thread haosdent huang 

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

(Updated Oct. 12, 2016, 10:02 a.m.)


Review request for mesos, Anand Mazumdar and Vinod Kone.


Changes
---

Rebase.


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


Repository: mesos


Description
---

This fixed the navigate error in Web UI because Web UI uses the
executor id of the task to search the corresponding sandbox directory.
Web UI uses the task id as the executor id if the executor id of the
task is empty when searching the sandbox directory. It works fine when
tasks are launched by `CommandExecutor` because the executor id of the
task is equal to the task id in this case. However, when tasks are
launched by `DefaultExecutor`, the executor id of the task is defined
in the framework side and may different with the task id. So we need to
fill the `ExecutorInfo` of the `TaskInfo` when `LAUNCH_GROUP` to avoid
the Web UI uses incorrect executor id to search sandbox directory.


Diffs (updated)
-

  include/mesos/mesos.proto 7d0201ead69c816b9d51f1a41cd64a8f922b069d 
  include/mesos/v1/mesos.proto 301d1aedca7ac0729229ba9b05e5bf2b79f286a5 
  src/master/master.hpp 43518b9bf1bfaa54e26acc7f2e70c4161c667a84 
  src/master/master.cpp ad8993ae43e2508a3777f4062acaca1177ac77f2 
  src/master/validation.cpp e5da3c9bf0a1042b42522f1ab74ce798fbb1738d 

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


Testing
---


Thanks,

haosdent huang

Re: Review Request 52470: Filled missing executor info in tasks when `LAUNCH_GROUP`.

2016-10-12 Thread haosdent huang 

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

(Updated Oct. 12, 2016, 9:29 a.m.)


Review request for mesos, Anand Mazumdar and Vinod Kone.


Changes
---

Address @vinodkone's comments.


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


Repository: mesos


Description
---

This fixed the navigate error in Web UI because Web UI uses the
executor id of the task to search the corresponding sandbox directory.
Web UI uses the task id as the executor id if the executor id of the
task is empty when searching the sandbox directory. It works fine when
tasks are launched by `CommandExecutor` because the executor id of the
task is equal to the task id in this case. However, when tasks are
launched by `DefaultExecutor`, the executor id of the task is defined
in the framework side and may different with the task id. So we need to
fill the `ExecutorInfo` of the `TaskInfo` when `LAUNCH_GROUP` to avoid
the Web UI uses incorrect executor id to search sandbox directory.


Diffs (updated)
-

  include/mesos/mesos.proto 7d0201ead69c816b9d51f1a41cd64a8f922b069d 
  include/mesos/v1/mesos.proto 301d1aedca7ac0729229ba9b05e5bf2b79f286a5 
  src/master/master.hpp 43518b9bf1bfaa54e26acc7f2e70c4161c667a84 
  src/master/master.cpp ad8993ae43e2508a3777f4062acaca1177ac77f2 
  src/master/validation.cpp e5da3c9bf0a1042b42522f1ab74ce798fbb1738d 

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


Testing (updated)
---


Thanks,

haosdent huang

Re: Review Request 52470: Filled missing executor info in tasks when `LAUNCH_GROUP`.

2016-10-11 Thread Vinod Kone 

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




src/master/master.cpp (lines 3529 - 3555)


I think we might want to mutate TaskInfo here so that authorization can use 
that information.

More importantly, we want pending tasks to have ExecutorInfo for the 
operator endpoints right? This is one of the main reasons we want to mutate 
TaskInfo right?

```
const RepeatedPtrField& tasks = [&]() {
  if (operation.type() == Offer::Operation::LAUNCH) {
return operation.launch().task_infos();
  } else if (operation.type() == Offer::Operation::LAUNCH_GROUP) {
// We mutate TaskInfo to include ExecutorInfo to make it easy
// for operator API and WebUI to get access to the corresponding
// executor for task group tasks.
for (int i = 0; i < operation.launch_group().tasks().size(); ++i) {
  TaskInfo* task = 
operation.launch_group().mutable_task_group()->mutable_tasks(i);
  if (!task->has_executor()) {

task->mutable_executor()->CopyFrom(operation.launch_group().executor());
  }
}

return operation.launch_group().task_group().tasks();
  }
UNREACHABLE();
  }();
```



src/master/master.cpp (line 4225)


need to update validate to expect `task.executor()` instead of not 
expecting it.

also need to ensure `task.executor()` is same as `launchgroup.executor` in 
validator.



src/master/master.cpp (line 4338)


need to do this in `accept()` instead, as commented earlier.


- Vinod Kone


On Oct. 11, 2016, 6:06 a.m., haosdent huang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52470/
> ---
> 
> (Updated Oct. 11, 2016, 6:06 a.m.)
> 
> 
> Review request for mesos, Anand Mazumdar and Vinod Kone.
> 
> 
> Bugs: MESOS-6283
> https://issues.apache.org/jira/browse/MESOS-6283
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This fixed the navigate error in Web UI because Web UI uses the
> executor id of the task to search the corresponding sandbox directory.
> Web UI uses the task id as the executor id if the executor id of the
> task is empty when searching the sandbox directory. It works fine when
> tasks are launched by `CommandExecutor` because the executor id of the
> task is equal to the task id in this case. However, when tasks are
> launched by `DefaultExecutor`, the executor id of the task is defined
> in the framework side and may different with the task id. So we need to
> fill the `ExecutorInfo` of the `TaskInfo` when `LAUNCH_GROUP` to avoid
> the Web UI uses incorrect executor id to search sandbox directory.
> 
> 
> Diffs
> -
> 
>   src/master/master.cpp ad8993ae43e2508a3777f4062acaca1177ac77f2 
> 
> Diff: https://reviews.apache.org/r/52470/diff/
> 
> 
> Testing
> ---
> 
> Add a new test case `CommandExecutorTest.EmptyExecutorIdInTask`.
> 
> 
> Thanks,
> 
> haosdent huang
> 
>

Re: Review Request 52470: Filled missing executor info in tasks when `LAUNCH_GROUP`.

2016-10-11 Thread haosdent huang 


> On Oct. 10, 2016, 6:54 p.m., Vinod Kone wrote:
> > I think it's probably worth to just mutate TaskInfo like you did in the 
> > very first diff rather than doing all these changes. They are bit messy and 
> > incomplete.
> 
> haosdent huang wrote:
> Let's use mutating way.

I drop following issues because use the mutating way now.


- haosdent


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


On Oct. 11, 2016, 6:06 a.m., haosdent huang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52470/
> ---
> 
> (Updated Oct. 11, 2016, 6:06 a.m.)
> 
> 
> Review request for mesos, Anand Mazumdar and Vinod Kone.
> 
> 
> Bugs: MESOS-6283
> https://issues.apache.org/jira/browse/MESOS-6283
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This fixed the navigate error in Web UI because Web UI uses the
> executor id of the task to search the corresponding sandbox directory.
> Web UI uses the task id as the executor id if the executor id of the
> task is empty when searching the sandbox directory. It works fine when
> tasks are launched by `CommandExecutor` because the executor id of the
> task is equal to the task id in this case. However, when tasks are
> launched by `DefaultExecutor`, the executor id of the task is defined
> in the framework side and may different with the task id. So we need to
> fill the `ExecutorInfo` of the `TaskInfo` when `LAUNCH_GROUP` to avoid
> the Web UI uses incorrect executor id to search sandbox directory.
> 
> 
> Diffs
> -
> 
>   src/master/master.cpp ad8993ae43e2508a3777f4062acaca1177ac77f2 
> 
> Diff: https://reviews.apache.org/r/52470/diff/
> 
> 
> Testing
> ---
> 
> Add a new test case `CommandExecutorTest.EmptyExecutorIdInTask`.
> 
> 
> Thanks,
> 
> haosdent huang
> 
>

Re: Review Request 52470: Filled missing executor info in tasks when `LAUNCH_GROUP`.

2016-10-11 Thread haosdent huang 

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

(Updated Oct. 11, 2016, 6:06 a.m.)


Review request for mesos, Anand Mazumdar and Vinod Kone.


Changes
---

Revert to mutate the `TaskInfo`.


Summary (updated)
-

Filled missing executor info in tasks when `LAUNCH_GROUP`.


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


Repository: mesos


Description (updated)
---

This fixed the navigate error in Web UI because Web UI uses the
executor id of the task to search the corresponding sandbox directory.
Web UI uses the task id as the executor id if the executor id of the
task is empty when searching the sandbox directory. It works fine when
tasks are launched by `CommandExecutor` because the executor id of the
task is equal to the task id in this case. However, when tasks are
launched by `DefaultExecutor`, the executor id of the task is defined
in the framework side and may different with the task id. So we need to
fill the `ExecutorInfo` of the `TaskInfo` when `LAUNCH_GROUP` to avoid
the Web UI uses incorrect executor id to search sandbox directory.


Diffs (updated)
-

  src/master/master.cpp ad8993ae43e2508a3777f4062acaca1177ac77f2 

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


Testing
---

Add a new test case `CommandExecutorTest.EmptyExecutorIdInTask`.


Thanks,

haosdent huang

Re: Review Request 52470: Filled missing executor info in tasks when `LAUNCH_GROUP`.

2016-10-07 Thread Vinod Kone 

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



ping, when do you plan to update this? we need this sooner than later.

- Vinod Kone


On Oct. 4, 2016, 5:59 p.m., haosdent huang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52470/
> ---
> 
> (Updated Oct. 4, 2016, 5:59 p.m.)
> 
> 
> Review request for mesos, Anand Mazumdar and Vinod Kone.
> 
> 
> Bugs: MESOS-6283
> https://issues.apache.org/jira/browse/MESOS-6283
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This fixed the navigate error in Web UI because Web UI uses the
> executor id of the task to search the corresponding sandbox directory.
> Web UI uses the task id as the executor id if the executor id of the
> task is empty when searching the sandbox directory. It works fine when
> tasks are launched by `CommandExecutor` because the executor id of the
> task is equal to the task id in this case. However, when tasks are
> launched by `DefaultExecutor`, the executor id of the task is defined
> in the framework side and may different with the task id. So we need to
> fill the `ExecutorInfo` of the `TaskInfo` when `LAUNCH_GROUP` to avoid
> the Web UI uses incorrect executor id to search sandbox directory.
> 
> 
> Diffs
> -
> 
>   src/master/master.cpp 02a2fb29bdd8484fc90e5cb033ac29b49a141860 
> 
> Diff: https://reviews.apache.org/r/52470/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> haosdent huang
> 
>

Re: Review Request 52470: Filled missing executor info in tasks when `LAUNCH_GROUP`.

2016-10-04 Thread Vinod Kone 


> On Oct. 3, 2016, 10:28 p.m., Vinod Kone wrote:
> > src/master/master.cpp, line 4339
> > 
> >
> > Mutating TaskInfo should be avoided as much as possible.
> > 
> > I don't follow why we need this. 
> > 
> > For the command executor, we were able to find it's corresponding 
> > executor without having to do this, why do we need it for task group tasks?
> 
> haosdent huang wrote:
> For tasks launched by the command executor, it's executor id are same 
> with the task id. 
> https://github.com/apache/mesos/blob/1.0.1/src/slave/slave.cpp#L3778 So in 
> Web UI, it would work that if we set the executor id to the task id when the 
> executor id is empty. 
> https://github.com/apache/mesos/blob/1.0.1/src/webui/master/static/js/controllers.js#L122
> 
> But for the tasks launched by the default executor, it's executor id are 
> given by the user 
> https://github.com/apache/mesos/blob/c13644984564179979d5ef5f40c8299fadd10ee7/src/cli/execute.cpp#L461-L462
>  . And it may different with the task id. So when Web UI use the task id to 
> search the executor, it would fail.

Instead of mutating TaskInfo, lets make sure "Task.executor_id" is properly set 
for task group tasks in `protobuf::createTask()`. You also need to make sure 
default executor is added to framework and slave structs in `addTask` (this is 
actually a bug that we need to fix irrespective of WebUI fix: maybe file a 
separate ticket for that and do it in a separate review).


- Vinod


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


On Oct. 4, 2016, 5:59 p.m., haosdent huang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52470/
> ---
> 
> (Updated Oct. 4, 2016, 5:59 p.m.)
> 
> 
> Review request for mesos, Anand Mazumdar and Vinod Kone.
> 
> 
> Bugs: MESOS-6283
> https://issues.apache.org/jira/browse/MESOS-6283
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This fixed the navigate error in Web UI because Web UI uses the
> executor id of the task to search the corresponding sandbox directory.
> Web UI uses the task id as the executor id if the executor id of the
> task is empty when searching the sandbox directory. It works fine when
> tasks are launched by `CommandExecutor` because the executor id of the
> task is equal to the task id in this case. However, when tasks are
> launched by `DefaultExecutor`, the executor id of the task is defined
> in the framework side and may different with the task id. So we need to
> fill the `ExecutorInfo` of the `TaskInfo` when `LAUNCH_GROUP` to avoid
> the Web UI uses incorrect executor id to search sandbox directory.
> 
> 
> Diffs
> -
> 
>   src/master/master.cpp 02a2fb29bdd8484fc90e5cb033ac29b49a141860 
> 
> Diff: https://reviews.apache.org/r/52470/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> haosdent huang
> 
>

Re: Review Request 52470: Filled missing executor info in tasks when `LAUNCH_GROUP`.

2016-10-04 Thread haosdent huang 

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

(Updated Oct. 4, 2016, 5:59 p.m.)


Review request for mesos, Anand Mazumdar and Vinod Kone.


Changes
---

Rebase.


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


Repository: mesos


Description
---

This fixed the navigate error in Web UI because Web UI uses the
executor id of the task to search the corresponding sandbox directory.
Web UI uses the task id as the executor id if the executor id of the
task is empty when searching the sandbox directory. It works fine when
tasks are launched by `CommandExecutor` because the executor id of the
task is equal to the task id in this case. However, when tasks are
launched by `DefaultExecutor`, the executor id of the task is defined
in the framework side and may different with the task id. So we need to
fill the `ExecutorInfo` of the `TaskInfo` when `LAUNCH_GROUP` to avoid
the Web UI uses incorrect executor id to search sandbox directory.


Diffs (updated)
-

  src/master/master.cpp 02a2fb29bdd8484fc90e5cb033ac29b49a141860 

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


Testing
---


Thanks,

haosdent huang

Re: Review Request 52470: Filled missing executor info in tasks when `LAUNCH_GROUP`.

2016-10-03 Thread haosdent huang 


> On Oct. 3, 2016, 10:28 p.m., Vinod Kone wrote:
> > src/master/master.cpp, line 4339
> > 
> >
> > Mutating TaskInfo should be avoided as much as possible.
> > 
> > I don't follow why we need this. 
> > 
> > For the command executor, we were able to find it's corresponding 
> > executor without having to do this, why do we need it for task group tasks?

For tasks launched by the command executor, it's executor id are same with the 
task id. https://github.com/apache/mesos/blob/1.0.1/src/slave/slave.cpp#L3778 
So in Web UI, it would work that if we set the executor id to the task id when 
the executor id is empty. 
https://github.com/apache/mesos/blob/1.0.1/src/webui/master/static/js/controllers.js#L122

But for the tasks launched by the default executor, it's executor id are given 
by the user 
https://github.com/apache/mesos/blob/c13644984564179979d5ef5f40c8299fadd10ee7/src/cli/execute.cpp#L461-L462
 . And it may different with the task id. So when Web UI use the task id to 
search the executor, it would fail.


- haosdent


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


On Oct. 3, 2016, 2:56 a.m., haosdent huang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52470/
> ---
> 
> (Updated Oct. 3, 2016, 2:56 a.m.)
> 
> 
> Review request for mesos, Anand Mazumdar and Vinod Kone.
> 
> 
> Bugs: MESOS-6283
> https://issues.apache.org/jira/browse/MESOS-6283
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This fixed the navigate error in Web UI because Web UI uses the
> executor id of the task to search the corresponding sandbox directory.
> Web UI uses the task id as the executor id if the executor id of the
> task is empty when searching the sandbox directory. It works fine when
> tasks are launched by `CommandExecutor` because the executor id of the
> task is equal to the task id in this case. However, when tasks are
> launched by `DefaultExecutor`, the executor id of the task is defined
> in the framework side and may different with the task id. So we need to
> fill the `ExecutorInfo` of the `TaskInfo` when `LAUNCH_GROUP` to avoid
> the Web UI uses incorrect executor id to search sandbox directory.
> 
> 
> Diffs
> -
> 
>   src/master/master.cpp c83ee2f9fa05372748ff5056229fbe2bf06bfabb 
> 
> Diff: https://reviews.apache.org/r/52470/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> haosdent huang
> 
>

Re: Review Request 52470: Filled missing executor info in tasks when `LAUNCH_GROUP`.

2016-10-03 Thread Vinod Kone 

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




src/master/master.cpp (line 4339)


Mutating TaskInfo should be avoided as much as possible.

I don't follow why we need this. 

For the command executor, we were able to find it's corresponding executor 
without having to do this, why do we need it for task group tasks?


- Vinod Kone


On Oct. 3, 2016, 2:56 a.m., haosdent huang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52470/
> ---
> 
> (Updated Oct. 3, 2016, 2:56 a.m.)
> 
> 
> Review request for mesos, Anand Mazumdar and Vinod Kone.
> 
> 
> Bugs: MESOS-6283
> https://issues.apache.org/jira/browse/MESOS-6283
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This fixed the navigate error in Web UI because Web UI uses the
> executor id of the task to search the corresponding sandbox directory.
> Web UI uses the task id as the executor id if the executor id of the
> task is empty when searching the sandbox directory. It works fine when
> tasks are launched by `CommandExecutor` because the executor id of the
> task is equal to the task id in this case. However, when tasks are
> launched by `DefaultExecutor`, the executor id of the task is defined
> in the framework side and may different with the task id. So we need to
> fill the `ExecutorInfo` of the `TaskInfo` when `LAUNCH_GROUP` to avoid
> the Web UI uses incorrect executor id to search sandbox directory.
> 
> 
> Diffs
> -
> 
>   src/master/master.cpp c83ee2f9fa05372748ff5056229fbe2bf06bfabb 
> 
> Diff: https://reviews.apache.org/r/52470/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> haosdent huang
> 
>

Re: Review Request 52470: Filled missing executor info in tasks when `LAUNCH_GROUP`.

2016-10-02 Thread haosdent huang 

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

(Updated Oct. 3, 2016, 2:56 a.m.)


Review request for mesos, Anand Mazumdar and Vinod Kone.


Changes
---

Rebase.


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


Repository: mesos


Description
---

This fixed the navigate error in Web UI because Web UI uses the
executor id of the task to search the corresponding sandbox directory.
Web UI uses the task id as the executor id if the executor id of the
task is empty when searching the sandbox directory. It works fine when
tasks are launched by `CommandExecutor` because the executor id of the
task is equal to the task id in this case. However, when tasks are
launched by `DefaultExecutor`, the executor id of the task is defined
in the framework side and may different with the task id. So we need to
fill the `ExecutorInfo` of the `TaskInfo` when `LAUNCH_GROUP` to avoid
the Web UI uses incorrect executor id to search sandbox directory.


Diffs (updated)
-

  src/master/master.cpp c83ee2f9fa05372748ff5056229fbe2bf06bfabb 

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


Testing
---


Thanks,

haosdent huang