You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@mesos.apache.org by James Peach <jp...@apache.org> on 2017/02/14 21:05:06 UTC
Review Request 56680: Apply glog to agent exit messages.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/56680/
-----------------------------------------------------------
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/
Testing
-------
make check (Fedora 25)
Thanks,
James Peach
Re: Review Request 56680: Apply glog to agent exit messages.
Posted by Jiang Yan Xu <ya...@jxu.me>.
-----------------------------------------------------------
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
>
>
Re: Review Request 56680: Use the EXIT() macro more consistently in
agent startup.
Posted by James Peach <jp...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/56680/
-----------------------------------------------------------
(Updated March 20, 2017, 5:50 p.m.)
Review request for mesos and haosdent huang.
Summary (updated)
-----------------
Use the EXIT() macro more consistently in agent startup.
Bugs: MESOS-7115
https://issues.apache.org/jira/browse/MESOS-7115
Repository: mesos
Description (updated)
-------
Use the EXIT() macro more consistently in agent startup.
Diffs (updated)
-----
src/slave/slave.cpp a4f4a9ca80b726de8e07571fd6d93120947c278b
Diff: https://reviews.apache.org/r/56680/diff/2/
Changes: https://reviews.apache.org/r/56680/diff/1-2/
Testing
-------
make check (Fedora 25)
Thanks,
James Peach