You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@mesos.apache.org by Benno Evers <be...@mesosphere.com> on 2019/12/03 18:30:59 UTC
Re: Review Request 71833: Created unix domain socket on agent startup.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/71833/
-----------------------------------------------------------
(Updated Dec. 3, 2019, 6:30 p.m.)
Review request for mesos and Benjamin Bannier.
Changes
-------
Cleanup some todo's and linter issues.
Summary (updated)
-----------------
Created unix domain socket on agent startup.
Bugs: MESOS-10036
https://issues.apache.org/jira/browse/MESOS-10036
Repository: mesos
Description (updated)
-------
Added agent code to check whether domain socket support is
enabled, and if so to create or open the socket at
MESOS_DOMAIN_SOCKET_LOCATION.
Diffs (updated)
-----
src/local/local.cpp 8ff43618f06463b4ba86b64b25e5de692e406448
src/slave/main.cpp fd58637cd680291e6794bcdb0655603bb97744c7
src/slave/slave.hpp 77b5bc0082c6bb73fbd48a2ebe812629921645cb
src/slave/slave.cpp 3839a120446339fea8aa857f431a2dba28ed4002
src/tests/cluster.cpp f7bc882a644ec65710ada3d15507e1d4c5ba06f7
src/tests/mock_slave.cpp 71be957884ea88258ef37e60649e3947e89b12d0
Diff: https://reviews.apache.org/r/71833/diff/4/
Changes: https://reviews.apache.org/r/71833/diff/3-4/
Testing
-------
Thanks,
Benno Evers
Re: Review Request 71833: Created unix domain socket on agent startup.
Posted by Benno Evers <be...@mesosphere.com>.
> On Dec. 5, 2019, 3:12 p.m., Benjamin Bannier wrote:
> > src/tests/cluster.cpp
> > Lines 630 (patched)
> > <https://reviews.apache.org/r/71833/diff/4/?file=2181822#file2181822line630>
> >
> > There is another, currently unhandled branch where `domain_socket_location` exists, but is not a socket. Either we handle that case as well, or just check if _something_ exists where we'd expect the socket and let this fail later.
I think the latter variant is what the code does, no? If there exists another file that is not a socket, we should end up with an error upon calling `bind()`.
- Benno
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/71833/#review218939
-----------------------------------------------------------
On Dec. 3, 2019, 6:30 p.m., Benno Evers wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/71833/
> -----------------------------------------------------------
>
> (Updated Dec. 3, 2019, 6:30 p.m.)
>
>
> Review request for mesos and Benjamin Bannier.
>
>
> Bugs: MESOS-10036
> https://issues.apache.org/jira/browse/MESOS-10036
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Added agent code to check whether domain socket support is
> enabled, and if so to create or open the socket at
> MESOS_DOMAIN_SOCKET_LOCATION.
>
>
> Diffs
> -----
>
> src/local/local.cpp 8ff43618f06463b4ba86b64b25e5de692e406448
> src/slave/main.cpp fd58637cd680291e6794bcdb0655603bb97744c7
> src/slave/slave.hpp 77b5bc0082c6bb73fbd48a2ebe812629921645cb
> src/slave/slave.cpp 3839a120446339fea8aa857f431a2dba28ed4002
> src/tests/cluster.cpp f7bc882a644ec65710ada3d15507e1d4c5ba06f7
> src/tests/mock_slave.cpp 71be957884ea88258ef37e60649e3947e89b12d0
>
>
> Diff: https://reviews.apache.org/r/71833/diff/4/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Benno Evers
>
>
Re: Review Request 71833: Created unix domain socket on agent startup.
Posted by Benjamin Bannier <bb...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/71833/#review218939
-----------------------------------------------------------
src/slave/main.cpp
Lines 614 (patched)
<https://reviews.apache.org/r/71833/#comment306915>
Same comments as in `tests/cluster.cpp` below.
src/tests/cluster.cpp
Lines 613-614 (patched)
<https://reviews.apache.org/r/71833/#comment306918>
Either implement this `TODO` before landing or explicitly reference the code you added above.
src/tests/cluster.cpp
Lines 630 (patched)
<https://reviews.apache.org/r/71833/#comment306917>
There is another, currently unhandled branch where `domain_socket_location` exists, but is not a socket. Either we handle that case as well, or just check if _something_ exists where we'd expect the socket and let this fail later.
src/tests/cluster.cpp
Lines 653-654 (patched)
<https://reviews.apache.org/r/71833/#comment306914>
This should be implemented when this patch lands, or be less terse.
src/tests/cluster.cpp
Lines 656 (patched)
<https://reviews.apache.org/r/71833/#comment306913>
Is this outdated? I think we want to log whether we created a new file or used an existing socket, and indicate whether we successfully did `bind` a socket at that path.
- Benjamin Bannier
On Dec. 3, 2019, 7:30 p.m., Benno Evers wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/71833/
> -----------------------------------------------------------
>
> (Updated Dec. 3, 2019, 7:30 p.m.)
>
>
> Review request for mesos and Benjamin Bannier.
>
>
> Bugs: MESOS-10036
> https://issues.apache.org/jira/browse/MESOS-10036
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Added agent code to check whether domain socket support is
> enabled, and if so to create or open the socket at
> MESOS_DOMAIN_SOCKET_LOCATION.
>
>
> Diffs
> -----
>
> src/local/local.cpp 8ff43618f06463b4ba86b64b25e5de692e406448
> src/slave/main.cpp fd58637cd680291e6794bcdb0655603bb97744c7
> src/slave/slave.hpp 77b5bc0082c6bb73fbd48a2ebe812629921645cb
> src/slave/slave.cpp 3839a120446339fea8aa857f431a2dba28ed4002
> src/tests/cluster.cpp f7bc882a644ec65710ada3d15507e1d4c5ba06f7
> src/tests/mock_slave.cpp 71be957884ea88258ef37e60649e3947e89b12d0
>
>
> Diff: https://reviews.apache.org/r/71833/diff/4/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Benno Evers
>
>
Re: Review Request 71833: Created unix domain socket on agent startup.
Posted by Benjamin Bannier <bb...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/71833/#review219213
-----------------------------------------------------------
Fix it, then Ship it!
src/common/domain_sockets.hpp
Lines 31 (patched)
<https://reviews.apache.org/r/71833/#comment307325>
Can you get rid of the `using` decl and use the FQN instead?
src/common/domain_sockets.hpp
Lines 39 (patched)
<https://reviews.apache.org/r/71833/#comment307326>
Can you get rid of this variable and instead directly return `*socket`?
src/common/domain_sockets.hpp
Lines 80-81 (patched)
<https://reviews.apache.org/r/71833/#comment307327>
Are we not returning an `Error` here on purpose? Without context that would seem preferable. If we do indeed want to allow this to fail we should add a comment explaining the reasoning.
src/slave/main.cpp
Lines 59-61 (original), 59-62 (patched)
<https://reviews.apache.org/r/71833/#comment307328>
Let's keep these sorted.
src/slave/main.cpp
Lines 111 (patched)
<https://reviews.apache.org/r/71833/#comment307329>
This looks unused.
src/slave/main.cpp
Line 352 (original), 358 (patched)
<https://reviews.apache.org/r/71833/#comment307330>
Reuse a generic constant instead of magic `108`.
src/slave/main.cpp
Lines 629 (patched)
<https://reviews.apache.org/r/71833/#comment307331>
Can you init this with `None`?
src/slave/main.cpp
Lines 633 (patched)
<https://reviews.apache.org/r/71833/#comment307332>
You can use `CHECK_SOME` here.
src/tests/cluster.cpp
Lines 124 (patched)
<https://reviews.apache.org/r/71833/#comment307333>
This looks unused (and sorted incorrectly).
src/tests/cluster.cpp
Lines 614 (patched)
<https://reviews.apache.org/r/71833/#comment307335>
Init with `None`.
src/tests/cluster.cpp
Lines 618 (patched)
<https://reviews.apache.org/r/71833/#comment307334>
`CHECK_SOME`
- Benjamin Bannier
On Jan. 10, 2020, 2:46 a.m., Benno Evers wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/71833/
> -----------------------------------------------------------
>
> (Updated Jan. 10, 2020, 2:46 a.m.)
>
>
> Review request for mesos and Benjamin Bannier.
>
>
> Bugs: MESOS-10036
> https://issues.apache.org/jira/browse/MESOS-10036
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Added agent code to check whether domain socket support is
> enabled, and if so to create or open the socket at
> MESOS_DOMAIN_SOCKET_LOCATION.
>
>
> Diffs
> -----
>
> src/Makefile.am 47ad1bd27e155f0193aafa956df0bd43baafd348
> src/common/domain_sockets.hpp PRE-CREATION
> src/local/local.cpp 8ff43618f06463b4ba86b64b25e5de692e406448
> src/slave/main.cpp fd58637cd680291e6794bcdb0655603bb97744c7
> src/slave/slave.hpp 77b5bc0082c6bb73fbd48a2ebe812629921645cb
> src/slave/slave.cpp 3839a120446339fea8aa857f431a2dba28ed4002
> src/tests/cluster.cpp f7bc882a644ec65710ada3d15507e1d4c5ba06f7
> src/tests/mock_slave.cpp 71be957884ea88258ef37e60649e3947e89b12d0
>
>
> Diff: https://reviews.apache.org/r/71833/diff/5/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Benno Evers
>
>
Re: Review Request 71833: Created unix domain socket on agent startup.
Posted by Benno Evers <be...@mesosphere.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/71833/
-----------------------------------------------------------
(Updated Jan. 10, 2020, 1:46 a.m.)
Review request for mesos and Benjamin Bannier.
Changes
-------
Addressed review comments
Bugs: MESOS-10036
https://issues.apache.org/jira/browse/MESOS-10036
Repository: mesos
Description
-------
Added agent code to check whether domain socket support is
enabled, and if so to create or open the socket at
MESOS_DOMAIN_SOCKET_LOCATION.
Diffs (updated)
-----
src/Makefile.am 47ad1bd27e155f0193aafa956df0bd43baafd348
src/common/domain_sockets.hpp PRE-CREATION
src/local/local.cpp 8ff43618f06463b4ba86b64b25e5de692e406448
src/slave/main.cpp fd58637cd680291e6794bcdb0655603bb97744c7
src/slave/slave.hpp 77b5bc0082c6bb73fbd48a2ebe812629921645cb
src/slave/slave.cpp 3839a120446339fea8aa857f431a2dba28ed4002
src/tests/cluster.cpp f7bc882a644ec65710ada3d15507e1d4c5ba06f7
src/tests/mock_slave.cpp 71be957884ea88258ef37e60649e3947e89b12d0
Diff: https://reviews.apache.org/r/71833/diff/5/
Changes: https://reviews.apache.org/r/71833/diff/4-5/
Testing
-------
Thanks,
Benno Evers