You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@mesos.apache.org by Vinod Kone <vi...@gmail.com> on 2015/12/02 23:29:40 UTC
Re: Review Request 38878: Added test for the Subscribe->Subscribed
workflow for the Executor HTTP API
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/38878/#review108731
-----------------------------------------------------------
src/tests/executor_http_api_tests.cpp (line 749)
<https://reviews.apache.org/r/38878/#comment168186>
s/on/by/
src/tests/executor_http_api_tests.cpp (line 757)
<https://reviews.apache.org/r/38878/#comment168195>
Hmm. this test is a bit convoluted. you start a pid based executor first, wait for its update to be received by the scheduler and then start a subscribe request from http executor?
i'm assuming you can't simply send a subscribe request because the slave wouldn't have an executor struct in its map. is it possible to stop the pid executor from starting at all? maybe by using testing isolator/containerizer?
- Vinod Kone
On Nov. 30, 2015, 3:56 a.m., Anand Mazumdar wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38878/
> -----------------------------------------------------------
>
> (Updated Nov. 30, 2015, 3:56 a.m.)
>
>
> Review request for mesos, Ben Mahler, Isabel Jimenez, and Vinod Kone.
>
>
> Bugs: MESOS-3515
> https://issues.apache.org/jira/browse/MESOS-3515
>
>
> Repository: mesos
>
>
> Description
> -------
>
> This change adds a basic test to validate the implementation for Subscribe->Subscribed workflow on the `api/v1/executor` endpoint on Agent.
>
>
> Diffs
> -----
>
> src/tests/executor_http_api_tests.cpp fe9df1f4d68babaf0960a3b689ffbe60704b8ad5
>
> Diff: https://reviews.apache.org/r/38878/diff/
>
>
> Testing
> -------
>
> make check. Would add more agent recovery tests/executor reconnect tests in a separate patch.
>
>
> Thanks,
>
> Anand Mazumdar
>
>
Re: Review Request 38878: Added test for the Subscribe->Subscribed
workflow for the Executor HTTP API
Posted by Anand Mazumdar <ma...@gmail.com>.
> On Dec. 2, 2015, 10:29 p.m., Vinod Kone wrote:
> > src/tests/executor_http_api_tests.cpp, line 757
> > <https://reviews.apache.org/r/38878/diff/3/?file=1148139#file1148139line757>
> >
> > Hmm. this test is a bit convoluted. you start a pid based executor first, wait for its update to be received by the scheduler and then start a subscribe request from http executor?
> >
> > i'm assuming you can't simply send a subscribe request because the slave wouldn't have an executor struct in its map. is it possible to stop the pid executor from starting at all? maybe by using testing isolator/containerizer?
>
> Anand Mazumdar wrote:
> Yes, The reasoning for doing this is as you specified. I could not send a `Subscribe` request directly as the slave did not have the `executor` struct in its map.
>
> 1. I had previously looked into using the `Containerizer` directly along with a `MockExecutor` but there did not seem to be currently an overload of `StartSlave(...)` that existed https://github.com/apache/mesos/blob/master/src/tests/mesos.hpp#L156 . Hence, we would still need a scheduler to do a `launchTask(...)` similar to what is being done here or am I missing something ?
> 2. We already have an existing instance in this file that also uses the above trick. https://github.com/apache/mesos/blob/master/src/tests/executor_http_api_tests.cpp#L324
>
> That being said, I may be missing something here and I am open to alternate methods that can simplify this test.
Thanks Vinod for helping me resolve 1.
Seems like we can pass on a `MockExecutor` to `TestContainerizer` and then pass that on to `StartSlave`. That explains why we don't have an `StartSlave(...)` overload as I had remarked earlier.
Also, instead of waiting on `launch` as we had discussed earlier. I now drop the `ExecutorRegisterMessage` to be sure that the previous PID based executor has not registered. We then send a `Subscribe` request from the HTTP based executor.
Would update the other test-case I pointed out in 2. in a separate patch.
- Anand
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/38878/#review108731
-----------------------------------------------------------
On Dec. 4, 2015, 7:02 p.m., Anand Mazumdar wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38878/
> -----------------------------------------------------------
>
> (Updated Dec. 4, 2015, 7:02 p.m.)
>
>
> Review request for mesos, Ben Mahler, Isabel Jimenez, and Vinod Kone.
>
>
> Bugs: MESOS-3515
> https://issues.apache.org/jira/browse/MESOS-3515
>
>
> Repository: mesos
>
>
> Description
> -------
>
> This change adds a basic test to validate the implementation for Subscribe->Subscribed workflow on the `api/v1/executor` endpoint on Agent.
>
>
> Diffs
> -----
>
> src/tests/executor_http_api_tests.cpp fe9df1f4d68babaf0960a3b689ffbe60704b8ad5
>
> Diff: https://reviews.apache.org/r/38878/diff/
>
>
> Testing
> -------
>
> make check. Would add more agent recovery tests/executor reconnect tests in a separate patch.
>
>
> Thanks,
>
> Anand Mazumdar
>
>
Re: Review Request 38878: Added test for the Subscribe->Subscribed
workflow for the Executor HTTP API
Posted by Anand Mazumdar <ma...@gmail.com>.
> On Dec. 2, 2015, 10:29 p.m., Vinod Kone wrote:
> > src/tests/executor_http_api_tests.cpp, line 757
> > <https://reviews.apache.org/r/38878/diff/3/?file=1148139#file1148139line757>
> >
> > Hmm. this test is a bit convoluted. you start a pid based executor first, wait for its update to be received by the scheduler and then start a subscribe request from http executor?
> >
> > i'm assuming you can't simply send a subscribe request because the slave wouldn't have an executor struct in its map. is it possible to stop the pid executor from starting at all? maybe by using testing isolator/containerizer?
Yes, The reasoning for doing this is as you specified. I could not send a `Subscribe` request directly as the slave did not have the `executor` struct in its map.
1. I had previously looked into using the `Containerizer` directly along with a `MockExecutor` but there did not seem to be currently an overload of `StartSlave(...)` that existed https://github.com/apache/mesos/blob/master/src/tests/mesos.hpp#L156 . Hence, we would still need a scheduler to do a `launchTask(...)` similar to what is being done here or am I missing something ?
2. We already have an existing instance in this file that also uses the above trick. https://github.com/apache/mesos/blob/master/src/tests/executor_http_api_tests.cpp#L324
That being said, I may be missing something here and I am open to alternate methods that can simplify this test.
- Anand
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/38878/#review108731
-----------------------------------------------------------
On Dec. 3, 2015, 6:48 a.m., Anand Mazumdar wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38878/
> -----------------------------------------------------------
>
> (Updated Dec. 3, 2015, 6:48 a.m.)
>
>
> Review request for mesos, Ben Mahler, Isabel Jimenez, and Vinod Kone.
>
>
> Bugs: MESOS-3515
> https://issues.apache.org/jira/browse/MESOS-3515
>
>
> Repository: mesos
>
>
> Description
> -------
>
> This change adds a basic test to validate the implementation for Subscribe->Subscribed workflow on the `api/v1/executor` endpoint on Agent.
>
>
> Diffs
> -----
>
> src/tests/executor_http_api_tests.cpp fe9df1f4d68babaf0960a3b689ffbe60704b8ad5
>
> Diff: https://reviews.apache.org/r/38878/diff/
>
>
> Testing
> -------
>
> make check. Would add more agent recovery tests/executor reconnect tests in a separate patch.
>
>
> Thanks,
>
> Anand Mazumdar
>
>