Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2277803005:
Logging for RestartJob. (Closed)
|
Created:
4 years, 4 months ago by achuithb Modified:
4 years, 3 months ago Reviewers:
Daniel Erat CC:
chromium-reviews, oshima+watch_chromium.org, hashimoto+watch_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionLogging for RestartJob.
BUG=chromium:631640
TEST=manual
Committed: https://crrev.com/d21cccd0d4270ea81ce31cc146a3660c6e631b73
Cr-Commit-Position: refs/heads/master@{#414609}
Patch Set 1 #Patch Set 2 : . #
Total comments: 4
Patch Set 3 : Dan feedback #Messages
Total messages: 22 (15 generated)
The CQ bit was checked by achuith@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
The CQ bit was checked by achuith@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
achuith@chromium.org changed reviewers: + derat@chromium.org
Dan, PTAL
https://codereview.chromium.org/2277803005/diff/20001/chromeos/dbus/session_m... File chromeos/dbus/session_manager_client.cc (right): https://codereview.chromium.org/2277803005/diff/20001/chromeos/dbus/session_m... chromeos/dbus/session_manager_client.cc:87: PLOG(ERROR) << "DBus error " << response->ToString(); LOG(ERROR) is the right thing to use here. PLOG is only appropriate when a posix call (read(), write(), fork(), exec(), etc.) failed immediately before and you want to include the reason why. you don't know what the d-bus library called immediately prior to handing you the error (and in this particular case, it probably did a successful read() of the error message from a socket or something similar to that, so PLOG would actually log "Success"). https://codereview.chromium.org/2277803005/diff/20001/chromeos/dbus/session_m... chromeos/dbus/session_manager_client.cc:530: << login_manager::kSessionManagerRestartJob; do you also want to add temporary logging on success here?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by achuith@chromium.org to run a CQ dry run
PTAL Dan https://codereview.chromium.org/2277803005/diff/20001/chromeos/dbus/session_m... File chromeos/dbus/session_manager_client.cc (right): https://codereview.chromium.org/2277803005/diff/20001/chromeos/dbus/session_m... chromeos/dbus/session_manager_client.cc:87: PLOG(ERROR) << "DBus error " << response->ToString(); On 2016/08/25 02:13:19, Daniel Erat wrote: > LOG(ERROR) is the right thing to use here. PLOG is only appropriate when a posix > call (read(), write(), fork(), exec(), etc.) failed immediately before and you > want to include the reason why. you don't know what the d-bus library called > immediately prior to handing you the error (and in this particular case, it > probably did a successful read() of the error message from a socket or something > similar to that, so PLOG would actually log "Success"). Done. Thanks for the explanation. https://codereview.chromium.org/2277803005/diff/20001/chromeos/dbus/session_m... chromeos/dbus/session_manager_client.cc:530: << login_manager::kSessionManagerRestartJob; On 2016/08/25 02:13:19, Daniel Erat wrote: > do you also want to add temporary logging on success here? Done.
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm
The CQ bit was checked by achuith@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Committed patchset #3 (id:40001)
Message was sent while issue was closed.
Description was changed from ========== Logging for RestartJob. BUG=chromium:631640 TEST=manual ========== to ========== Logging for RestartJob. BUG=chromium:631640 TEST=manual Committed: https://crrev.com/d21cccd0d4270ea81ce31cc146a3660c6e631b73 Cr-Commit-Position: refs/heads/master@{#414609} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/d21cccd0d4270ea81ce31cc146a3660c6e631b73 Cr-Commit-Position: refs/heads/master@{#414609} |
