You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@mesos.apache.org by Benjamin Mahler <bm...@apache.org> on 2017/08/03 18:30:18 UTC
Review Request 61410: Remove support for omitting 202 responses to old
libprocess clients.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/61410/
-----------------------------------------------------------
Review request for mesos and Benjamin Hindman.
Repository: mesos
Description
-------
Prior to commit d5fe51c on April 11 2014, we needed to omit the 202
responses for libprocess messages because libprocess did not read
the data from its message passing sockets.
This change removes support for omitting the responses to old clients.
This also means that any 3rdparty libprocess clients (e.g. someone's
go library) need to be correctly reading from their sockets to
communicate with libprocess after this change.
Diffs
-----
3rdparty/libprocess/src/process.cpp af5a75950bf24059d291acfd48399ab2d55eb8c2
3rdparty/libprocess/src/tests/process_tests.cpp 30d0fb845468ff993e42a5568f57be131f0cd24b
Diff: https://reviews.apache.org/r/61410/diff/1/
Testing
-------
make check
Thanks,
Benjamin Mahler
Re: Review Request 61410: Remove support for omitting 202 responses
to old libprocess clients.
Posted by Benjamin Mahler <bm...@apache.org>.
> On Aug. 3, 2017, 7:13 p.m., Benjamin Hindman wrote:
> > Random question, do you know if there is a test that sends responses back to libprocess to exercise the `process::internal::ignore_recv_data` function?
Hm.. I couldn't find one.
> On Aug. 3, 2017, 7:13 p.m., Benjamin Hindman wrote:
> > 3rdparty/libprocess/src/tests/process_tests.cpp
> > Lines 1318 (patched)
> > <https://reviews.apache.org/r/61410/diff/1/?file=1789074#file1789074line1318>
> >
> > Were these actually necessary? I thought this was only necessary after we started to use `http::Server` since without `keepAlive` the `http::Server` will shutdown the socket right away (and `connection.disconnect()` would fail?).
They're necessary because before this change no response is received, so the connection remains open even with no keep alive. After this change, the response is received and http::Connection will disconnect after receiving the response. This test expects to be able to disconnect sucessfully, whereas disconnect() fails if already disconnected (although perhaps it should just return a ready future in that case!)
- Benjamin
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/61410/#review182140
-----------------------------------------------------------
On Aug. 3, 2017, 6:30 p.m., Benjamin Mahler wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/61410/
> -----------------------------------------------------------
>
> (Updated Aug. 3, 2017, 6:30 p.m.)
>
>
> Review request for mesos and Benjamin Hindman.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Prior to commit d5fe51c on April 11 2014, we needed to omit the 202
> responses for libprocess messages because libprocess did not read
> the data from its message passing sockets.
>
> This change removes support for omitting the responses to old clients.
> This also means that any 3rdparty libprocess clients (e.g. someone's
> go library) need to be correctly reading from their sockets to
> communicate with libprocess after this change.
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/src/process.cpp af5a75950bf24059d291acfd48399ab2d55eb8c2
> 3rdparty/libprocess/src/tests/process_tests.cpp 30d0fb845468ff993e42a5568f57be131f0cd24b
>
>
> Diff: https://reviews.apache.org/r/61410/diff/1/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Benjamin Mahler
>
>
Re: Review Request 61410: Remove support for omitting 202 responses
to old libprocess clients.
Posted by Benjamin Hindman <be...@berkeley.edu>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/61410/#review182140
-----------------------------------------------------------
Fix it, then Ship it!
Random question, do you know if there is a test that sends responses back to libprocess to exercise the `process::internal::ignore_recv_data` function?
3rdparty/libprocess/src/tests/process_tests.cpp
Lines 1318 (patched)
<https://reviews.apache.org/r/61410/#comment257983>
Were these actually necessary? I thought this was only necessary after we started to use `http::Server` since without `keepAlive` the `http::Server` will shutdown the socket right away (and `connection.disconnect()` would fail?).
- Benjamin Hindman
On Aug. 3, 2017, 6:30 p.m., Benjamin Mahler wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/61410/
> -----------------------------------------------------------
>
> (Updated Aug. 3, 2017, 6:30 p.m.)
>
>
> Review request for mesos and Benjamin Hindman.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Prior to commit d5fe51c on April 11 2014, we needed to omit the 202
> responses for libprocess messages because libprocess did not read
> the data from its message passing sockets.
>
> This change removes support for omitting the responses to old clients.
> This also means that any 3rdparty libprocess clients (e.g. someone's
> go library) need to be correctly reading from their sockets to
> communicate with libprocess after this change.
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/src/process.cpp af5a75950bf24059d291acfd48399ab2d55eb8c2
> 3rdparty/libprocess/src/tests/process_tests.cpp 30d0fb845468ff993e42a5568f57be131f0cd24b
>
>
> Diff: https://reviews.apache.org/r/61410/diff/1/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Benjamin Mahler
>
>