You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@mesos.apache.org by Jojy Varghese <jo...@mesosphere.io> on 2015/12/02 17:26:48 UTC
Review Request 40873: SimpleRegistryPullerTest: Moved blob response
preparation to the top.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/40873/
-----------------------------------------------------------
Review request for mesos and Timothy Chen.
Bugs: MESOS-4025
https://issues.apache.org/jira/browse/MESOS-4025
Repository: mesos
Description
-------
By moving the preperation task above any test task avoids socket waiting for the response to be prepared. This change has eliminated socket timeouts seen sometimes.
Diffs
-----
src/tests/containerizer/provisioner_docker_tests.cpp c63bf53fee40ef12536a16e11f4d5224c4e4278e
Diff: https://reviews.apache.org/r/40873/diff/
Testing
-------
make check (600 count).
Thanks,
Jojy Varghese
Re: Review Request 40873: SimpleRegistryPullerTest: Moved blob
response preparation to the top.
Posted by Anand Mazumdar <ma...@gmail.com>.
> On Dec. 2, 2015, 4:40 p.m., Anand Mazumdar wrote:
> > @jojy, Do we know why preparing the response was taking so long that was causing the socket to timeout previously ?
>
> Jojy Varghese wrote:
> @anand: The observed behavior is that the server socket gets a RST. This is ususally when the peer side of the socket is closed. Also observed is that there are asycnhronous socket close being done ( i think from the HTTP layer). This was observed when I added a log inside the dtor of the socket (socket.hpp: L103). For failed test cases, I saw an extra socket close. This change reduces the time between the peer interactions. This is not a "solution" of the problem but an effort to eliminate possible false alarms and be able to focus on the real issue. Since its very difficult to reproduce this issue predictably, we need to elimitate all red herrings.
Thanks for the reply @jojy.
Do we know why the peer side of the socket was closed i.e. it got a RST and why the peer side of the socket was closed in the first place ? Can you also help me understand how are these false alarms ? To me, it suggests a bug inside the `RegistryClient` itself and we are finding a workaround in the tests to mask it, no ?
- Anand
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/40873/#review108662
-----------------------------------------------------------
On Dec. 2, 2015, 4:26 p.m., Jojy Varghese wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/40873/
> -----------------------------------------------------------
>
> (Updated Dec. 2, 2015, 4:26 p.m.)
>
>
> Review request for mesos and Timothy Chen.
>
>
> Bugs: MESOS-4025
> https://issues.apache.org/jira/browse/MESOS-4025
>
>
> Repository: mesos
>
>
> Description
> -------
>
> By moving the preperation task above any test task avoids socket waiting for the response to be prepared. This change has eliminated socket timeouts seen sometimes.
>
>
> Diffs
> -----
>
> src/tests/containerizer/provisioner_docker_tests.cpp c63bf53fee40ef12536a16e11f4d5224c4e4278e
>
> Diff: https://reviews.apache.org/r/40873/diff/
>
>
> Testing
> -------
>
> make check (600 count).
>
>
> Thanks,
>
> Jojy Varghese
>
>
Re: Review Request 40873: SimpleRegistryPullerTest: Moved blob
response preparation to the top.
Posted by Jojy Varghese <jo...@mesosphere.io>.
> On Dec. 2, 2015, 4:40 p.m., Anand Mazumdar wrote:
> > @jojy, Do we know why preparing the response was taking so long that was causing the socket to timeout previously ?
>
> Jojy Varghese wrote:
> @anand: The observed behavior is that the server socket gets a RST. This is ususally when the peer side of the socket is closed. Also observed is that there are asycnhronous socket close being done ( i think from the HTTP layer). This was observed when I added a log inside the dtor of the socket (socket.hpp: L103). For failed test cases, I saw an extra socket close. This change reduces the time between the peer interactions. This is not a "solution" of the problem but an effort to eliminate possible false alarms and be able to focus on the real issue. Since its very difficult to reproduce this issue predictably, we need to elimitate all red herrings.
>
> Anand Mazumdar wrote:
> Thanks for the reply @jojy.
>
> Do we know why the peer side of the socket was closed i.e. it got a RST and why the peer side of the socket was closed in the first place ? Can you also help me understand how are these false alarms ? To me, it suggests a bug inside the `RegistryClient` itself and we are finding a workaround in the tests to mask it, no ?
This fix addresses couple of things:
- Eliminates the assumption that a socket accept call will be invoked before the client sends the request.
- Eliminates the assumption of any timeout dependencies of http client socket on the processing at the server side.
Assumption no. 1 is clearly a bug in the test that this fix addresses.
@anand: We discussed about this and I agree that we could be hiding an underlying issue at the http layer. But we want to address the real issue and not red herring. This test case failure is just that. Moreover, these fixes are improvements that the test need anyways to eliminate the above mentioned assumptions.
- Jojy
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/40873/#review108662
-----------------------------------------------------------
On Dec. 2, 2015, 4:26 p.m., Jojy Varghese wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/40873/
> -----------------------------------------------------------
>
> (Updated Dec. 2, 2015, 4:26 p.m.)
>
>
> Review request for mesos and Timothy Chen.
>
>
> Bugs: MESOS-4025
> https://issues.apache.org/jira/browse/MESOS-4025
>
>
> Repository: mesos
>
>
> Description
> -------
>
> By moving the preperation task above any test task avoids socket waiting for the response to be prepared. This change has eliminated socket timeouts seen sometimes.
>
>
> Diffs
> -----
>
> src/tests/containerizer/provisioner_docker_tests.cpp c63bf53fee40ef12536a16e11f4d5224c4e4278e
>
> Diff: https://reviews.apache.org/r/40873/diff/
>
>
> Testing
> -------
>
> make check (600 count).
>
>
> Thanks,
>
> Jojy Varghese
>
>
Re: Review Request 40873: SimpleRegistryPullerTest: Moved blob
response preparation to the top.
Posted by Jojy Varghese <jo...@mesosphere.io>.
> On Dec. 2, 2015, 4:40 p.m., Anand Mazumdar wrote:
> > @jojy, Do we know why preparing the response was taking so long that was causing the socket to timeout previously ?
@anand: The observed behavior is that the server socket gets a RST. This is ususally when the peer side of the socket is closed. Also observed is that there are asycnhronous socket close being done ( i think from the HTTP layer). This was observed when I added a log inside the dtor of the socket (socket.hpp: L103). For failed test cases, I saw an extra socket close. This change reduces the time between the peer interactions. This is not a "solution" of the problem but an effort to eliminate possible false alarms and be able to focus on the real issue. Since its very difficult to reproduce this issue predictably, we need to elimitate all red herrings.
- Jojy
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/40873/#review108662
-----------------------------------------------------------
On Dec. 2, 2015, 4:26 p.m., Jojy Varghese wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/40873/
> -----------------------------------------------------------
>
> (Updated Dec. 2, 2015, 4:26 p.m.)
>
>
> Review request for mesos and Timothy Chen.
>
>
> Bugs: MESOS-4025
> https://issues.apache.org/jira/browse/MESOS-4025
>
>
> Repository: mesos
>
>
> Description
> -------
>
> By moving the preperation task above any test task avoids socket waiting for the response to be prepared. This change has eliminated socket timeouts seen sometimes.
>
>
> Diffs
> -----
>
> src/tests/containerizer/provisioner_docker_tests.cpp c63bf53fee40ef12536a16e11f4d5224c4e4278e
>
> Diff: https://reviews.apache.org/r/40873/diff/
>
>
> Testing
> -------
>
> make check (600 count).
>
>
> Thanks,
>
> Jojy Varghese
>
>
Re: Review Request 40873: SimpleRegistryPullerTest: Moved blob
response preparation to the top.
Posted by Anand Mazumdar <ma...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/40873/#review108662
-----------------------------------------------------------
@jojy, Do we know why preparing the response was taking so long that was causing the socket to timeout previously ?
- Anand Mazumdar
On Dec. 2, 2015, 4:26 p.m., Jojy Varghese wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/40873/
> -----------------------------------------------------------
>
> (Updated Dec. 2, 2015, 4:26 p.m.)
>
>
> Review request for mesos and Timothy Chen.
>
>
> Bugs: MESOS-4025
> https://issues.apache.org/jira/browse/MESOS-4025
>
>
> Repository: mesos
>
>
> Description
> -------
>
> By moving the preperation task above any test task avoids socket waiting for the response to be prepared. This change has eliminated socket timeouts seen sometimes.
>
>
> Diffs
> -----
>
> src/tests/containerizer/provisioner_docker_tests.cpp c63bf53fee40ef12536a16e11f4d5224c4e4278e
>
> Diff: https://reviews.apache.org/r/40873/diff/
>
>
> Testing
> -------
>
> make check (600 count).
>
>
> Thanks,
>
> Jojy Varghese
>
>
Re: Review Request 40873: SimpleRegistryPullerTest: Moved blob
response preparation to the top.
Posted by Mesos ReviewBot <re...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/40873/#review108723
-----------------------------------------------------------
Patch looks great!
Reviews applied: [40872, 40873]
Passed command: export OS=ubuntu:14.04;export CONFIGURATION="--verbose";export COMPILER=gcc; ./support/docker_build.sh
- Mesos ReviewBot
On Dec. 2, 2015, 4:26 p.m., Jojy Varghese wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/40873/
> -----------------------------------------------------------
>
> (Updated Dec. 2, 2015, 4:26 p.m.)
>
>
> Review request for mesos and Timothy Chen.
>
>
> Bugs: MESOS-4025
> https://issues.apache.org/jira/browse/MESOS-4025
>
>
> Repository: mesos
>
>
> Description
> -------
>
> By moving the preperation task above any test task avoids socket waiting for the response to be prepared. This change has eliminated socket timeouts seen sometimes.
>
>
> Diffs
> -----
>
> src/tests/containerizer/provisioner_docker_tests.cpp c63bf53fee40ef12536a16e11f4d5224c4e4278e
>
> Diff: https://reviews.apache.org/r/40873/diff/
>
>
> Testing
> -------
>
> make check (600 count).
>
>
> Thanks,
>
> Jojy Varghese
>
>
Re: Review Request 40873: RegistryClientTests: Created separate
server to serve blobs.
Posted by Timothy Chen <tn...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/40873/#review109716
-----------------------------------------------------------
Ship it!
Ship It!
- Timothy Chen
On Dec. 3, 2015, 5:20 p.m., Jojy Varghese wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/40873/
> -----------------------------------------------------------
>
> (Updated Dec. 3, 2015, 5:20 p.m.)
>
>
> Review request for mesos and Timothy Chen.
>
>
> Bugs: MESOS-4025
> https://issues.apache.org/jira/browse/MESOS-4025
>
>
> Repository: mesos
>
>
> Description
> -------
>
> By having a separate blob server, it simulates the real world more closely. It also allows the test server to be in accept mode early.
>
>
> Diffs
> -----
>
> src/tests/containerizer/provisioner_docker_tests.cpp c63bf53fee40ef12536a16e11f4d5224c4e4278e
>
> Diff: https://reviews.apache.org/r/40873/diff/
>
>
> Testing
> -------
>
> make check (600 count).
>
>
> Thanks,
>
> Jojy Varghese
>
>
Re: Review Request 40873: RegistryClientTests: Created separate
server to serve blobs.
Posted by Jojy Varghese <jo...@mesosphere.io>.
> On Dec. 3, 2015, 6:21 p.m., Mesos ReviewBot wrote:
> > Patch looks great!
> >
> > Reviews applied: [40872, 40873]
> >
> > Passed command: export OS=ubuntu:14.04;export CONFIGURATION="--verbose";export COMPILER=gcc; ./support/docker_build.sh
>
> Timothy Chen wrote:
> What does simulates the real world more closely mean here?
The change introduces a new server to serve the blobs like how the docker registry has a blob server (where the blob request is rediercted to).
- Jojy
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/40873/#review108853
-----------------------------------------------------------
On Dec. 3, 2015, 5:20 p.m., Jojy Varghese wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/40873/
> -----------------------------------------------------------
>
> (Updated Dec. 3, 2015, 5:20 p.m.)
>
>
> Review request for mesos and Timothy Chen.
>
>
> Bugs: MESOS-4025
> https://issues.apache.org/jira/browse/MESOS-4025
>
>
> Repository: mesos
>
>
> Description
> -------
>
> By having a separate blob server, it simulates the real world more closely. It also allows the test server to be in accept mode early.
>
>
> Diffs
> -----
>
> src/tests/containerizer/provisioner_docker_tests.cpp c63bf53fee40ef12536a16e11f4d5224c4e4278e
>
> Diff: https://reviews.apache.org/r/40873/diff/
>
>
> Testing
> -------
>
> make check (600 count).
>
>
> Thanks,
>
> Jojy Varghese
>
>
Re: Review Request 40873: RegistryClientTests: Created separate
server to serve blobs.
Posted by Timothy Chen <tn...@apache.org>.
> On Dec. 3, 2015, 6:21 p.m., Mesos ReviewBot wrote:
> > Patch looks great!
> >
> > Reviews applied: [40872, 40873]
> >
> > Passed command: export OS=ubuntu:14.04;export CONFIGURATION="--verbose";export COMPILER=gcc; ./support/docker_build.sh
What does simulates the real world more closely mean here?
- Timothy
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/40873/#review108853
-----------------------------------------------------------
On Dec. 3, 2015, 5:20 p.m., Jojy Varghese wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/40873/
> -----------------------------------------------------------
>
> (Updated Dec. 3, 2015, 5:20 p.m.)
>
>
> Review request for mesos and Timothy Chen.
>
>
> Bugs: MESOS-4025
> https://issues.apache.org/jira/browse/MESOS-4025
>
>
> Repository: mesos
>
>
> Description
> -------
>
> By having a separate blob server, it simulates the real world more closely. It also allows the test server to be in accept mode early.
>
>
> Diffs
> -----
>
> src/tests/containerizer/provisioner_docker_tests.cpp c63bf53fee40ef12536a16e11f4d5224c4e4278e
>
> Diff: https://reviews.apache.org/r/40873/diff/
>
>
> Testing
> -------
>
> make check (600 count).
>
>
> Thanks,
>
> Jojy Varghese
>
>
Re: Review Request 40873: RegistryClientTests: Created separate
server to serve blobs.
Posted by Mesos ReviewBot <re...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/40873/#review108853
-----------------------------------------------------------
Patch looks great!
Reviews applied: [40872, 40873]
Passed command: export OS=ubuntu:14.04;export CONFIGURATION="--verbose";export COMPILER=gcc; ./support/docker_build.sh
- Mesos ReviewBot
On Dec. 3, 2015, 5:20 p.m., Jojy Varghese wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/40873/
> -----------------------------------------------------------
>
> (Updated Dec. 3, 2015, 5:20 p.m.)
>
>
> Review request for mesos and Timothy Chen.
>
>
> Bugs: MESOS-4025
> https://issues.apache.org/jira/browse/MESOS-4025
>
>
> Repository: mesos
>
>
> Description
> -------
>
> By having a separate blob server, it simulates the real world more closely. It also allows the test server to be in accept mode early.
>
>
> Diffs
> -----
>
> src/tests/containerizer/provisioner_docker_tests.cpp c63bf53fee40ef12536a16e11f4d5224c4e4278e
>
> Diff: https://reviews.apache.org/r/40873/diff/
>
>
> Testing
> -------
>
> make check (600 count).
>
>
> Thanks,
>
> Jojy Varghese
>
>
Re: Review Request 40873: RegistryClientTests: Created separate
server to serve blobs.
Posted by Jojy Varghese <jo...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/40873/
-----------------------------------------------------------
(Updated Dec. 3, 2015, 5:20 p.m.)
Review request for mesos and Timothy Chen.
Summary (updated)
-----------------
RegistryClientTests: Created separate server to serve blobs.
Bugs: MESOS-4025
https://issues.apache.org/jira/browse/MESOS-4025
Repository: mesos
Description (updated)
-------
By having a separate blob server, it simulates the real world more closely. It also allows the test server to be in accept mode early.
Diffs (updated)
-----
src/tests/containerizer/provisioner_docker_tests.cpp c63bf53fee40ef12536a16e11f4d5224c4e4278e
Diff: https://reviews.apache.org/r/40873/diff/
Testing
-------
make check (600 count).
Thanks,
Jojy Varghese