DeviceUtils: Make Install and PushChangedFiles respect instance retries / timeout

DeviceUtils: Make Install and PushChangedFiles respect instance retries / timeout

If you create a DeviceUtils with default_retries=1, then all methods should have
retries=1 as the default value for the parameter. Before this change, Install*,
PushChangedFiles, and Reboot would retry twice regardless of the constructor's
default_retries value.

BUG=

Committed: https://crrev.com/fbe71fac40444fcf4b25493ec1e0ab3e06975d3e
Cr-Commit-Position: refs/heads/master@{#348832}

[agrieve, 2015-09-11 13:56:26]

agrieve@chromium.org changed reviewers:
+ perezju@chromium.org

[agrieve, 2015-09-11 13:56:26]

I was hitting this in incremental_install.py, where when Install was failing, it
was retrying when I had specified retries=1 in the DeviceUtils() constructor.

[jbudorick, 2015-09-11 14:00:56]

jbudorick@chromium.org changed reviewers:
+ jbudorick@chromium.org

[jbudorick, 2015-09-11 14:00:56]

https://codereview.chromium.org/1340603002/diff/1/build/android/devil/android...
File build/android/devil/android/device_utils.py (right):

https://codereview.chromium.org/1340603002/diff/1/build/android/devil/android...
build/android/devil/android/device_utils.py:490:
@decorators.WithTimeoutAndRetriesDefaults(
Reboot has the same problem. We should probably use min_default_timeout here,
too.

[jbudorick, 2015-09-11 15:29:52]

lgtm

[agrieve, 2015-09-11 15:44:28]

https://codereview.chromium.org/1340603002/diff/1/build/android/devil/android...
File build/android/devil/android/device_utils.py (right):

https://codereview.chromium.org/1340603002/diff/1/build/android/devil/android...
build/android/devil/android/device_utils.py:490:
@decorators.WithTimeoutAndRetriesDefaults(
On 2015/09/11 14:00:56, jbudorick wrote:
> Reboot has the same problem. We should probably use min_default_timeout here,
> too.

Done.

[agrieve, 2015-09-11 15:44:38]

The CQ bit was checked by agrieve@chromium.org

[commit-bot: I haz the power, 2015-09-11 15:45:05]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1340603002/20001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1340603002/20001

[commit-bot: I haz the power, 2015-09-11 18:47:10]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-09-11 18:47:11]

Try jobs failed on following builders:
  linux_android_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_android_r...)

[perezju, 2015-09-14 08:27:38]

Could you write a more detailed description in the commit message? I still don't
understand why the previous approach was not working as expected, and what the
fix is actually doing. For example, is this disabling retries for all of these
methods, except if requested explicitly?

[agrieve, 2015-09-14 14:34:53]

On 2015/09/14 08:27:38, perezju wrote:
> Could you write a more detailed description in the commit message? I still
don't
> understand why the previous approach was not working as expected, and what the
> fix is actually doing. For example, is this disabling retries for all of these
> methods, except if requested explicitly?

Elaborated with an example.

[perezju, 2015-09-14 15:56:36]

On 2015/09/14 14:34:53, agrieve wrote:
> On 2015/09/14 08:27:38, perezju wrote:
> > Could you write a more detailed description in the commit message? I still
> don't
> > understand why the previous approach was not working as expected, and what
the
> > fix is actually doing. For example, is this disabling retries for all of
these
> > methods, except if requested explicitly?
> 
> Elaborated with an example.

I see. So the plan is to deprecate @decorators.WithTimeoutAndRetriesDefaults in
favor of the new "min_default_timeout"? Or do we see some cases where
WithTimeoutAndRetriesDefaults might still be needed? If the former, could we
either remove the code or add a deprecated notice to the implementation of
this decorator?

[agrieve, 2015-09-14 15:57:48]

On 2015/09/14 15:56:36, perezju wrote:
> On 2015/09/14 14:34:53, agrieve wrote:
> > On 2015/09/14 08:27:38, perezju wrote:
> > > Could you write a more detailed description in the commit message? I still
> > don't
> > > understand why the previous approach was not working as expected, and what
> the
> > > fix is actually doing. For example, is this disabling retries for all of
> these
> > > methods, except if requested explicitly?
> > 
> > Elaborated with an example.
> 
> I see. So the plan is to deprecate @decorators.WithTimeoutAndRetriesDefaults
in
> favor of the new "min_default_timeout"? Or do we see some cases where
> WithTimeoutAndRetriesDefaults might still be needed? If the former, could we
> either remove the code or add a deprecated notice to the implementation of
> this decorator?

The non-instance decorator is still used & useful for non-instance methods.

[jbudorick, 2015-09-14 15:57:55]

On 2015/09/14 at 15:56:36, perezju wrote:
> On 2015/09/14 14:34:53, agrieve wrote:
> > On 2015/09/14 08:27:38, perezju wrote:
> > > Could you write a more detailed description in the commit message? I still
> > don't
> > > understand why the previous approach was not working as expected, and what
the
> > > fix is actually doing. For example, is this disabling retries for all of
these
> > > methods, except if requested explicitly?
> > 
> > Elaborated with an example.
> 
> I see. So the plan is to deprecate @decorators.WithTimeoutAndRetriesDefaults
in
> favor of the new "min_default_timeout"? Or do we see some cases where
> WithTimeoutAndRetriesDefaults might still be needed? If the former, could we
> either remove the code or add a deprecated notice to the implementation of
> this decorator?

there's still one legal client:
https://code.google.com/p/chromium/codesearch#chromium/src/build/android/devi...

[perezju, 2015-09-14 16:01:36]

On 2015/09/14 15:57:48, agrieve wrote:
> On 2015/09/14 15:56:36, perezju wrote:
> > On 2015/09/14 14:34:53, agrieve wrote:
> > > On 2015/09/14 08:27:38, perezju wrote:
> > > > Could you write a more detailed description in the commit message? I
still
> > > don't
> > > > understand why the previous approach was not working as expected, and
what
> > the
> > > > fix is actually doing. For example, is this disabling retries for all of
> > these
> > > > methods, except if requested explicitly?
> > > 
> > > Elaborated with an example.
> > 
> > I see. So the plan is to deprecate @decorators.WithTimeoutAndRetriesDefaults
> in
> > favor of the new "min_default_timeout"? Or do we see some cases where
> > WithTimeoutAndRetriesDefaults might still be needed? If the former, could we
> > either remove the code or add a deprecated notice to the implementation of
> > this decorator?
> 
> The non-instance decorator is still used & useful for non-instance methods.

ok lgtm then. thanks for updating the description

[agrieve, 2015-09-14 19:01:46]

The CQ bit was checked by agrieve@chromium.org

[commit-bot: I haz the power, 2015-09-14 19:02:44]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1340603002/20001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1340603002/20001

[commit-bot: I haz the power, 2015-09-14 22:59:29]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-09-14 22:59:30]

