Re: Review Request 71833: Created unix domain socket on agent startup.

2020-01-10 Thread Benjamin Bannier 

---
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)


Can you get rid of the `using` decl and use the FQN instead?



src/common/domain_sockets.hpp
Lines 39 (patched)


Can you get rid of this variable and instead directly return `*socket`?



src/common/domain_sockets.hpp
Lines 80-81 (patched)


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)


Let's keep these sorted.



src/slave/main.cpp
Lines 111 (patched)


This looks unused.



src/slave/main.cpp
Line 352 (original), 358 (patched)


Reuse a generic constant instead of magic `108`.



src/slave/main.cpp
Lines 629 (patched)


Can you init this with `None`?



src/slave/main.cpp
Lines 633 (patched)


You can use `CHECK_SOME` here.



src/tests/cluster.cpp
Lines 124 (patched)


This looks unused (and sorted incorrectly).



src/tests/cluster.cpp
Lines 614 (patched)


Init with `None`.



src/tests/cluster.cpp
Lines 618 (patched)


`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.

2020-01-09 Thread Benno Evers 

---
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

Re: Review Request 71833: Created unix domain socket on agent startup.

2019-12-09 Thread Benno Evers 


> On Dec. 5, 2019, 3:12 p.m., Benjamin Bannier wrote:
> > src/tests/cluster.cpp
> > Lines 630 (patched)
> > 
> >
> > 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.

2019-12-05 Thread Benjamin Bannier 

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/71833/#review218939
---




src/slave/main.cpp
Lines 614 (patched)


Same comments as in `tests/cluster.cpp` below.



src/tests/cluster.cpp
Lines 613-614 (patched)


Either implement this `TODO` before landing or explicitly reference the 
code you added above.



src/tests/cluster.cpp
Lines 630 (patched)


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)


This should be implemented when this patch lands, or be less terse.



src/tests/cluster.cpp
Lines 656 (patched)


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.

2019-12-03 Thread Benno Evers 

---
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