You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@mesos.apache.org by Anand Mazumdar <an...@apache.org> on 2017/10/14 01:40:00 UTC
Re: Review Request 62241: Added
SchedulerHttpApiTest.UpdateHttpToPidSchedulerAndBack test.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/62241/#review188053
-----------------------------------------------------------
Looks good, just a few minor things to clean up.
src/tests/scheduler_http_api_tests.cpp
Lines 674 (patched)
<https://reviews.apache.org/r/62241/#comment265133>
s/an/a
src/tests/scheduler_http_api_tests.cpp
Lines 712-716 (patched)
<https://reviews.apache.org/r/62241/#comment265134>
Do you care about a `HEARTBEAT` even for this test?
src/tests/scheduler_http_api_tests.cpp
Lines 737-747 (patched)
<https://reviews.apache.org/r/62241/#comment265135>
Kill this? We don't need this to test if the downgrade worked.
- Anand Mazumdar
On Sept. 12, 2017, 12:59 p.m., Ilya Pronin wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/62241/
> -----------------------------------------------------------
>
> (Updated Sept. 12, 2017, 12:59 p.m.)
>
>
> Review request for mesos and Anand Mazumdar.
>
>
> Bugs: MESOS-7867
> https://issues.apache.org/jira/browse/MESOS-7867
>
>
> Repository: mesos
>
>
> Description
> -------
>
> This test verifies that we are able to upgrade from a PID based
> framework to an HTTP framework and then downgrade back without
> restarting the master.
>
>
> Diffs
> -----
>
> src/tests/scheduler_http_api_tests.cpp cc03be0d2110f08023bf0ce181da37658737bd7c
>
>
> Diff: https://reviews.apache.org/r/62241/diff/1/
>
>
> Testing
> -------
>
> Ran `make check`. Verified that the test fails without the previous patch.
>
>
> Thanks,
>
> Ilya Pronin
>
>
Re: Review Request 62241: Added
SchedulerHttpApiTest.UpdateHttpToPidSchedulerAndBack test.
Posted by Ilya Pronin <ip...@twopensource.com>.
> On Oct. 13, 2017, 6:40 p.m., Anand Mazumdar wrote:
> > src/tests/scheduler_http_api_tests.cpp
> > Lines 737-747 (patched)
> > <https://reviews.apache.org/r/62241/diff/1/?file=1820104#file1820104line737>
> >
> > Kill this? We don't need this to test if the downgrade worked.
>
> Ilya Pronin wrote:
> The crash that this test reproduces occurs when the master handles a `Message` from the scheduler. We can remove the reconciliation part and the crash would still happen when the scheduler driver sends a `TEARDOWN` call message from `SchedulerProcess::stop()`. But I think it is better to be explicit here and not leave exercising the code that we were fixing to the cleanup logic.
>
> Anand Mazumdar wrote:
> I see. Can you modify the comment to also mention why are you exercising the message handling i.e., it used to crash the master previously? Otherwise, its not immediately obvious as to why it's being done here.
Done!
- Ilya
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/62241/#review188053
-----------------------------------------------------------
On Oct. 16, 2017, 1:05 p.m., Ilya Pronin wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/62241/
> -----------------------------------------------------------
>
> (Updated Oct. 16, 2017, 1:05 p.m.)
>
>
> Review request for mesos and Anand Mazumdar.
>
>
> Bugs: MESOS-7867
> https://issues.apache.org/jira/browse/MESOS-7867
>
>
> Repository: mesos
>
>
> Description
> -------
>
> This test verifies that we are able to upgrade from a PID based
> framework to an HTTP framework and then downgrade back without
> restarting the master.
>
>
> Diffs
> -----
>
> src/tests/scheduler_http_api_tests.cpp cc03be0d2110f08023bf0ce181da37658737bd7c
>
>
> Diff: https://reviews.apache.org/r/62241/diff/3/
>
>
> Testing
> -------
>
> Ran `make check`. Verified that the test fails without the previous patch.
>
>
> Thanks,
>
> Ilya Pronin
>
>
Re: Review Request 62241: Added
SchedulerHttpApiTest.UpdateHttpToPidSchedulerAndBack test.
Posted by Ilya Pronin <ip...@twopensource.com>.
> On Oct. 13, 2017, 6:40 p.m., Anand Mazumdar wrote:
> > src/tests/scheduler_http_api_tests.cpp
> > Lines 674 (patched)
> > <https://reviews.apache.org/r/62241/diff/1/?file=1820104#file1820104line674>
> >
> > s/an/a
I'm pretty sure it's "an HTTP" :) https://en.wikipedia.org/wiki/Hypertext_Transfer_Protocol
> On Oct. 13, 2017, 6:40 p.m., Anand Mazumdar wrote:
> > src/tests/scheduler_http_api_tests.cpp
> > Lines 712-716 (patched)
> > <https://reviews.apache.org/r/62241/diff/1/?file=1820104#file1820104line712>
> >
> > Do you care about a `HEARTBEAT` even for this test?
No. I copied that part from the other test :) I think we can can remove this part and failover right after we get a `SUBSCRIBED` event. Ran the updated test with `--gtest_repeat=1000 --gtest_break_on_failure` to verify that I don't introduce any flakiness and it seems fine.
> On Oct. 13, 2017, 6:40 p.m., Anand Mazumdar wrote:
> > src/tests/scheduler_http_api_tests.cpp
> > Lines 737-747 (patched)
> > <https://reviews.apache.org/r/62241/diff/1/?file=1820104#file1820104line737>
> >
> > Kill this? We don't need this to test if the downgrade worked.
The crash that this test reproduces occurs when the master handles a `Message` from the scheduler. We can remove the reconciliation part and the crash would still happen when the scheduler driver sends a `TEARDOWN` call message from `SchedulerProcess::stop()`. But I think it is better to be explicit here and not leave exercising the code that we were fixing to the cleanup logic.
- Ilya
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/62241/#review188053
-----------------------------------------------------------
On Oct. 16, 2017, 11:17 a.m., Ilya Pronin wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/62241/
> -----------------------------------------------------------
>
> (Updated Oct. 16, 2017, 11:17 a.m.)
>
>
> Review request for mesos and Anand Mazumdar.
>
>
> Bugs: MESOS-7867
> https://issues.apache.org/jira/browse/MESOS-7867
>
>
> Repository: mesos
>
>
> Description
> -------
>
> This test verifies that we are able to upgrade from a PID based
> framework to an HTTP framework and then downgrade back without
> restarting the master.
>
>
> Diffs
> -----
>
> src/tests/scheduler_http_api_tests.cpp cc03be0d2110f08023bf0ce181da37658737bd7c
>
>
> Diff: https://reviews.apache.org/r/62241/diff/2/
>
>
> Testing
> -------
>
> Ran `make check`. Verified that the test fails without the previous patch.
>
>
> Thanks,
>
> Ilya Pronin
>
>
Re: Review Request 62241: Added
SchedulerHttpApiTest.UpdateHttpToPidSchedulerAndBack test.
Posted by Anand Mazumdar <an...@apache.org>.
> On Oct. 14, 2017, 1:40 a.m., Anand Mazumdar wrote:
> > src/tests/scheduler_http_api_tests.cpp
> > Lines 737-747 (patched)
> > <https://reviews.apache.org/r/62241/diff/1/?file=1820104#file1820104line737>
> >
> > Kill this? We don't need this to test if the downgrade worked.
>
> Ilya Pronin wrote:
> The crash that this test reproduces occurs when the master handles a `Message` from the scheduler. We can remove the reconciliation part and the crash would still happen when the scheduler driver sends a `TEARDOWN` call message from `SchedulerProcess::stop()`. But I think it is better to be explicit here and not leave exercising the code that we were fixing to the cleanup logic.
I see. Can you modify the comment to also mention why are you exercising the message handling i.e., it used to crash the master previously? Otherwise, its not immediately obvious as to why it's being done here.
- Anand
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/62241/#review188053
-----------------------------------------------------------
On Oct. 16, 2017, 6:17 p.m., Ilya Pronin wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/62241/
> -----------------------------------------------------------
>
> (Updated Oct. 16, 2017, 6:17 p.m.)
>
>
> Review request for mesos and Anand Mazumdar.
>
>
> Bugs: MESOS-7867
> https://issues.apache.org/jira/browse/MESOS-7867
>
>
> Repository: mesos
>
>
> Description
> -------
>
> This test verifies that we are able to upgrade from a PID based
> framework to an HTTP framework and then downgrade back without
> restarting the master.
>
>
> Diffs
> -----
>
> src/tests/scheduler_http_api_tests.cpp cc03be0d2110f08023bf0ce181da37658737bd7c
>
>
> Diff: https://reviews.apache.org/r/62241/diff/2/
>
>
> Testing
> -------
>
> Ran `make check`. Verified that the test fails without the previous patch.
>
>
> Thanks,
>
> Ilya Pronin
>
>