Try jobs failed on following builders:
  linux_android_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_android_r...)

[jbudorick, 2015-09-14 23:22:52]

https://codereview.chromium.org/1340603002/diff/20001/build/android/devil/and...
File build/android/devil/android/decorators.py (right):

https://codereview.chromium.org/1340603002/diff/20001/build/android/devil/and...
build/android/devil/android/decorators.py:142: ret = min(min_default_timeout,
ret)
er, this should be max(min_default_timeout, ret) s.t. if min_default_timeout >
ret, we use that, otherwise we use ret

[agrieve, 2015-09-15 00:53:46]

https://codereview.chromium.org/1340603002/diff/20001/build/android/devil/and...
File build/android/devil/android/decorators.py (right):

https://codereview.chromium.org/1340603002/diff/20001/build/android/devil/and...
build/android/devil/android/decorators.py:142: ret = min(min_default_timeout,
ret)
On 2015/09/14 23:22:52, jbudorick wrote:
> er, this should be max(min_default_timeout, ret) s.t. if min_default_timeout >
> ret, we use that, otherwise we use ret

Aha! Thank you! Saw test failing due to timeout, figured something was wrong,
and just couldn't figure it out for some reason. :S.

[agrieve, 2015-09-15 00:53:56]

The CQ bit was checked by agrieve@chromium.org

[agrieve, 2015-09-15 00:53:56]

The patchset sent to the CQ was uploaded after l-g-t-m from
perezju@chromium.org, jbudorick@chromium.org
Link to the patchset: https://codereview.chromium.org/1340603002/#ps40001
(title: "min->max")

[commit-bot: I haz the power, 2015-09-15 00:54:33]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1340603002/40001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1340603002/40001

[commit-bot: I haz the power, 2015-09-15 03:36:31]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2015-09-15 03:37:04]

Patchset 3 (id:??) landed as
https://crrev.com/fbe71fac40444fcf4b25493ec1e0ab3e06975d3e
Cr-Commit-Position: refs/heads/master@{#348832}

[commit-bot: I haz the power, 2015-09-23 12:42:24]

Patchset 3 (id:??) landed as
https://crrev.com/fbe71fac40444fcf4b25493ec1e0ab3e06975d3e
Cr-Commit-Position: refs/heads/master@{#348832}