mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Anand Mazumdar <an...@apache.org>
Subject Re: Review Request 54954: Improve the warning when failing to find the executor PID.
Date Mon, 20 Mar 2017 22:16:53 GMT

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




src/slave/state.cpp
Line 545 (original), 546 (patched)
<https://reviews.apache.org/r/54954/#comment241872>

    hmm, In this function and elsewhere in general, we return early when there is an error.
This seems to do so for the "happy" cases too.
    
    Can we keep the original not exists check but move the `LOG(WARNING)` inside the `if`
conditional?
    
    Something like:
    `cpp
    if (!os::exists(path)) {
      LOG(WARNING) << ...;
    }
    ```



src/slave/state.cpp
Line 548 (original), 549-553 (patched)
<https://reviews.apache.org/r/54954/#comment241873>

    Move this to L544 as per my earlier comment.
    
    Also can you quote `executorId` since it's generated by the scheduler and can have spaces.
AFAIK, We haven't been consistent with this in the agent code too and should do a sweep there
too.


- Anand Mazumdar


On Jan. 3, 2017, 5:13 p.m., James Peach wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/54954/
> -----------------------------------------------------------
> 
> (Updated Jan. 3, 2017, 5:13 p.m.)
> 
> 
> Review request for mesos, Anand Mazumdar and Jiang Yan Xu.
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> When recovering the run state, if we don't find either the libprocess
> PID or the HTTP marker for the executor, that fact was being logged,
> but there was nothing to identify the executor or container. Add the
> framework, executor and container IDs to the message, and clarify that
> we can't find either marker.
> 
> 
> Diffs
> -----
> 
>   src/slave/paths.hpp e64e89678b7d2ab32702edaf5575c92e41fae1e2 
>   src/slave/state.cpp a94bb8d7029295abef70d6595ebc732ac1ab87a8 
> 
> 
> Diff: https://reviews.apache.org/r/54954/diff/1/
> 
> 
> Testing
> -------
> 
> make check (Fedora 25)
> 
> 
> Thanks,
> 
> James Peach
> 
>


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