Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
651843007:
Intelligently handle a device with critically low battery. (Closed)
DescriptionIntelligently handle a device with critically low battery.
A device with a critically low battery gets listed as a bad device. The device
status check only fails if all devices are bad. Otherwise, it will continue with
the good devices.
BUG=423709
Committed: https://crrev.com/e49a5ce4fbcd3651db68bd7f27c89418a4a3c197
Cr-Commit-Position: refs/heads/master@{#300982}
Patch Set 1 #
Total comments: 7
Patch Set 2 : Use += 1. #Patch Set 3 : Make sure charging is enabled on low battery devices. #
Total comments: 6
Patch Set 4 : Fix nits. #Messages
Total messages: 12 (2 generated)
navabi@google.com changed reviewers: + jbudorick@chromium.org, perezju@chromium.org, zty@chromium.org
https://codereview.chromium.org/651843007/diff/1/build/android/buildbot/bb_de... File build/android/buildbot/bb_device_status_check.py (right): https://codereview.chromium.org/651843007/diff/1/build/android/buildbot/bb_de... build/android/buildbot/bb_device_status_check.py:384: device_blacklist.ExtendBlacklist([str(devices[index])]) We may want to do something like what provision_devices does + try to enable charging if we can. https://codereview.chromium.org/651843007/diff/1/build/android/buildbot/bb_de... build/android/buildbot/bb_device_status_check.py:385: num_failed_devs = num_failed_devs + 1 nit: num_failed_devs += 1 https://codereview.chromium.org/651843007/diff/1/build/android/buildbot/bb_de... build/android/buildbot/bb_device_status_check.py:386: index = index + 1 nit: index += 1
https://codereview.chromium.org/651843007/diff/1/build/android/buildbot/bb_de... File build/android/buildbot/bb_device_status_check.py (right): https://codereview.chromium.org/651843007/diff/1/build/android/buildbot/bb_de... build/android/buildbot/bb_device_status_check.py:384: device_blacklist.ExtendBlacklist([str(devices[index])]) On 2014/10/23 01:19:32, jbudorick wrote: > We may want to do something like what provision_devices does + try to enable > charging if we can. I'm actually not sure I like what provision_devices does. It waits around to charge the device. If you're waiting for a build to go through, you have no idea how long you need to wait. And if a device has problems charging, it could never charge enough (since the requirement is 95%). WDYT, John? Do you like the way provision waits around for the devices to charge? This way the device is not used for the build, all the while it's charging, and every build will remove the bad_devices file and give it another chance. https://codereview.chromium.org/651843007/diff/1/build/android/buildbot/bb_de... build/android/buildbot/bb_device_status_check.py:385: num_failed_devs = num_failed_devs + 1 On 2014/10/23 01:19:31, jbudorick wrote: > nit: num_failed_devs += 1 Done. https://codereview.chromium.org/651843007/diff/1/build/android/buildbot/bb_de... build/android/buildbot/bb_device_status_check.py:386: index = index + 1 On 2014/10/23 01:19:32, jbudorick wrote: > nit: index += 1 Done.
https://codereview.chromium.org/651843007/diff/1/build/android/buildbot/bb_de... File build/android/buildbot/bb_device_status_check.py (right): https://codereview.chromium.org/651843007/diff/1/build/android/buildbot/bb_de... build/android/buildbot/bb_device_status_check.py:384: device_blacklist.ExtendBlacklist([str(devices[index])]) On 2014/10/23 01:27:27, navabi wrote: > On 2014/10/23 01:19:32, jbudorick wrote: > > We may want to do something like what provision_devices does + try to enable > > charging if we can. > > I'm actually not sure I like what provision_devices does. It waits around to > charge the device. If you're waiting for a build to go through, you have no idea > how long you need to wait. > > And if a device has problems charging, it could never charge enough (since the > requirement is 95%). WDYT, John? Do you like the way provision waits around for > the devices to charge? > I don't like it, but I can understand why the perf bots do it. There are probably better ways to achieve what they're doing, but those would be out of the scope of this review. I should've been more clear in my original comment. I think we should enable charging if possible before adding the device to the blacklist. I'm fine with blacklisting instead of waiting around for the device to charge. (basically, this if block: https://code.google.com/p/chromium/codesearch#chromium/src/build/android/prov... but not the containing while loop) > This way the device is not used for the build, all the while it's charging, and > every build will remove the bad_devices file and give it another chance.
Thanks jbudorick for the input. PTAL. https://codereview.chromium.org/651843007/diff/40001/build/android/buildbot/b... File build/android/buildbot/bb_device_status_check.py (left): https://codereview.chromium.org/651843007/diff/40001/build/android/buildbot/b... build/android/buildbot/bb_device_status_check.py:106: device_adb.old_interface.Shutdown() we are no longer trying to turn the device off. Instead it will be blacklisted for this build. (I think this wasn't working anyway, i.e. I don't think devices were being turned off).
lgtm w/ nits https://codereview.chromium.org/651843007/diff/40001/build/android/buildbot/b... File build/android/buildbot/bb_device_status_check.py (left): https://codereview.chromium.org/651843007/diff/40001/build/android/buildbot/b... build/android/buildbot/bb_device_status_check.py:106: device_adb.old_interface.Shutdown() On 2014/10/23 21:13:41, navabi wrote: > we are no longer trying to turn the device off. Instead it will be blacklisted > for this build. (I think this wasn't working anyway, i.e. I don't think devices > were being turned off). sgtm https://codereview.chromium.org/651843007/diff/40001/build/android/buildbot/b... File build/android/buildbot/bb_device_status_check.py (right): https://codereview.chromium.org/651843007/diff/40001/build/android/buildbot/b... build/android/buildbot/bb_device_status_check.py:98: if not device_adb.old_interface.IsDeviceCharging(): nit: Can't we do this in the first if battery_level < 15 block? https://codereview.chromium.org/651843007/diff/40001/build/android/buildbot/b... build/android/buildbot/bb_device_status_check.py:378: for fail_status in fail_step_lst: nit: you could get rid of index with something like this: for fail_status, device in itertools.izip(fail_step_lst, devices): if not fail_status: device_blacklist.ExtendBlacklist([str(device)]) num_failed_devs += 1
https://codereview.chromium.org/651843007/diff/40001/build/android/buildbot/b... File build/android/buildbot/bb_device_status_check.py (right): https://codereview.chromium.org/651843007/diff/40001/build/android/buildbot/b... build/android/buildbot/bb_device_status_check.py:98: if not device_adb.old_interface.IsDeviceCharging(): On 2014/10/23 21:36:58, jbudorick wrote: > nit: Can't we do this in the first if battery_level < 15 block? Done. https://codereview.chromium.org/651843007/diff/40001/build/android/buildbot/b... build/android/buildbot/bb_device_status_check.py:378: for fail_status in fail_step_lst: On 2014/10/23 21:36:58, jbudorick wrote: > nit: you could get rid of index with something like this: > > for fail_status, device in itertools.izip(fail_step_lst, devices): > if not fail_status: > device_blacklist.ExtendBlacklist([str(device)]) > num_failed_devs += 1 Done. (i used zip instead of itertools.izip)
The CQ bit was checked by navabi@google.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/651843007/60001
Message was sent while issue was closed.
Committed patchset #4 (id:60001)
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/e49a5ce4fbcd3651db68bd7f27c89418a4a3c197 Cr-Commit-Position: refs/heads/master@{#300982} |
