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 2020/01/10 01:48:14 UTC
Review Request 71976: Added support for systemd socket activation API.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/71976/
-----------------------------------------------------------
Review request for mesos, Benjamin Bannier and Benjamin Mahler.
Repository: mesos
Description
-------
Added support for the systemd socket activation api,
that allows systemd to pass listening file descriptors
to a given service.
Diffs
-----
src/linux/systemd.hpp f760e2c02683f0169ba5d3b0a3da53f66a7f3d91
src/linux/systemd.cpp 8126b31a542b62ce51cc6e0f6372986bffcc948b
Diff: https://reviews.apache.org/r/71976/diff/1/
Testing
-------
Thanks,
Benno Evers
Re: Review Request 71976: Added support for systemd socket activation
API.
Posted by Benjamin Bannier <bb...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/71976/#review219222
-----------------------------------------------------------
Fix it, then Ship it!
src/linux/systemd.cpp
Lines 346 (patched)
<https://reviews.apache.org/r/71976/#comment307370>
Can you declare and initilize in one go?
```
int flags = ...
```
src/linux/systemd.cpp
Lines 351-355 (patched)
<https://reviews.apache.org/r/71976/#comment307371>
Let's also declare and init in one go. Using a ternary operator shouldn't be too bad.
src/linux/systemd.cpp
Lines 384 (patched)
<https://reviews.apache.org/r/71976/#comment307376>
It might be helpful to rephrase the `Error` so we can show `*listenPidEnv` in quotes.
src/linux/systemd.cpp
Lines 385 (patched)
<https://reviews.apache.org/r/71976/#comment307373>
No period at the end of this `Error`.
src/linux/systemd.cpp
Lines 402 (patched)
<https://reviews.apache.org/r/71976/#comment307377>
It might be helpful to rephrase the `Error` so we can show `*listenPidEnv` in quotes.
src/linux/systemd.cpp
Lines 403 (patched)
<https://reviews.apache.org/r/71976/#comment307374>
No period at the end of this `Error`.
src/linux/systemd.cpp
Lines 408 (patched)
<https://reviews.apache.org/r/71976/#comment307375>
Remove period.
src/linux/systemd.cpp
Lines 414 (patched)
<https://reviews.apache.org/r/71976/#comment307378>
Can we add context here which fd we couldn't set cloexec for?
```
return Error("Could not set CLOEXEC for filedescriptor ...: + " cloexec.error());
```
src/linux/systemd.cpp
Lines 419 (patched)
<https://reviews.apache.org/r/71976/#comment307372>
Let's add spaces around `+` and a newline before `return`.
src/linux/systemd.cpp
Lines 458 (patched)
<https://reviews.apache.org/r/71976/#comment307380>
Can you document somewhere very prominently that this function is not safe to call from multiple threads since it mutates env?
src/linux/systemd.cpp
Lines 460-462 (patched)
<https://reviews.apache.org/r/71976/#comment307379>
Let's use `os::unsetenv` for "consistency".
- Benjamin Bannier
On Jan. 10, 2020, 2:48 a.m., Benno Evers wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/71976/
> -----------------------------------------------------------
>
> (Updated Jan. 10, 2020, 2:48 a.m.)
>
>
> Review request for mesos, Benjamin Bannier and Benjamin Mahler.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Added support for the systemd socket activation api,
> that allows systemd to pass listening file descriptors
> to a given service.
>
>
> Diffs
> -----
>
> src/linux/systemd.hpp f760e2c02683f0169ba5d3b0a3da53f66a7f3d91
> src/linux/systemd.cpp 8126b31a542b62ce51cc6e0f6372986bffcc948b
>
>
> Diff: https://reviews.apache.org/r/71976/diff/1/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Benno Evers
>
>