You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@mesos.apache.org by haosdent huang <ha...@gmail.com> on 2015/07/25 17:01:53 UTC
Review Request 36814: Use executor in mesos-execute.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/36814/
-----------------------------------------------------------
Review request for mesos, Adam B and Ben Mahler.
Bugs: MESOS-527
https://issues.apache.org/jira/browse/MESOS-527
Repository: mesos
Description
-------
Use executor in mesos-execute.
Diffs
-----
src/cli/execute.cpp d070164e080cb74ee15d3184487a121f429a29fc
Diff: https://reviews.apache.org/r/36814/diff/
Testing
-------
Thanks,
haosdent huang
Re: Review Request 36814: Fill executor_id in state.json when task is
run in CommandExecutor.
Posted by Mesos ReviewBot <re...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/36814/#review93858
-----------------------------------------------------------
Patch looks great!
Reviews applied: [36814]
All tests passed.
- Mesos ReviewBot
On Aug. 2, 2015, 7:57 a.m., haosdent huang wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/36814/
> -----------------------------------------------------------
>
> (Updated Aug. 2, 2015, 7:57 a.m.)
>
>
> Review request for mesos, Adam B and Ben Mahler.
>
>
> Bugs: MESOS-527
> https://issues.apache.org/jira/browse/MESOS-527
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Fill executor_id in state.json when task is run in CommandExecutor.
>
>
> Diffs
> -----
>
> src/common/http.cpp a74c51d9392d0b0a67d51a0143eb314cfca11245
> src/tests/common/http_tests.cpp 38d062b2b4062e0a2fc912bad0cc2d73339eb66e
>
> Diff: https://reviews.apache.org/r/36814/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> haosdent huang
>
>
Re: Review Request 36814: Fill executor_id in state.json when task is
run in CommandExecutor.
Posted by Ben Mahler <be...@gmail.com>.
> On Aug. 4, 2015, 3:21 a.m., Adam B wrote:
> > LGTM. I like the idea of not setting the executorId=taskId in the actual TaskInfo/Task, since that could confuse other logic downstream that expects the executor/executorId to be empty for command executors. However, since this is exposed in state.json, there might be external tools/scripts that expect the same. Now they can just check if executorId==taskId, assuming there are no custom executors using the same naming scheme. Could you please add a note to docs/upgrades.md mentioning the API change, in case anybody does depend on the old behavior?
> > Does this actually solve the problem of: "The webui is broken for command executors because the master does not know the executor ID for the tasks using a command executor."?
> > @bmahler do you have any further thoughts/questions on this simple change?
Can we at least add comment in the slave code where we set the executor id for command executor, that the master is now assuming the slave uses the task id as the executor id?
- Ben
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/36814/#review93995
-----------------------------------------------------------
On Aug. 4, 2015, 9:33 a.m., haosdent huang wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/36814/
> -----------------------------------------------------------
>
> (Updated Aug. 4, 2015, 9:33 a.m.)
>
>
> Review request for mesos, Adam B, Ben Mahler, and Michael Park.
>
>
> Bugs: MESOS-527
> https://issues.apache.org/jira/browse/MESOS-527
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Fill executor_id in state.json when task is run in CommandExecutor.
>
>
> Diffs
> -----
>
> src/common/http.cpp a74c51d9392d0b0a67d51a0143eb314cfca11245
> src/tests/common/http_tests.cpp 38d062b2b4062e0a2fc912bad0cc2d73339eb66e
>
> Diff: https://reviews.apache.org/r/36814/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> haosdent huang
>
>
Re: Review Request 36814: Fill executor_id in state.json when task is
run in CommandExecutor.
Posted by Adam B <ad...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/36814/#review93995
-----------------------------------------------------------
Ship it!
LGTM. I like the idea of not setting the executorId=taskId in the actual TaskInfo/Task, since that could confuse other logic downstream that expects the executor/executorId to be empty for command executors. However, since this is exposed in state.json, there might be external tools/scripts that expect the same. Now they can just check if executorId==taskId, assuming there are no custom executors using the same naming scheme. Could you please add a note to docs/upgrades.md mentioning the API change, in case anybody does depend on the old behavior?
Does this actually solve the problem of: "The webui is broken for command executors because the master does not know the executor ID for the tasks using a command executor."?
@bmahler do you have any further thoughts/questions on this simple change?
- Adam B
On Aug. 2, 2015, 12:57 a.m., haosdent huang wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/36814/
> -----------------------------------------------------------
>
> (Updated Aug. 2, 2015, 12:57 a.m.)
>
>
> Review request for mesos, Adam B and Ben Mahler.
>
>
> Bugs: MESOS-527
> https://issues.apache.org/jira/browse/MESOS-527
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Fill executor_id in state.json when task is run in CommandExecutor.
>
>
> Diffs
> -----
>
> src/common/http.cpp a74c51d9392d0b0a67d51a0143eb314cfca11245
> src/tests/common/http_tests.cpp 38d062b2b4062e0a2fc912bad0cc2d73339eb66e
>
> Diff: https://reviews.apache.org/r/36814/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> haosdent huang
>
>
Re: Review Request 36814: Fill executor_id in state.json when task is
run in CommandExecutor.
Posted by haosdent huang <ha...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/36814/
-----------------------------------------------------------
(Updated Aug. 2, 2015, 7:57 a.m.)
Review request for mesos, Adam B and Ben Mahler.
Bugs: MESOS-527
https://issues.apache.org/jira/browse/MESOS-527
Repository: mesos
Description
-------
Fill executor_id in state.json when task is run in CommandExecutor.
Diffs (updated)
-----
src/common/http.cpp a74c51d9392d0b0a67d51a0143eb314cfca11245
src/tests/common/http_tests.cpp 38d062b2b4062e0a2fc912bad0cc2d73339eb66e
Diff: https://reviews.apache.org/r/36814/diff/
Testing
-------
Thanks,
haosdent huang
Re: Review Request 36814: Fill executor_id in state.json when task is
run in CommandExecutor.
Posted by haosdent huang <ha...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/36814/
-----------------------------------------------------------
(Updated Aug. 2, 2015, 6:21 a.m.)
Review request for mesos, Adam B and Ben Mahler.
Changes
-------
Update HTTPTest.ModelTask
Bugs: MESOS-527
https://issues.apache.org/jira/browse/MESOS-527
Repository: mesos
Description
-------
Fill executor_id in state.json when task is run in CommandExecutor.
Diffs (updated)
-----
src/common/http.cpp a74c51d9392d0b0a67d51a0143eb314cfca11245
src/tests/common/http_tests.cpp 38d062b2b4062e0a2fc912bad0cc2d73339eb66e
Diff: https://reviews.apache.org/r/36814/diff/
Testing
-------
Thanks,
haosdent huang
Re: Review Request 36814: Fill executor_id in state.json when task is
run in CommandExecutor.
Posted by haosdent huang <ha...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/36814/
-----------------------------------------------------------
(Updated Aug. 1, 2015, 5:58 p.m.)
Review request for mesos, Adam B and Ben Mahler.
Summary (updated)
-----------------
Fill executor_id in state.json when task is run in CommandExecutor.
Bugs: MESOS-527
https://issues.apache.org/jira/browse/MESOS-527
Repository: mesos
Description (updated)
-------
Fill executor_id in state.json when task is run in CommandExecutor.
Diffs
-----
src/common/http.cpp a74c51d9392d0b0a67d51a0143eb314cfca11245
Diff: https://reviews.apache.org/r/36814/diff/
Testing
-------
Thanks,
haosdent huang
Re: Review Request 36814: Use executor in mesos-execute.
Posted by haosdent huang <ha...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/36814/
-----------------------------------------------------------
(Updated Aug. 1, 2015, 5:49 p.m.)
Review request for mesos, Adam B and Ben Mahler.
Bugs: MESOS-527
https://issues.apache.org/jira/browse/MESOS-527
Repository: mesos
Description
-------
Use executor in mesos-execute.
Diffs (updated)
-----
src/common/http.cpp a74c51d9392d0b0a67d51a0143eb314cfca11245
Diff: https://reviews.apache.org/r/36814/diff/
Testing
-------
Thanks,
haosdent huang
Re: Review Request 36814: Use executor in mesos-execute.
Posted by Mesos ReviewBot <re...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/36814/#review93034
-----------------------------------------------------------
Patch looks great!
Reviews applied: [36814]
All tests passed.
- Mesos ReviewBot
On July 25, 2015, 3:01 p.m., haosdent huang wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/36814/
> -----------------------------------------------------------
>
> (Updated July 25, 2015, 3:01 p.m.)
>
>
> Review request for mesos, Adam B and Ben Mahler.
>
>
> Bugs: MESOS-527
> https://issues.apache.org/jira/browse/MESOS-527
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Use executor in mesos-execute.
>
>
> Diffs
> -----
>
> src/cli/execute.cpp d070164e080cb74ee15d3184487a121f429a29fc
>
> Diff: https://reviews.apache.org/r/36814/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> haosdent huang
>
>