mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Jiang Yan Xu <...@jxu.me>
Subject Re: Review Request 56680: Apply glog to agent exit messages.
Date Mon, 20 Mar 2017 08:37:04 GMT

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



Thanks for the cleanup for these. I think we are currently being inconsistent in the usage
of CHECK/LOG(FATAL) vs EXIT, especially in handling local filesystem operation failures. This
is mostly due to historical reaons I think: we used to just simply CHECK_SOME everywhere and
later changed individual places / started to use EXIT for local filesystem failures.

To keep the rules simple we should probably consistently use EXIT when we expect that things
can possibly fail and only use CHECK/LOG(FATAL) when it's unexpected and result of local logic
and thus would benefit from the backtrace in investigation.

For filesystem operations when we change something for consistency, let's change the ones
with CHECK/CHECK_SOME to EXIT.


src/slave/slave.cpp
Lines 248-250 (original), 248-251 (patched)
<https://reviews.apache.org/r/56680/#comment241380>

    In places like this where we replace EXIT with a LOG(ERROR) and exit, I feel we don't
need it anymore if we have /r/56681/?



src/slave/slave.cpp
Lines 413-414 (original), 427-429 (patched)
<https://reviews.apache.org/r/56680/#comment241512>

    Ditto (all occurrences above)



src/slave/slave.cpp
Lines 418-419 (original), 433-439 (patched)
<https://reviews.apache.org/r/56680/#comment241655>

    As I mentioned in the review header, it's probably good to use EXIT.
    
    So +1 on this.



src/slave/slave.cpp
Lines 423-424 (original), 443-445 (patched)
<https://reviews.apache.org/r/56680/#comment241513>

    Keep EXIT.



src/slave/slave.cpp
Lines 442-444 (original), 463-466 (patched)
<https://reviews.apache.org/r/56680/#comment241516>

    Ditto.



src/slave/slave.cpp
Lines 496-497 (original), 522-524 (patched)
<https://reviews.apache.org/r/56680/#comment241657>

    Ditto here and occurrences above.



src/slave/slave.cpp
Line 514 (original), 540-541 (patched)
<https://reviews.apache.org/r/56680/#comment241517>

    +1 on replacing LOG(FATAL) with EXIT but s/EXIT_SUCCESS/EXIT_FAILURE/ ?



src/slave/slave.cpp
Lines 773-775 (original), 800-803 (patched)
<https://reviews.apache.org/r/56680/#comment241520>

    Keep EXIT.



src/slave/slave.cpp
Lines 788-789 (original), 817-819 (patched)
<https://reviews.apache.org/r/56680/#comment241518>

    Ditto here and the occurrence above.



src/slave/slave.cpp
Line 914 (original), 944 (patched)
<https://reviews.apache.org/r/56680/#comment241384>

    External conditions can result in this failure (which doesn't suggest anything unexpected)
so EXIT makes more sense.



src/slave/slave.cpp
Lines 1013-1015 (original), 1043-1045 (patched)
<https://reviews.apache.org/r/56680/#comment241552>

    Module creation could fail due to incorrect config EXIT with the error message makes more
sense.



src/slave/slave.cpp
Line 1084 (original), 1114 (patched)
<https://reviews.apache.org/r/56680/#comment241742>

    Keep EXIT.



src/slave/slave.cpp
Line 1182 (original), 1212 (patched)
<https://reviews.apache.org/r/56680/#comment241554>

    Keep EXIT. This situation although unexpected, is likely not a local error that could
benefit from the stacktrace.



src/slave/slave.cpp
Line 1229 (original), 1259 (patched)
<https://reviews.apache.org/r/56680/#comment241746>

    Ditto.



src/slave/slave.cpp
Lines 2923-2940 (original), 2953-2964 (patched)
<https://reviews.apache.org/r/56680/#comment241386>

    Instead of changing EXIT to CHECK_SOME, we could change CHECK_SOME to EXIT. The practice
of using of `CHECK_SOME` on checkpoints is before started to use EXIT for more graceful exit
messages. Inconsistency does exist for this case elsewhere but to make this method more consistency
let's follow the general principle of not printing backtrace in cases where failures are due
to external conditions.



src/slave/slave.cpp
Lines 5546-5552 (original), 5570-5577 (patched)
<https://reviews.apache.org/r/56680/#comment241654>

    Keep EXIT.


- Jiang Yan Xu


On Feb. 14, 2017, 1:05 p.m., James Peach wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/56680/
> -----------------------------------------------------------
> 
> (Updated Feb. 14, 2017, 1:05 p.m.)
> 
> 
> Review request for mesos and haosdent huang.
> 
> 
> Bugs: MESOS-7115
>     https://issues.apache.org/jira/browse/MESOS-7115
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> EXIT(...) doesn't format messages like glog so it's less easy to
> tell where a message came from and when it was emitted. For agent all
> initialization messages, perform a LOG(ERROR) followe by an exit. For
> other agent exit scenarios, use LOG(FATAL) or the equivalent CHECK().
> 
> 
> Diffs
> -----
> 
>   src/slave/slave.cpp ebba8e16bc9ec45781183e78cb5a3c351a5f65f5 
> 
> 
> Diff: https://reviews.apache.org/r/56680/diff/1/
> 
> 
> Testing
> -------
> 
> make check (Fedora 25)
> 
> 
> Thanks,
> 
> James Peach
> 
>


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