mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Benjamin Bannier <bbann...@apache.org>
Subject Re: Review Request 71976: Added support for systemd socket activation API.
Date Sat, 11 Jan 2020 11:57:56 GMT

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


Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message