Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1522013002:
Add minimum battery level to adb_wait_for_device (Closed)
DescriptionAdd minimum battery level to adb_wait_for_device
This should prevent the Android bots running out of battery at the
expense of extra time spent waiting at the end of the build.
BUG=skia:4606
Committed: https://skia.googlesource.com/skia/+/0b29b728cae6c3f3738b4cda0b786960d05dda2b
Patch Set 1 #
Total comments: 9
Patch Set 2 : "Return" the battery level, better error handling #Patch Set 3 : Remove unnecessary line #Messages
Total messages: 13 (6 generated)
borenet@google.com changed reviewers: + rmistry@google.com
Description was changed from ========== Add minimum battery level to adb_wait_for_device This should prevent the Android bots running out of battery at the expense of extra time spent waiting at the end of the build. BUG=skia:4606 ========== to ========== Add minimum battery level to adb_wait_for_device This should prevent the Android bots running out of battery at the expense of extra time spent waiting at the end of the build. BUG=skia:4606 ==========
borenet@google.com changed reviewers: + djsollen@google.com
https://codereview.chromium.org/1522013002/diff/1/platform_tools/android/bin/... File platform_tools/android/bin/adb_wait_for_device (right): https://codereview.chromium.org/1522013002/diff/1/platform_tools/android/bin/... platform_tools/android/bin/adb_wait_for_device:33: get_battery_level Working on the global BATTERY_LEVEL is confusing. Why not have get_battery level return the battery level? i.e. current_battery_level = get_battery_level https://codereview.chromium.org/1522013002/diff/1/platform_tools/android/bin/... platform_tools/android/bin/adb_wait_for_device:36: sleep 5 5 seconds seems like a very short time to wait for battery level to rise by any amount. Should this instead be 30seconds? or 1min? or more?
https://codereview.chromium.org/1522013002/diff/1/platform_tools/android/bin/... File platform_tools/android/bin/adb_wait_for_device (right): https://codereview.chromium.org/1522013002/diff/1/platform_tools/android/bin/... platform_tools/android/bin/adb_wait_for_device:33: get_battery_level On 2015/12/14 13:11:27, rmistry wrote: > Working on the global BATTERY_LEVEL is confusing. Why not have get_battery level > return the battery level? i.e. > > current_battery_level = get_battery_level I'm not aware of a way to do that in a bash script. Changed to have the function echo its "return" value and the caller to read that. https://codereview.chromium.org/1522013002/diff/1/platform_tools/android/bin/... platform_tools/android/bin/adb_wait_for_device:36: sleep 5 On 2015/12/14 13:11:27, rmistry wrote: > 5 seconds seems like a very short time to wait for battery level to rise by any > amount. Should this instead be 30seconds? or 1min? or more? The work we're doing every 5 seconds is cheap, and running more often prevents us from wasting time when we could have already started the next build. IMO there's no reason to wait longer between checks.
lgtm https://codereview.chromium.org/1522013002/diff/1/platform_tools/android/bin/... File platform_tools/android/bin/adb_wait_for_device (right): https://codereview.chromium.org/1522013002/diff/1/platform_tools/android/bin/... platform_tools/android/bin/adb_wait_for_device:9: BATTERY_LEVEL=0 Remove. https://codereview.chromium.org/1522013002/diff/1/platform_tools/android/bin/... platform_tools/android/bin/adb_wait_for_device:33: get_battery_level On 2015/12/14 13:26:56, borenet wrote: > On 2015/12/14 13:11:27, rmistry wrote: > > Working on the global BATTERY_LEVEL is confusing. Why not have get_battery > level > > return the battery level? i.e. > > > > current_battery_level = get_battery_level > > I'm not aware of a way to do that in a bash script. Changed to have the > function echo its "return" value and the caller to read that. Yes that is how you return values from bash functions. Are the quotes here and below needed? https://codereview.chromium.org/1522013002/diff/1/platform_tools/android/bin/... platform_tools/android/bin/adb_wait_for_device:36: sleep 5 On 2015/12/14 13:26:56, borenet wrote: > On 2015/12/14 13:11:27, rmistry wrote: > > 5 seconds seems like a very short time to wait for battery level to rise by > any > > amount. Should this instead be 30seconds? or 1min? or more? > > The work we're doing every 5 seconds is cheap, and running more often prevents > us from wasting time when we could have already started the next build. IMO > there's no reason to wait longer between checks. Sounds good since the calls are cheap. Can always revisit later by looking at the logs to see how long these actually end up waiting.
https://codereview.chromium.org/1522013002/diff/1/platform_tools/android/bin/... File platform_tools/android/bin/adb_wait_for_device (right): https://codereview.chromium.org/1522013002/diff/1/platform_tools/android/bin/... platform_tools/android/bin/adb_wait_for_device:9: BATTERY_LEVEL=0 On 2015/12/14 13:35:05, rmistry wrote: > Remove. Done. https://codereview.chromium.org/1522013002/diff/1/platform_tools/android/bin/... platform_tools/android/bin/adb_wait_for_device:33: get_battery_level On 2015/12/14 13:35:05, rmistry wrote: > On 2015/12/14 13:26:56, borenet wrote: > > On 2015/12/14 13:11:27, rmistry wrote: > > > Working on the global BATTERY_LEVEL is confusing. Why not have get_battery > > level > > > return the battery level? i.e. > > > > > > current_battery_level = get_battery_level > > > > I'm not aware of a way to do that in a bash script. Changed to have the > > function echo its "return" value and the caller to read that. > > Yes that is how you return values from bash functions. > Are the quotes here and below needed? No, but they don't hurt anything, and my bash paranoia requires me to always use them.
The CQ bit was checked by borenet@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from rmistry@google.com Link to the patchset: https://codereview.chromium.org/1522013002/#ps40001 (title: "Remove unnecessary line")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1522013002/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1522013002/40001
Message was sent while issue was closed.
Description was changed from ========== Add minimum battery level to adb_wait_for_device This should prevent the Android bots running out of battery at the expense of extra time spent waiting at the end of the build. BUG=skia:4606 ========== to ========== Add minimum battery level to adb_wait_for_device This should prevent the Android bots running out of battery at the expense of extra time spent waiting at the end of the build. BUG=skia:4606 Committed: https://skia.googlesource.com/skia/+/0b29b728cae6c3f3738b4cda0b786960d05dda2b ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001) as https://skia.googlesource.com/skia/+/0b29b728cae6c3f3738b4cda0b786960d05dda2b |
