Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
265743002:
[Android] Switch to new interfaces of GetAVDs and RestartAdbServer. (Closed)
|
Created:
6 years, 7 months ago by jbudorick Modified:
6 years, 7 months ago CC:
chromium-reviews, klundberg+watch_chromium.org, bulach+watch_chromium.org, yfriedman+watch_chromium.org, ilevy-cc_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Visibility:
Public. |
Description[Android] Switch to new interfaces of GetAVDs and RestartAdbServer.
BUG=267773
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=270336
Patch Set 1 #Patch Set 2 : #
Total comments: 2
Patch Set 3 : #
Total comments: 3
Patch Set 4 : back to decorators. #
Total comments: 4
Patch Set 5 : rebase + nitfixes #
Total comments: 5
Patch Set 6 : #Patch Set 7 : #Patch Set 8 : #Messages
Total messages: 44 (0 generated)
https://codereview.chromium.org/265743002/diff/20001/build/android/pylib/devi... File build/android/pylib/device/device_utils.py (right): https://codereview.chromium.org/265743002/diff/20001/build/android/pylib/devi... build/android/pylib/device/device_utils.py:37: kwargs['retries'] = retries nit: how about: timeout = kwargs['timeout'] = kwargs.get('timeout', default_timeout) https://codereview.chromium.org/265743002/diff/20001/build/android/pylib/devi... build/android/pylib/device/device_utils.py:59: @WithTimeoutAndRetries(_DEFAULT_TIMEOUT, _DEFAULT_RETRIES) to be very honest... :) this is neat, but I think it just adds extra indirections.. I'd rather have slightly more code, but more explicit everywhere (remember, this will be running in the bots, so the more explicit, the simpler it'll be the debug the eventual error messages, etc...). as such, I'd suggest having a straightforward helper: def RunWithTimeoutAndRetries(callable, timeout, retries): while ... callable() ... then: def GetAVDs(timeout=_DEFAULT_TIMEOUT, retries=_DEFAULT_RETRIES): return RunWithTimeoutAndRetries(pylib.android_commands.GetAVDs, timeout, retries) this would be more straightforward, and I bet error messages would be less convoluted. also, the RunWithTimeoutAndRetries takes a callable, so it can be just as easily tested. wdyt? of course, I may be missing the advantages of using out-of-band annotations, so please let me know! :)
On 2014/05/01 11:18:57, bulach wrote: > https://codereview.chromium.org/265743002/diff/20001/build/android/pylib/devi... > File build/android/pylib/device/device_utils.py (right): > > https://codereview.chromium.org/265743002/diff/20001/build/android/pylib/devi... > build/android/pylib/device/device_utils.py:37: kwargs['retries'] = retries > nit: how about: > timeout = kwargs['timeout'] = kwargs.get('timeout', default_timeout) > > https://codereview.chromium.org/265743002/diff/20001/build/android/pylib/devi... > build/android/pylib/device/device_utils.py:59: > @WithTimeoutAndRetries(_DEFAULT_TIMEOUT, _DEFAULT_RETRIES) > to be very honest... :) > I prefer code reviews that way. :) > this is neat, but I think it just adds extra indirections.. I'd rather have > slightly more code, but more explicit everywhere (remember, this will be running > in the bots, so the more explicit, the simpler it'll be the debug the eventual > error messages, etc...). > > as such, I'd suggest having a straightforward helper: > > def RunWithTimeoutAndRetries(callable, timeout, retries): > while ... > callable() ... > > then: > def GetAVDs(timeout=_DEFAULT_TIMEOUT, retries=_DEFAULT_RETRIES): > return RunWithTimeoutAndRetries(pylib.android_commands.GetAVDs, timeout, > retries) > > this would be more straightforward, and I bet error messages would be less > convoluted. also, the RunWithTimeoutAndRetries takes a callable, so it can be > just as easily tested. > > wdyt? > > of course, I may be missing the advantages of using out-of-band annotations, so > please let me know! :) The advantage is mostly one of brevity. I can see how the debugging concern could outweigh that, though. (The method version of this will wind up looking a little nastier.)
As anticipated, the member version is a bit uglier than the function version because we have to manually load the default values from the instance (rather than having them set in the parameter declaration). The updated tests provide a good example. I don't consider that to be a deal breaker, though.
On 2014/05/01 15:53:39, jbudorick wrote: > As anticipated, the member version is a bit uglier than the function version > because we have to manually load the default values from the instance (rather > than having them set in the parameter declaration). The updated tests provide a > good example. > > I don't consider that to be a deal breaker, though. I would actually much prefer the annotations in this particular case. Annotations are really just functions being called by a different syntax to keep the logic clean, so the stack traces are just as readable (especially if @functools.update_wrapper is used). It makes the actual business logic much more readable, which is where the bugs are likely to actually be. It's also next to impossible to support per-instance defaults cleanly without using the annotations.
On 2014/05/01 16:03:25, craigdh wrote: > On 2014/05/01 15:53:39, jbudorick wrote: > > As anticipated, the member version is a bit uglier than the function version > > because we have to manually load the default values from the instance (rather > > than having them set in the parameter declaration). The updated tests provide > a > > good example. > > > > I don't consider that to be a deal breaker, though. > > I would actually much prefer the annotations in this particular case. > Annotations are really just functions being called by a different syntax to keep > the logic clean, so the stack traces are just as readable (especially if > @functools.update_wrapper is used). It makes the actual business logic much more > readable, which is where the bugs are likely to actually be. It's also next to > impossible to support per-instance defaults cleanly without using the > annotations. whoops... too much Java... s/annotation/decorator/
https://codereview.chromium.org/265743002/diff/40001/build/android/pylib/devi... File build/android/pylib/device/device_utils.py (right): https://codereview.chromium.org/265743002/diff/40001/build/android/pylib/devi... build/android/pylib/device/device_utils.py:22: def RunWithTimeoutAndRetries(f, *args, **kwargs): There's also something similar in AdbWrapper. Can you create a helper class for this? https://codereview.chromium.org/265743002/diff/40001/build/android/pylib/devi... build/android/pylib/device/device_utils.py:61: def RestartServer(timeout=_DEFAULT_TIMEOUT, retries=_DEFAULT_RETRIES): RestartServer -> RestartAdbServer https://codereview.chromium.org/265743002/diff/40001/build/android/pylib/devi... build/android/pylib/device/device_utils.py:79: pylib.android_commands.AndroidCommands().RestartAdbServer() You should think of thread-safety for any method you add here.
On 2014/05/01 18:31:01, frankf wrote: > https://codereview.chromium.org/265743002/diff/40001/build/android/pylib/devi... > File build/android/pylib/device/device_utils.py (right): > > https://codereview.chromium.org/265743002/diff/40001/build/android/pylib/devi... > build/android/pylib/device/device_utils.py:22: def RunWithTimeoutAndRetries(f, > *args, **kwargs): > There's also something similar in AdbWrapper. Can you create a helper class for > this? > I can definitely try to make a helper something. Whether it winds up being a decorator, a function, or a class of some sort seems to be unresolved. Like Craig, I think I prefer the decorator. > https://codereview.chromium.org/265743002/diff/40001/build/android/pylib/devi... > build/android/pylib/device/device_utils.py:61: def > RestartServer(timeout=_DEFAULT_TIMEOUT, retries=_DEFAULT_RETRIES): > RestartServer -> RestartAdbServer > > https://codereview.chromium.org/265743002/diff/40001/build/android/pylib/devi... > build/android/pylib/device/device_utils.py:79: > pylib.android_commands.AndroidCommands().RestartAdbServer() > You should think of thread-safety for any method you add here.
On 2014/05/01 18:45:45, jbudorick wrote: > On 2014/05/01 18:31:01, frankf wrote: > > > https://codereview.chromium.org/265743002/diff/40001/build/android/pylib/devi... > > File build/android/pylib/device/device_utils.py (right): > > > > > https://codereview.chromium.org/265743002/diff/40001/build/android/pylib/devi... > > build/android/pylib/device/device_utils.py:22: def RunWithTimeoutAndRetries(f, > > *args, **kwargs): > > There's also something similar in AdbWrapper. Can you create a helper class > for > > this? > > > > I can definitely try to make a helper something. Whether it winds up being a > decorator, a function, or a class of some sort seems to be unresolved. Like > Craig, I think I prefer the decorator. to clarify :) I don't care much about the decorator versus the helper final decision: I'll leave that to you folks, and I'm sure you'll take the right approach whatever that is! let me just expose my points to help clarify: - first and foremost, the decorator makes it a bit more convoluted / adds a learning curve for those not too familiar with it... (I agree people should know about it and can learn if they don't, but still). - on the calling side: there's no way to know if calling "foo.DoSomething()" has magic decorators and how they'd behave.. I'd much rather have "foo.DoSomethingWithRetries(retries=3)" or something equivalent, so it's immediately known at the call site what will happen. - on the implementation side: there are a few extra levels of default'ness, exception handling, etc, etc... either these parameters are really parameters, and every caller should pass them, or they're internal constants within the function itself.. ditto for exception handling: I'd much rather have them explicitly there and then, rather than trying to understand what is being thrown, what the decorator swallows, what would it re-raise, etc. a parallel I'd draw is Telemetry's "Wait": https://code.google.com/p/chromium/codesearch#chromium/src/tools/telemetry/te... we could've parametrized every little thing, but in reality, all callers would either use the default parameters, or would use it wrong :) so instead, there are a few hard-coded constants that make things explicit... also, to further clarify: decorators are *super* useful! https://code.google.com/p/chromium/codesearch#chromium/src/tools/telemetry/te... https://code.google.com/p/chromium/codesearch#chromium/src/build/android/pyli... imho decorators should be used when they "orthogonal" to the actual function, not to change their intrinsic behavior: again, imho, retrying / dealing with exceptions / timing out is an intrinsic behavior. I guess this is where we could argue forever, so I'd stop and leave it to you all. ;) > > > > https://codereview.chromium.org/265743002/diff/40001/build/android/pylib/devi... > > build/android/pylib/device/device_utils.py:61: def > > RestartServer(timeout=_DEFAULT_TIMEOUT, retries=_DEFAULT_RETRIES): > > RestartServer -> RestartAdbServer > > > > > https://codereview.chromium.org/265743002/diff/40001/build/android/pylib/devi... > > build/android/pylib/device/device_utils.py:79: > > pylib.android_commands.AndroidCommands().RestartAdbServer() > > You should think of thread-safety for any method you add here.
@Marcus, you definitely make some good points. I think we're mostly on the same page, except that I'd consider retrying/timeouts to be mostly orthogonal (conceptually) to the behavior of the function. Regardless, we're in complete agreement on the importance of having readable stack traces. @John, would you mind posting an example of a stack trace with decorators and one with helpers when a retry or timeout happens? The best way to decide if decorators are reasonable is probably to compare the actual stack traces people will have to decode.
On 2014/05/02 20:20:53, craigdh wrote:
> @Marcus, you definitely make some good points. I think we're mostly on the
same
> page, except that I'd consider retrying/timeouts to be mostly orthogonal
> (conceptually) to the behavior of the function. Regardless, we're in complete
> agreement on the importance of having readable stack traces.
>
> @John, would you mind posting an example of a stack trace with decorators and
> one with helpers when a retry or timeout happens? The best way to decide if
> decorators are reasonable is probably to compare the actual stack traces
people
> will have to decode.
Example stack traces below. The decorator traces correspond to revision 2, while
the explicit function traces correspond to revision 3. The TL;DR is that there's
no difference in depth.
Decorator (Exception):
Traceback (most recent call last):
File "pylib/device/device_utils_test.py", line 185, in
testViewExceptionStackTrace
test_obj.alwaysRaisesCommandFailedError(retries=1)
File
"/usr/local/google/code/clankium/src/build/android/pylib/device/device_utils.py",
line 51, in wrapper
return WithTimeoutAndRetries(timeout, retries)(f)(device, *args, **kwargs)
File
"/usr/local/google/code/clankium/src/build/android/pylib/device/device_utils.py",
line 36, in wrapper
return timeout_retry.Run(_impl, timeout, retries)
File
"/usr/local/google/code/clankium/src/build/android/pylib/utils/timeout_retry.py",
line 45, in Run
thread_group.JoinAll(watchdog_timer.WatchdogTimer(timeout))
File
"/usr/local/google/code/clankium/src/build/android/pylib/utils/reraiser_thread.py",
line 136, in JoinAll
self._JoinAll(watcher)
File
"/usr/local/google/code/clankium/src/build/android/pylib/utils/reraiser_thread.py",
line 123, in _JoinAll
thread.ReraiseIfException()
File
"/usr/local/google/code/clankium/src/build/android/pylib/utils/reraiser_thread.py",
line 70, in run
self._func(*self._args, **self._kwargs)
File
"/usr/local/google/code/clankium/src/build/android/pylib/utils/timeout_retry.py",
line 37, in RunOnTimeoutThread
ret[0] = func(*args, **kwargs)
File
"/usr/local/google/code/clankium/src/build/android/pylib/device/device_utils.py",
line 34, in _impl
return f(*args, **kwargs)
File "pylib/device/device_utils_test.py", line 117, in
alwaysRaisesCommandFailedError
'testCommand failed')
CommandFailedError: adb command 'testCommand' failed with message: 'testCommand
failed'
Explicit Functions (Exception):
Traceback (most recent call last):
File "pylib/device/device_utils_test.py", line 120, in
testViewExceptionStackTrace
test_obj.alwaysRaisesCommandFailedError(retries=1)
File "pylib/device/device_utils_test.py", line 76, in
alwaysRaisesCommandFailedError
self, timeout=timeout, retries=retries)
File
"/usr/local/google/code/clankium/src/build/android/pylib/device/device_utils.py",
line 35, in RunWithTimeoutAndRetries
return timeout_retry.Run(bound_f, timeout, retries)
File
"/usr/local/google/code/clankium/src/build/android/pylib/utils/timeout_retry.py",
line 45, in Run
thread_group.JoinAll(watchdog_timer.WatchdogTimer(timeout))
File
"/usr/local/google/code/clankium/src/build/android/pylib/utils/reraiser_thread.py",
line 136, in JoinAll
self._JoinAll(watcher)
File
"/usr/local/google/code/clankium/src/build/android/pylib/utils/reraiser_thread.py",
line 123, in _JoinAll
thread.ReraiseIfException()
File
"/usr/local/google/code/clankium/src/build/android/pylib/utils/reraiser_thread.py",
line 70, in run
self._func(*self._args, **self._kwargs)
File
"/usr/local/google/code/clankium/src/build/android/pylib/utils/timeout_retry.py",
line 37, in RunOnTimeoutThread
ret[0] = func(*args, **kwargs)
File
"/usr/local/google/code/clankium/src/build/android/pylib/device/device_utils.py",
line 32, in bound_f
return f(*args, **kwargs)
File "pylib/device/device_utils_test.py", line 81, in
_alwaysRaisesCommandFailedErrorImpl
'testCommand failed')
CommandFailedError: adb command 'testCommand' failed with message: 'testCommand
failed'
Decorator (Timeout):
Traceback (most recent call last):
File "pylib/device/device_utils_test.py", line 175, in
testViewTimeoutStackTrace
test_obj.alwaysTimesOut(timeout=1, retries=0)
File
"/usr/local/google/code/clankium/src/build/android/pylib/device/device_utils.py",
line 51, in wrapper
return WithTimeoutAndRetries(timeout, retries)(f)(device, *args, **kwargs)
File
"/usr/local/google/code/clankium/src/build/android/pylib/device/device_utils.py",
line 38, in wrapper
raise adb_wrapper.CommandTimeoutError(str(e))
CommandTimeoutError: Timed out waiting for 1 of 1 threads.
Explicit Functions (Timeout):
Traceback (most recent call last):
File "pylib/device/device_utils_test.py", line 110, in
testViewTimeoutStackTrace
test_obj.alwaysTimesOut(timeout=1, retries=0)
File "pylib/device/device_utils_test.py", line 64, in alwaysTimesOut
retries=retries)
File
"/usr/local/google/code/clankium/src/build/android/pylib/device/device_utils.py",
line 37, in RunWithTimeoutAndRetries
raise adb_wrapper.CommandTimeoutError(str(e))
CommandTimeoutError: Timed out waiting for 1 of 1 threads.
Thanks! I have to say those are fairly identically difficult to read if you didn't already know the code, but the difficult part is probably in all the retry/reraiser logic which is common to both. I think the best thing is just to fail early and provide informative error messages. btw, looks like you weren't yet using functools, which should help slightly. John, whatever you decide is good with me as you'll probably be the one maintaining this.
On 2014/05/05 16:35:21, craigdh wrote: > Thanks! I have to say those are fairly identically difficult to read if you > didn't already know the code, but the difficult part is probably in all the > retry/reraiser logic which is common to both. I think the best thing is just to > fail early and provide informative error messages. btw, looks like you weren't > yet using functools, which should help slightly. > > John, whatever you decide is good with me as you'll probably be the one > maintaining this. I absolutely agree that neither version makes particularly easy to read traces, but as you said, that's due to the common timeout / retry logic. I was using @wraps when I generated those stack traces. I'm still looking into why the stack traces still came back with "in wrapper".
yep, I think we're in the same page here, but let me further exemplify... :)
"except that I'd consider retrying/timeouts to be mostly orthogonal
(conceptually) to the behavior of the function"
I agree for this particular function, since it mostly has no side effects :)
however, for things that can have side-effects, say:
1)
@decorator(retries=3,timeout=100)
def RunShellCommand(cmd): ...
versus explicit params:
2)
def RunShellCommand(cmd, retries, timeout): ...
def RunShellCommandWithRetries(cmd, retries=X, timeout=Y): ...
the calling site can be one of the following:
a) RunShellCommand('cat foo >> bar')
b) RunShellCommand('cat foo >> bar', retries=0, timeout=Y)
c) RunShellCommandWithRetries('cat foo >> bar', retries=0, timeout=Y)
d) RunShellCommandWithRetries('cat foo >> bar')
imho:
a) would only work well with NO retries and NO decorator. it'd be terrible to
have something doing retries behind our backs, and the caller would NEVER know
why "bar" contains N copies of "foo"... :)
b) every single caller would have to pass retries and timeout, but that's fine,
right? explicit is better than artificial conciseness.
c and d): also fine because the name is explicit in itself, so we can tell just
by looking at the calling code that something dodgy can happen..
I guess what I'm trying to get at is :)
* for things that are truly orthogonal and would have no side effects (like this
patch!) I agree!!! decorators are super nice.
* for things that have potential side effects, making it more explicit is
probably better..
since we're bound to have both cases, would it be possible / worth it to solve
both problems with a single pattern?
I think the "Wait" mechanism on telemetry is a nice one, and perhaps we could
have a similar "Retry"... and then either the API would be
"DoSomethingWithRetryAndTimeout", or the caller can compose like,
"Retry(Wait(lambda: DoSomething, 'cat foo >> bar', timeout=10), retry=5)"...
On 2014/05/06 11:30:21, bulach wrote:
> yep, I think we're in the same page here, but let me further exemplify... :)
>
> "except that I'd consider retrying/timeouts to be mostly orthogonal
> (conceptually) to the behavior of the function"
>
> I agree for this particular function, since it mostly has no side effects :)
>
> however, for things that can have side-effects, say:
> 1)
> @decorator(retries=3,timeout=100)
> def RunShellCommand(cmd): ...
>
> versus explicit params:
> 2)
> def RunShellCommand(cmd, retries, timeout): ...
> def RunShellCommandWithRetries(cmd, retries=X, timeout=Y): ...
>
>
> the calling site can be one of the following:
> a) RunShellCommand('cat foo >> bar')
> b) RunShellCommand('cat foo >> bar', retries=0, timeout=Y)
> c) RunShellCommandWithRetries('cat foo >> bar', retries=0, timeout=Y)
> d) RunShellCommandWithRetries('cat foo >> bar')
>
>
> imho:
> a) would only work well with NO retries and NO decorator. it'd be terrible to
> have something doing retries behind our backs, and the caller would NEVER know
> why "bar" contains N copies of "foo"... :)
>
>
> b) every single caller would have to pass retries and timeout, but that's
fine,
> right? explicit is better than artificial conciseness.
>
> c and d): also fine because the name is explicit in itself, so we can tell
just
> by looking at the calling code that something dodgy can happen..
>
>
>
> I guess what I'm trying to get at is :)
> * for things that are truly orthogonal and would have no side effects (like
this
> patch!) I agree!!! decorators are super nice.
>
> * for things that have potential side effects, making it more explicit is
> probably better..
>
>
> since we're bound to have both cases, would it be possible / worth it to solve
> both problems with a single pattern?
>
> I think the "Wait" mechanism on telemetry is a nice one, and perhaps we could
> have a similar "Retry"... and then either the API would be
> "DoSomethingWithRetryAndTimeout", or the caller can compose like,
> "Retry(Wait(lambda: DoSomething, 'cat foo >> bar', timeout=10), retry=5)"...
Correct me if I'm wrong, but it seems that you're opposed to the use of default
parameters (which aren't exclusive to the decorator implementation) for retries
in particular because it's liable to be confusing in the event that whatever
we're doing has side effects. I can certainly see the potential for confusion
there. I'll think about a better way to do this. I think it would be worth it to
address this in a way that's clean in all cases.
> I think the "Wait" mechanism on telemetry is a nice one, and perhaps we could > have a similar "Retry"... and then either the API would be > "DoSomethingWithRetryAndTimeout", or the caller can compose like, > "Retry(Wait(lambda: DoSomething, 'cat foo >> bar', timeout=10), retry=5)"... What Frank and I experienced is that people tend to semi-randomly pick a timeout they think is reasonable (or copied from somewhere) but which ends up being inconsistent throughout the codebase, so we really want to discourage the caller from supplying an explicit timeout in the majority case. John should be able to supply sensible defaults that are good for 95% of uses. Unfortunately for something like RunShellCommand you're definitely right, we don't have a good idea what the side effects of retries could be for an arbitrary command, and we probably should require the caller explicitly provide it. But I think the vast majority of the functions John's providing will have well-known behavior which standard retries and timeouts will only make more reliable. So perhaps we should require explicit retries/timeouts only in the minority cases where there are side effects? It's not quite as clean, but I think it will lead to more reliable test scripts and I bet John can come up with a good way to differentiate between these two cases in the API.
On 2014/05/06 15:50:06, craigdh wrote: > > I think the "Wait" mechanism on telemetry is a nice one, and perhaps we could > > have a similar "Retry"... and then either the API would be > > "DoSomethingWithRetryAndTimeout", or the caller can compose like, > > "Retry(Wait(lambda: DoSomething, 'cat foo >> bar', timeout=10), retry=5)"... > > What Frank and I experienced is that people tend to semi-randomly pick a timeout > they think is reasonable (or copied from somewhere) but which ends up being > inconsistent throughout the codebase, so we really want to discourage the caller > from supplying an explicit timeout in the majority case. John should be able to > supply sensible defaults that are good for 95% of uses. > > Unfortunately for something like RunShellCommand you're definitely right, we > don't have a good idea what the side effects of retries could be for an > arbitrary command, and we probably should require the caller explicitly provide > it. But I think the vast majority of the functions John's providing will have > well-known behavior which standard retries and timeouts will only make more > reliable. So perhaps we should require explicit retries/timeouts only in the > minority cases where there are side effects? It's not quite as clean, but I > think it will lead to more reliable test scripts and I bet John can come up with > a good way to differentiate between these two cases in the API. I think you both put it better than I did :) default / implicit parameters are evil (regardless of the decorator / helper function), and people pick the wrong values, if at all. having everything explicit, with an API that clearly tells when retries / timeouts are going to happen is definitely something I'd love to see. whether these will be decorators or helpers, it's an implementation detail.. :) I suppose I'd be happier if the decorator would take explicit parameters rather than trying to derive default values. a part of me still thinks the decorator is more prone to abuse since it can be (mis)used at a whole-function level, whilst helpers would probably be a bit harder to misuse...
After talking with Craig about this extensively, decorators with sensible
default parameters for both timeouts and retries still seem like the best way to
go.
The main reason to favor decorators vs explicit functions is that the code is
more concise.
The main reason to favor using default parameters in general is that, in the
typical use case, a client will want timeouts and retries but doesn't have
specific timeout and retry requirements beyond something that makes sense. We'd
like to make it as easy as possible in that case.
For example, if a client wants to call, say, WriteFile with root permissions,
but doesn't have specific timeout / retry requirements, I think we'd rather have
this:
device.WriteFile('/path/to/file/temp_script_file_1234.sh',
'echo "Hello, world!"', root=True)
than this:
device.WriteFile('/path/to/file/temp_script_file_1234.sh',
'echo "Hello, world!"', root=True,
device.default_timeout, device.default_retries)
or this:
device.WriteFile('/path/to/file/temp_script_file_1234.sh',
'echo "Hello, world!"', root=True,
timeout=device.default_timeout,
retries=device.default_retries)
or this:
WithTimeoutAndRetries(
device.WriteFile(
'/path/to/file/temp_script_file_1234.sh',
'echo "Hello, world!"', root=True)
device.default_timeout,
device.default_retries)
or this:
device.WithDefaultBehavior().WriteFile(
'/path/to/file/temp_script_file_1234.sh',
'echo "Hello, world!"', root=True)
or even this:
device.WithDefaultRetries().WithDefaultTimeout().WriteFile(
'/path/to/file/temp_script_file_1234.sh',
'echo "Hello, world!"', root=True)
These alternatives all have at least one of the following flaws:
- they are verbose to the point of being clunky, especially in multiples
- they make it easier for people to make mistakes, e.g., by accidentally
omitting timeouts (big problem) or retries (not as much)
w.r.t. operations that can't be repeated - Craig pointed out that these should
generally be avoided if possible, as they'll produce undesirable results even
with explicit retries.
If a particular function or method should _always_ have explicit values for
timeouts and retries, we can use the @WithTimeoutAndRetries decorator.
On 2014/05/08 17:59:21, jbudorick wrote:
> After talking with Craig about this extensively, decorators with sensible
> default parameters for both timeouts and retries still seem like the best way
to
> go.
>
> The main reason to favor decorators vs explicit functions is that the code is
> more concise.
>
> The main reason to favor using default parameters in general is that, in the
> typical use case, a client will want timeouts and retries but doesn't have
> specific timeout and retry requirements beyond something that makes sense.
We'd
> like to make it as easy as possible in that case.
>
> For example, if a client wants to call, say, WriteFile with root permissions,
> but doesn't have specific timeout / retry requirements, I think we'd rather
have
> this:
>
> device.WriteFile('/path/to/file/temp_script_file_1234.sh',
> 'echo "Hello, world!"', root=True)
>
> than this:
>
> device.WriteFile('/path/to/file/temp_script_file_1234.sh',
> 'echo "Hello, world!"', root=True,
> device.default_timeout, device.default_retries)
>
> or this:
>
> device.WriteFile('/path/to/file/temp_script_file_1234.sh',
> 'echo "Hello, world!"', root=True,
> timeout=device.default_timeout,
> retries=device.default_retries)
>
> or this:
>
> WithTimeoutAndRetries(
> device.WriteFile(
> '/path/to/file/temp_script_file_1234.sh',
> 'echo "Hello, world!"', root=True)
> device.default_timeout,
> device.default_retries)
>
> or this:
>
> device.WithDefaultBehavior().WriteFile(
> '/path/to/file/temp_script_file_1234.sh',
> 'echo "Hello, world!"', root=True)
>
> or even this:
> device.WithDefaultRetries().WithDefaultTimeout().WriteFile(
> '/path/to/file/temp_script_file_1234.sh',
> 'echo "Hello, world!"', root=True)
>
> These alternatives all have at least one of the following flaws:
> - they are verbose to the point of being clunky, especially in multiples
> - they make it easier for people to make mistakes, e.g., by accidentally
> omitting timeouts (big problem) or retries (not as much)
>
> w.r.t. operations that can't be repeated - Craig pointed out that these should
> generally be avoided if possible, as they'll produce undesirable results even
> with explicit retries.
>
> If a particular function or method should _always_ have explicit values for
> timeouts and retries, we can use the @WithTimeoutAndRetries decorator.
hmm... say "WriteFile('/proc/sys/vm/drop_caches', 3)", would have potentially
different behavior depending on the retries.. :)
I agree with your examples that conciseness is nice to have, but there's a
danger that will be misused...
I think if we were to call "WriteFileWithRetriesAndTimeout", to explicitly
indicate what is going on under the hood, would that be too bad?
again, at that point, I don't care about decorator or internal helpers, would be
happy either way :)
I just want to avoid someone writing the code much higher up, say in telemetry
where there's already adb.adb.adb :) and having *no* indication of what the
function is doing under the hood...
if we all agree to name it explicitly, I'm happy to go with the decorators :)
(of course, for truly utterly "const" functions where no harm can possibly ever
done with retries / timeout, there's no need to have a long name... my concern
is about functions that can change the world, not pure "getters")...
On 2014/05/08 18:17:58, bulach wrote:
> On 2014/05/08 17:59:21, jbudorick wrote:
> > After talking with Craig about this extensively, decorators with sensible
> > default parameters for both timeouts and retries still seem like the best
way
> to
> > go.
> >
> > The main reason to favor decorators vs explicit functions is that the code
is
> > more concise.
> >
> > The main reason to favor using default parameters in general is that, in the
> > typical use case, a client will want timeouts and retries but doesn't have
> > specific timeout and retry requirements beyond something that makes sense.
> We'd
> > like to make it as easy as possible in that case.
> >
> > For example, if a client wants to call, say, WriteFile with root
permissions,
> > but doesn't have specific timeout / retry requirements, I think we'd rather
> have
> > this:
> >
> > device.WriteFile('/path/to/file/temp_script_file_1234.sh',
> > 'echo "Hello, world!"', root=True)
> >
> > than this:
> >
> > device.WriteFile('/path/to/file/temp_script_file_1234.sh',
> > 'echo "Hello, world!"', root=True,
> > device.default_timeout, device.default_retries)
> >
> > or this:
> >
> > device.WriteFile('/path/to/file/temp_script_file_1234.sh',
> > 'echo "Hello, world!"', root=True,
> > timeout=device.default_timeout,
> > retries=device.default_retries)
> >
> > or this:
> >
> > WithTimeoutAndRetries(
> > device.WriteFile(
> > '/path/to/file/temp_script_file_1234.sh',
> > 'echo "Hello, world!"', root=True)
> > device.default_timeout,
> > device.default_retries)
> >
> > or this:
> >
> > device.WithDefaultBehavior().WriteFile(
> > '/path/to/file/temp_script_file_1234.sh',
> > 'echo "Hello, world!"', root=True)
> >
> > or even this:
> > device.WithDefaultRetries().WithDefaultTimeout().WriteFile(
> > '/path/to/file/temp_script_file_1234.sh',
> > 'echo "Hello, world!"', root=True)
> >
> > These alternatives all have at least one of the following flaws:
> > - they are verbose to the point of being clunky, especially in multiples
> > - they make it easier for people to make mistakes, e.g., by accidentally
> > omitting timeouts (big problem) or retries (not as much)
> >
> > w.r.t. operations that can't be repeated - Craig pointed out that these
should
> > generally be avoided if possible, as they'll produce undesirable results
even
> > with explicit retries.
> >
> > If a particular function or method should _always_ have explicit values for
> > timeouts and retries, we can use the @WithTimeoutAndRetries decorator.
>
> hmm... say "WriteFile('/proc/sys/vm/drop_caches', 3)", would have potentially
> different behavior depending on the retries.. :)
>
> I agree with your examples that conciseness is nice to have, but there's a
> danger that will be misused...
>
> I think if we were to call "WriteFileWithRetriesAndTimeout", to explicitly
> indicate what is going on under the hood, would that be too bad?
>
> again, at that point, I don't care about decorator or internal helpers, would
be
> happy either way :)
>
> I just want to avoid someone writing the code much higher up, say in telemetry
> where there's already adb.adb.adb :) and having *no* indication of what the
> function is doing under the hood...
>
> if we all agree to name it explicitly, I'm happy to go with the decorators :)
>
> (of course, for truly utterly "const" functions where no harm can possibly
ever
> done with retries / timeout, there's no need to have a long name... my concern
> is about functions that can change the world, not pure "getters")...
So you're suggesting that almost every function in DeviceUtils should be
"FooWithTimeoutAndRetries" (or "FooWithRetriesAndTimeout")? I'm not crazy about
that, either, but I won't dismiss it out of hand since my best reason is that
it's ugly. (I don't want to be the one calling, e.g.,
GetMemoryUsageForPidWithTimeoutAndRetries.)
FWIW, somewhere along this chain of CLs, I'm going to collapse telemetry's fun
adb.adb.Adb().whatever_else. Unfortunately, it's probably close to the end of
the chain.
The sorts of commands that can't be retried are by far the edge case (most uses of WriteFile won't be to a magic file in proc :), and I think it's fair to expect people writing unusual code like that to explicitly disable the low-level retries, as if they're disabling those retries they really ought to be adding support for retries at higher level anyway. People will know they need to do that because _everything_ is retried by default. My point of view is that in general people should be writing commands that make as few assumptions of device state as possible (for example, it's safer to read the device's file contents, append in Python, and write the whole file all at once than it is to append to an existing device file). I think the API here should encourage this sort of coding behavior by default. Running adb commands is never going to be perfectly reliable, and with the sheer number of commands being run some are going to occasionally fail due to race conditions, transient device state, adb bugs, etc. To achieve reliability we need to retry commands at a low level by default. They only reason things are as stable as the are right now is because the underlying third party library we're using to run the adb commands right now retries 3 times. That said, there are some commands which are more dangerous than others. In particular, RunShellCommand has far less predictable side effects than something like WriteFile, and we'll probably need to treat it differently when John gets to adding it. We're also trying to cover as many current uses of RunShellCommand as possible with specific API calls, so the API can guide people to do things the right away (and consistently).
On 2014/05/08 19:21:03, craigdh wrote:
> The sorts of commands that can't be retried are by far the edge case (most
uses
> of WriteFile won't be to a magic file in proc :), and I think it's fair to
> expect people writing unusual code like that to explicitly disable the
low-level
> retries, as if they're disabling those retries they really ought to be adding
> support for retries at higher level anyway. People will know they need to do
> that because _everything_ is retried by default.
>
> My point of view is that in general people should be writing commands that
make
> as few assumptions of device state as possible (for example, it's safer to
read
> the device's file contents, append in Python, and write the whole file all at
> once than it is to append to an existing device file). I think the API here
> should encourage this sort of coding behavior by default. Running adb commands
> is never going to be perfectly reliable, and with the sheer number of commands
> being run some are going to occasionally fail due to race conditions,
transient
> device state, adb bugs, etc. To achieve reliability we need to retry commands
at
> a low level by default. They only reason things are as stable as the are right
> now is because the underlying third party library we're using to run the adb
> commands right now retries 3 times.
>
> That said, there are some commands which are more dangerous than others. In
> particular, RunShellCommand has far less predictable side effects than
something
> like WriteFile, and we'll probably need to treat it differently when John gets
> to adding it. We're also trying to cover as many current uses of
RunShellCommand
> as possible with specific API calls, so the API can guide people to do things
> the right away (and consistently).
I really think we're in agreement here and talking about the same thing :)
splitting:
1) GetMemoryUsageForPidWithTimeoutAndRetries ==> as I said, it's a simple getter
/ const function with no side effects, so we can safely omit
"...WithTimeoutAndRetries".
2) RunShellCommand ==> we also agree it's dangerous, so it won't have hidden
retries / timeouts..
3) WriteFile ==> looks like it's a borderline edge case :) I agree the vast
majority of uses should not have side effects, but since it can't be guaranteed,
I'd err on the safe side and make it explicit... for "people writing unusual
code": my parallel here is |file('foo', 'w').write(...)|... we don't expect any
retries / timeouts there, so WriteFile shouldn't... more generically: unless
it's absolutely safe, let's make it explicit!
does it summarize an agreement? :)
On 2014/05/09 08:37:59, bulach wrote:
> On 2014/05/08 19:21:03, craigdh wrote:
> > The sorts of commands that can't be retried are by far the edge case (most
> uses
> > of WriteFile won't be to a magic file in proc :), and I think it's fair to
> > expect people writing unusual code like that to explicitly disable the
> low-level
> > retries, as if they're disabling those retries they really ought to be
adding
> > support for retries at higher level anyway. People will know they need to do
> > that because _everything_ is retried by default.
> >
> > My point of view is that in general people should be writing commands that
> make
> > as few assumptions of device state as possible (for example, it's safer to
> read
> > the device's file contents, append in Python, and write the whole file all
at
> > once than it is to append to an existing device file). I think the API here
> > should encourage this sort of coding behavior by default. Running adb
commands
> > is never going to be perfectly reliable, and with the sheer number of
commands
> > being run some are going to occasionally fail due to race conditions,
> transient
> > device state, adb bugs, etc. To achieve reliability we need to retry
commands
> at
> > a low level by default. They only reason things are as stable as the are
right
> > now is because the underlying third party library we're using to run the adb
> > commands right now retries 3 times.
> >
> > That said, there are some commands which are more dangerous than others. In
> > particular, RunShellCommand has far less predictable side effects than
> something
> > like WriteFile, and we'll probably need to treat it differently when John
gets
> > to adding it. We're also trying to cover as many current uses of
> RunShellCommand
> > as possible with specific API calls, so the API can guide people to do
things
> > the right away (and consistently).
>
> I really think we're in agreement here and talking about the same thing :)
>
> splitting:
> 1) GetMemoryUsageForPidWithTimeoutAndRetries ==> as I said, it's a simple
getter
> / const function with no side effects, so we can safely omit
> "...WithTimeoutAndRetries".
>
> 2) RunShellCommand ==> we also agree it's dangerous, so it won't have hidden
> retries / timeouts..
>
> 3) WriteFile ==> looks like it's a borderline edge case :) I agree the vast
> majority of uses should not have side effects, but since it can't be
guaranteed,
> I'd err on the safe side and make it explicit... for "people writing unusual
> code": my parallel here is |file('foo', 'w').write(...)|... we don't expect
any
> retries / timeouts there, so WriteFile shouldn't... more generically: unless
> it's absolutely safe, let's make it explicit!
>
> does it summarize an agreement? :)
I think that sounds reasonable. We'll have a chance to look at each function
individually to decide what's dangerous and what isn't as we go through this
chain of CLs.
Sounds good to me. Thanks for taking the time to discuss all this Marcus, hopefully we'll end up with the best device library around!
On 2014/05/09 15:33:39, craigdh wrote: > Sounds good to me. Thanks for taking the time to discuss all this Marcus, > hopefully we'll end up with the best device library around! all good, all good! thank YOU both for putting up with my comments :) very glad we're all in the same page ;)
Now that we've resolved the timeout / retry discussion, I'm looking to get this in s.t. we can continue with this review chain. PTAL.
lgtm w/ nit https://codereview.chromium.org/265743002/diff/60001/build/android/pylib/devi... File build/android/pylib/device/device_utils.py (right): https://codereview.chromium.org/265743002/diff/60001/build/android/pylib/devi... build/android/pylib/device/device_utils.py:21: _WithTimeoutAndRetries = decorators.WithTimeoutAndRetries This additional level of indirection is confusing. If you really don't want to use the full module name just do something like "from pylib.device import decorators as D".
lgtm with some other nits, thanks a ton! (hopefully not opening a can of worms again, so feel free to go ahead regardless of my suggestions below..) https://codereview.chromium.org/265743002/diff/60001/build/android/pylib/devi... File build/android/pylib/device/device_utils.py (right): https://codereview.chromium.org/265743002/diff/60001/build/android/pylib/devi... build/android/pylib/device/device_utils.py:21: _WithTimeoutAndRetries = decorators.WithTimeoutAndRetries On 2014/05/13 15:46:24, craigdh wrote: > This additional level of indirection is confusing. If you really don't want to > use the full module name just do something like "from pylib.device import > decorators as D". gargh! :D ok, rant #271 :) these "translations" just make it much harder to follow, and save maybe a one line with extra indent ;) see "H(..)", "T(...)" and friends in the buildbot steps.. as you probably have guessed my opinion by now, keep it explicit. :) https://codereview.chromium.org/265743002/diff/60001/build/android/pylib/devi... build/android/pylib/device/device_utils.py:29: def GetAVDs(timeout=None, retries=None): is there any client passing these args in any sensible way? if not, remove it from the function definition, and just use the _DEFAULT values via the decorator (ditto for the one below)... should anybody ever need to change these values in one particular call, we'd then add these parameters again.
https://codereview.chromium.org/265743002/diff/60001/build/android/pylib/devi... File build/android/pylib/device/device_utils.py (right): https://codereview.chromium.org/265743002/diff/60001/build/android/pylib/devi... build/android/pylib/device/device_utils.py:21: _WithTimeoutAndRetries = decorators.WithTimeoutAndRetries I'm 100% with Marcus on this one.
On 2014/05/13 16:14:40, bulach wrote: > lgtm with some other nits, thanks a ton! > (hopefully not opening a can of worms again, so feel free to go ahead regardless > of my suggestions below..) It's ok, turns out there's a rebase involved, too. > > https://codereview.chromium.org/265743002/diff/60001/build/android/pylib/devi... > File build/android/pylib/device/device_utils.py (right): > > https://codereview.chromium.org/265743002/diff/60001/build/android/pylib/devi... > build/android/pylib/device/device_utils.py:21: _WithTimeoutAndRetries = > decorators.WithTimeoutAndRetries > On 2014/05/13 15:46:24, craigdh wrote: > > This additional level of indirection is confusing. If you really don't want to > > use the full module name just do something like "from pylib.device import > > decorators as D". > > gargh! :D > ok, rant #271 :) > these "translations" just make it much harder to follow, and save maybe a one > line with extra indent ;) > see "H(..)", "T(...)" and friends in the buildbot steps.. > as you probably have guessed my opinion by now, keep it explicit. :) > Yeah, I'm in the process of ditching these altogether. Don't worry. > https://codereview.chromium.org/265743002/diff/60001/build/android/pylib/devi... > build/android/pylib/device/device_utils.py:29: def GetAVDs(timeout=None, > retries=None): > is there any client passing these args in any sensible way? if not, remove it > from the function definition, and just use the _DEFAULT values via the decorator > (ditto for the one below)... > should anybody ever need to change these values in one particular call, we'd > then add these parameters again. AFAIK, no client is using these arguments. (There's currently only one GetAVDs client at all.) At issue is whether or not we want _any_ function decorated with one of the three decorators to receive the timeout and retries values as parameters. If one wants them, then the decorators have to pass them, and so all decorated functions have to accept them. I'm not sure what use functions would have for the explicit timeout and retries values.
Now with indirection-free use of the decorators + an OWNERS file for build/android/pylib/device/
On 2014/05/13 16:21:57, jbudorick wrote: > On 2014/05/13 16:14:40, bulach wrote: > > lgtm with some other nits, thanks a ton! > > (hopefully not opening a can of worms again, so feel free to go ahead > regardless > > of my suggestions below..) > > It's ok, turns out there's a rebase involved, too. > > > > > > https://codereview.chromium.org/265743002/diff/60001/build/android/pylib/devi... > > File build/android/pylib/device/device_utils.py (right): > > > > > https://codereview.chromium.org/265743002/diff/60001/build/android/pylib/devi... > > build/android/pylib/device/device_utils.py:21: _WithTimeoutAndRetries = > > decorators.WithTimeoutAndRetries > > On 2014/05/13 15:46:24, craigdh wrote: > > > This additional level of indirection is confusing. If you really don't want > to > > > use the full module name just do something like "from pylib.device import > > > decorators as D". > > > > gargh! :D > > ok, rant #271 :) > > these "translations" just make it much harder to follow, and save maybe a one > > line with extra indent ;) > > see "H(..)", "T(...)" and friends in the buildbot steps.. > > as you probably have guessed my opinion by now, keep it explicit. :) > > > > Yeah, I'm in the process of ditching these altogether. Don't worry. > > > > https://codereview.chromium.org/265743002/diff/60001/build/android/pylib/devi... > > build/android/pylib/device/device_utils.py:29: def GetAVDs(timeout=None, > > retries=None): > > is there any client passing these args in any sensible way? if not, remove it > > from the function definition, and just use the _DEFAULT values via the > decorator > > (ditto for the one below)... > > should anybody ever need to change these values in one particular call, we'd > > then add these parameters again. > > AFAIK, no client is using these arguments. (There's currently only one GetAVDs > client at all.) > > At issue is whether or not we want _any_ function decorated with one of the > three decorators to receive the timeout and retries values as parameters. If one > wants them, then the decorators have to pass them, and so all decorated > functions have to accept them. > > I'm not sure what use functions would have for the explicit timeout and retries > values. still lgtm! I'm not sure I followed... my suggestion was to use something like: @decorators.WithTimeoutAndRetriesDefaults( _DEFAULT_TIMEOUT, _DEFAULT_RETRIES) def GetAVDs(): ... wouldn't that work? as in, the API for GetAVDs() takes no parameters, since no client can sensibly pass anything else than the default values. the decorator on the function itself provides default values. wouldn't be a matter on decorator.py to change: 49 return WithTimeoutAndRetries(f)(*args, **kwargs) to: 49 return WithTimeoutAndRetries(f, timeout=..., retries=...)(*args, **kwargs) rather than than tweaking kwargs there? alternatively, we could have yet-another-one, "WithHardcodedTimeoutAndRetries"... the idea here being to avoid "bleeding" the timeout/retry parameter all the way through when no caller can ever do anything with it... :)
https://codereview.chromium.org/265743002/diff/80001/build/android/pylib/devi... File build/android/pylib/device/OWNERS (right): https://codereview.chromium.org/265743002/diff/80001/build/android/pylib/devi... build/android/pylib/device/OWNERS:1: set noparent ops, sorry... I wouldn't do setnoparent, in practice I've seen it hurting more by blocking people rather than helping in preventing breakages... but again, up to you. :) (btw, I shouldn't be here!)
still lgtm w/ new nits https://codereview.chromium.org/265743002/diff/80001/build/android/pylib/devi... File build/android/pylib/device/adb_wrapper.py (right): https://codereview.chromium.org/265743002/diff/80001/build/android/pylib/devi... build/android/pylib/device/adb_wrapper.py:23: BaseError = device_errors.BaseError I don't like this either. Exceptions raised from different modules are actually equal? That could lead to very confusing try/except behavior. Let people reference these directly from their real module.
On 2014/05/13 16:59:41, craigdh wrote: > still lgtm w/ new nits > > https://codereview.chromium.org/265743002/diff/80001/build/android/pylib/devi... > File build/android/pylib/device/adb_wrapper.py (right): > > https://codereview.chromium.org/265743002/diff/80001/build/android/pylib/devi... > build/android/pylib/device/adb_wrapper.py:23: BaseError = > device_errors.BaseError > I don't like this either. Exceptions raised from different modules are actually > equal? That could lead to very confusing try/except behavior. Let people > reference these directly from their real module. This was my plan, but I had thought that their current use was more widespread than it is and was planning to update in a separate CL. However, these are not very heavily used yet, so I'll add those switches to this CL.
On 2014/05/13 16:57:52, bulach wrote: > https://codereview.chromium.org/265743002/diff/80001/build/android/pylib/devi... > File build/android/pylib/device/OWNERS (right): > > https://codereview.chromium.org/265743002/diff/80001/build/android/pylib/devi... > build/android/pylib/device/OWNERS:1: set noparent > ops, sorry... I wouldn't do setnoparent, in practice I've seen it hurting more > by blocking people rather than helping in preventing breakages... but again, up > to you. :) > (btw, I shouldn't be here!) Craig suggested noparent because build/OWNERS is just *, so changes can fly through here without any of us knowing (e.g. https://codereview.chromium.org/255783008 ) Having the three of us in there is kind of amusing at some level - one who's leaving, one who's already left, and one who isn't a committer.
On 2014/05/13 16:56:12, bulach wrote: > On 2014/05/13 16:21:57, jbudorick wrote: > > On 2014/05/13 16:14:40, bulach wrote: > > > lgtm with some other nits, thanks a ton! > > > (hopefully not opening a can of worms again, so feel free to go ahead > > regardless > > > of my suggestions below..) > > > > It's ok, turns out there's a rebase involved, too. > > > > > > > > > > > https://codereview.chromium.org/265743002/diff/60001/build/android/pylib/devi... > > > File build/android/pylib/device/device_utils.py (right): > > > > > > > > > https://codereview.chromium.org/265743002/diff/60001/build/android/pylib/devi... > > > build/android/pylib/device/device_utils.py:21: _WithTimeoutAndRetries = > > > decorators.WithTimeoutAndRetries > > > On 2014/05/13 15:46:24, craigdh wrote: > > > > This additional level of indirection is confusing. If you really don't > want > > to > > > > use the full module name just do something like "from pylib.device import > > > > decorators as D". > > > > > > gargh! :D > > > ok, rant #271 :) > > > these "translations" just make it much harder to follow, and save maybe a > one > > > line with extra indent ;) > > > see "H(..)", "T(...)" and friends in the buildbot steps.. > > > as you probably have guessed my opinion by now, keep it explicit. :) > > > > > > > Yeah, I'm in the process of ditching these altogether. Don't worry. > > > > > > > > https://codereview.chromium.org/265743002/diff/60001/build/android/pylib/devi... > > > build/android/pylib/device/device_utils.py:29: def GetAVDs(timeout=None, > > > retries=None): > > > is there any client passing these args in any sensible way? if not, remove > it > > > from the function definition, and just use the _DEFAULT values via the > > decorator > > > (ditto for the one below)... > > > should anybody ever need to change these values in one particular call, we'd > > > then add these parameters again. > > > > AFAIK, no client is using these arguments. (There's currently only one GetAVDs > > client at all.) > > > > At issue is whether or not we want _any_ function decorated with one of the > > three decorators to receive the timeout and retries values as parameters. If > one > > wants them, then the decorators have to pass them, and so all decorated > > functions have to accept them. > > > > I'm not sure what use functions would have for the explicit timeout and > retries > > values. > > still lgtm! > > I'm not sure I followed... > my suggestion was to use something like: > @decorators.WithTimeoutAndRetriesDefaults( > _DEFAULT_TIMEOUT, _DEFAULT_RETRIES) > def GetAVDs(): > ... > > wouldn't that work? > as in, the API for GetAVDs() takes no parameters, since no client can sensibly > pass anything else than the default values. > the decorator on the function itself provides default values. > > wouldn't be a matter on decorator.py to change: > 49 return WithTimeoutAndRetries(f)(*args, **kwargs) > to: > 49 return WithTimeoutAndRetries(f, timeout=..., retries=...)(*args, > **kwargs) > > rather than than tweaking kwargs there? > > alternatively, we could have yet-another-one, > "WithHardcodedTimeoutAndRetries"... > > the idea here being to avoid "bleeding" the timeout/retry parameter all the way > through when no caller can ever do anything with it... :) That would work. My concern was that it may not be obvious that timeouts and retries are settable if they aren't in the parameter list. Perhaps it would be better to have this decorator not pass the values through. If something for some reason needs them, we can address it then, probably be adding (another!) decorator.
On 2014/05/13 17:05:11, jbudorick wrote: > On 2014/05/13 16:57:52, bulach wrote: > > > https://codereview.chromium.org/265743002/diff/80001/build/android/pylib/devi... > > File build/android/pylib/device/OWNERS (right): > > > > > https://codereview.chromium.org/265743002/diff/80001/build/android/pylib/devi... > > build/android/pylib/device/OWNERS:1: set noparent > > ops, sorry... I wouldn't do setnoparent, in practice I've seen it hurting more > > by blocking people rather than helping in preventing breakages... but again, > up > > to you. :) > > (btw, I shouldn't be here!) > > Craig suggested noparent because build/OWNERS is just *, so changes can fly > through here without any of us knowing (e.g. > https://codereview.chromium.org/255783008 ) > > Having the three of us in there is kind of amusing at some level - one who's > leaving, one who's already left, and one who isn't a committer. Yeah, I definitely wouldn't suggest setnoparent for all of pylib/, but historically people haven't been very good about including OWNERS in good faith when dumping things into pylib's device utils, and legitimate changes here should be pretty rare.
lgtm https://codereview.chromium.org/265743002/diff/80001/build/android/pylib/devi... File build/android/pylib/device/OWNERS (right): https://codereview.chromium.org/265743002/diff/80001/build/android/pylib/devi... build/android/pylib/device/OWNERS:1: set noparent Hmm, I've had the opposite experience where people bypass the owners (mostly accidentally) and check in code that violates the design and/or style guide. Needing owners to sign off is how it's done in other parts of code base (//build being an exception since it's a hodgepodge) and rest of google. It's specially important since we're going through all this trouble to refactor and get rid of code smell. @John, keep in mind that you're the only here who's actually in the team, so responsibility is on you to be responsive (< 24 hours review time preferably). https://codereview.chromium.org/265743002/diff/80001/build/android/pylib/devi... File build/android/pylib/device/decorators.py (right): https://codereview.chromium.org/265743002/diff/80001/build/android/pylib/devi... build/android/pylib/device/decorators.py:17: """ A decorator that handles timeouts and retries. fit everything on 1 line https://codereview.chromium.org/265743002/diff/80001/build/android/pylib/devi... build/android/pylib/device/decorators.py:56: """ Returns a decorator that handles timeouts and retries using default first sentence should fit on a line.
nits addressed. Added a new decorator for functions for which clients should never need to adjust the provided default timeout & retry values.
The CQ bit was checked by jbudorick@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/jbudorick@chromium.org/265743002/140001
FYI, CQ is re-trying this CL (attempt #1). Please consider checking whether the failures are real, and report flakes to chrome-troopers@google.com. The failing builders are: android_aosp on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/android_aosp/builds/7...) win_chromium_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_rel/buil...)
Message was sent while issue was closed.
Change committed as 270336 |
