Rework RetryCommand so it's usable beyond just RunCommand ta... [chromiumos/chromite : master]
32 views
Skip to first unread message
Brian Harring (Code Review)
Oct 25, 2012, 7:00:34 PM10/25/12
to Ryan Cui, Chris Sosa, Brian Harring
Brian Harring has uploaded a new change for review.
Change subject: Rework RetryCommand so it's usable beyond just RunCommand targets.
......................................................................
Rework RetryCommand so it's usable beyond just RunCommand targets.
BUG=chromium-os:32910
TEST=unittests
Change-Id: I117e399c34d16d86f3df6b7eac878e15be025d9e
---
M lib/cros_build_lib.py
1 file changed, 9 insertions(+), 3 deletions(-)
git pull ssh://gerrit.chromium.org:29418/chromiumos/chromite refs/changes/07/36607/1
--
To view, visit https://gerrit.chromium.org/gerrit/36607
To unsubscribe, visit https://gerrit.chromium.org/gerrit/settings
Gerrit-MessageType: newchange
Gerrit-Change-Id: I117e399c34d16d86f3df6b7eac878e15be025d9e
Gerrit-PatchSet: 1
Gerrit-Project: chromiumos/chromite
Gerrit-Branch: master
Gerrit-Owner: Brian Harring <ferr...@chromium.org>
Change subject: Rework RetryCommand so it's usable beyond just RunCommand targets.
......................................................................
Rework RetryCommand so it's usable beyond just RunCommand targets.
BUG=chromium-os:32910
TEST=unittests
Change-Id: I117e399c34d16d86f3df6b7eac878e15be025d9e
---
M lib/cros_build_lib.py
1 file changed, 9 insertions(+), 3 deletions(-)
git pull ssh://gerrit.chromium.org:29418/chromiumos/chromite refs/changes/07/36607/1
--
To view, visit https://gerrit.chromium.org/gerrit/36607
To unsubscribe, visit https://gerrit.chromium.org/gerrit/settings
Gerrit-MessageType: newchange
Gerrit-Change-Id: I117e399c34d16d86f3df6b7eac878e15be025d9e
Gerrit-PatchSet: 1
Gerrit-Project: chromiumos/chromite
Gerrit-Branch: master
Gerrit-Owner: Brian Harring <ferr...@chromium.org>
Brian Harring (Code Review)
Oct 25, 2012, 7:00:49 PM10/25/12
to Brian Harring, David James, Ryan Cui
Brian Harring has posted comments on this change.
Change subject: Rework RetryCommand so it's usable beyond just RunCommand targets.
......................................................................
Patch Set 1: Verified
--
To view, visit https://gerrit.chromium.org/gerrit/36607
To unsubscribe, visit https://gerrit.chromium.org/gerrit/settings
Gerrit-MessageType: comment
Gerrit-Reviewer: David James <david...@chromium.org>
Gerrit-Reviewer: Ryan Cui <rc...@chromium.org>
Change subject: Rework RetryCommand so it's usable beyond just RunCommand targets.
......................................................................
--
To view, visit https://gerrit.chromium.org/gerrit/36607
To unsubscribe, visit https://gerrit.chromium.org/gerrit/settings
Gerrit-Change-Id: I117e399c34d16d86f3df6b7eac878e15be025d9e
Gerrit-PatchSet: 1
Gerrit-Project: chromiumos/chromite
Gerrit-Branch: master
Gerrit-Owner: Brian Harring <ferr...@chromium.org>
Gerrit-Reviewer: Brian Harring <ferr...@chromium.org>
Gerrit-PatchSet: 1
Gerrit-Project: chromiumos/chromite
Gerrit-Branch: master
Gerrit-Owner: Brian Harring <ferr...@chromium.org>
Gerrit-Reviewer: David James <david...@chromium.org>
Gerrit-Reviewer: Ryan Cui <rc...@chromium.org>
Brian Harring (Code Review)
Oct 25, 2012, 7:04:52 PM10/25/12
to Brian Harring, David James, Ryan Cui
Brian Harring has posted comments on this change.
Change subject: Rework RetryCommand so it's usable beyond just RunCommand targets.
......................................................................
Patch Set 1: No score
......................................................................
screwing w/ the api...
--
To view, visit https://gerrit.chromium.org/gerrit/36607
To unsubscribe, visit https://gerrit.chromium.org/gerrit/settings
Gerrit-Change-Id: I117e399c34d16d86f3df6b7eac878e15be025d9e
Gerrit-PatchSet: 1
Gerrit-Project: chromiumos/chromite
Gerrit-Branch: master
Gerrit-Owner: Brian Harring <ferr...@chromium.org>
Gerrit-PatchSet: 1
Gerrit-Project: chromiumos/chromite
Gerrit-Branch: master
Gerrit-Owner: Brian Harring <ferr...@chromium.org>
Mike Frysinger (Code Review)
Oct 26, 2012, 5:17:39 PM10/26/12
to Brian Harring, David James, Ryan Cui, Mike Frysinger
Mike Frysinger has posted comments on this change.
Change subject: Rework RetryCommand so it's usable beyond just RunCommand targets.
......................................................................
Patch Set 3: I would prefer that you didn't submit this
(8 inline comments)
....................................................
File lib/cros_build_lib.py
Line 1524: functor(*args, **kwds). If it returns True, then a retry
well, it has to return something that evaluates to true and not exactly "True" ... but i don't think we've been that pedantic in our documentation in the past
Line 1527: non-excepting invocations of functor(*args, **kwds)
should have a period at the end here
Line 1556: stopper = lambda x:False
space after the colon ?
Line 1557: return_handler = stopper if return_handler is None else return_handler
hard to say if this is easier to read than just:
if not return_handler:
return_handler = stopper
or:
return_handler = return_handler or stopper
not sure there's any case where a function would evaluate to false
Line 1568: except TerminateRunCommandError:
shouldn't this be in RetryCommand() in the do_retry handler ?
Line 1598:
needs two blank lines
Line 1607: that is given the raise exception. If the raise exception
raise->raised
Line 1618:
needs two blank lines
--
To view, visit https://gerrit.chromium.org/gerrit/36607
To unsubscribe, visit https://gerrit.chromium.org/gerrit/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I117e399c34d16d86f3df6b7eac878e15be025d9e
Gerrit-PatchSet: 3
Gerrit-Reviewer: Ryan Cui <rc...@chromium.org>
Change subject: Rework RetryCommand so it's usable beyond just RunCommand targets.
......................................................................
(8 inline comments)
....................................................
File lib/cros_build_lib.py
Line 1524: functor(*args, **kwds). If it returns True, then a retry
well, it has to return something that evaluates to true and not exactly "True" ... but i don't think we've been that pedantic in our documentation in the past
Line 1527: non-excepting invocations of functor(*args, **kwds)
should have a period at the end here
Line 1556: stopper = lambda x:False
space after the colon ?
Line 1557: return_handler = stopper if return_handler is None else return_handler
hard to say if this is easier to read than just:
if not return_handler:
return_handler = stopper
or:
return_handler = return_handler or stopper
not sure there's any case where a function would evaluate to false
Line 1568: except TerminateRunCommandError:
shouldn't this be in RetryCommand() in the do_retry handler ?
Line 1598:
needs two blank lines
Line 1607: that is given the raise exception. If the raise exception
raise->raised
Line 1618:
needs two blank lines
--
To view, visit https://gerrit.chromium.org/gerrit/36607
To unsubscribe, visit https://gerrit.chromium.org/gerrit/settings
Gerrit-Change-Id: I117e399c34d16d86f3df6b7eac878e15be025d9e
Gerrit-PatchSet: 3
Gerrit-Project: chromiumos/chromite
Gerrit-Branch: master
Gerrit-Owner: Brian Harring <ferr...@chromium.org>
Gerrit-Branch: master
Gerrit-Owner: Brian Harring <ferr...@chromium.org>
Gerrit-Reviewer: Brian Harring <ferr...@chromium.org>
Gerrit-Reviewer: David James <david...@chromium.org>
Gerrit-Reviewer: Mike Frysinger <vap...@chromium.org>
Gerrit-Reviewer: David James <david...@chromium.org>
Gerrit-Reviewer: Ryan Cui <rc...@chromium.org>
Brian Harring (Code Review)
Oct 26, 2012, 9:25:50 PM10/26/12
to Brian Harring, David James, Ryan Cui, Mike Frysinger
Brian Harring has posted comments on this change.
Change subject: Rework RetryCommand so it's usable beyond just RunCommand targets.
......................................................................
Patch Set 3: (2 inline comments)
....................................................
File lib/cros_build_lib.py
If someone passing in something- specifically non None, then regardless of the content, I'll use it (and it may explode, but that's fine). Using the identity approach here means that it's impossible for an inadvertant '' (or similar) to be passed in (which is wrong) and silently ignored.
Line 1568: except TerminateRunCommandError:
No. do_retry cannot suppress this, same as it can't suppress keyboardinterrupts/systemexits.
do_retry can only state "yes, retry it" or "no, don't"; meaning it should not see exceptions where we don't want retries despite what it thinks (we wouldn't retry on a MemoryError for example).
--
To view, visit https://gerrit.chromium.org/gerrit/36607
To unsubscribe, visit https://gerrit.chromium.org/gerrit/settings
Change subject: Rework RetryCommand so it's usable beyond just RunCommand targets.
......................................................................
....................................................
File lib/cros_build_lib.py
Line 1557: return_handler = stopper if return_handler is None else return_handler
you're ignoring identity; the reason I use 'is None' is such that I avoid boolean protocol.
If someone passing in something- specifically non None, then regardless of the content, I'll use it (and it may explode, but that's fine). Using the identity approach here means that it's impossible for an inadvertant '' (or similar) to be passed in (which is wrong) and silently ignored.
Line 1568: except TerminateRunCommandError:
No. do_retry cannot suppress this, same as it can't suppress keyboardinterrupts/systemexits.
do_retry can only state "yes, retry it" or "no, don't"; meaning it should not see exceptions where we don't want retries despite what it thinks (we wouldn't retry on a MemoryError for example).
--
To view, visit https://gerrit.chromium.org/gerrit/36607
To unsubscribe, visit https://gerrit.chromium.org/gerrit/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I117e399c34d16d86f3df6b7eac878e15be025d9e
Gerrit-PatchSet: 3
Gerrit-Change-Id: I117e399c34d16d86f3df6b7eac878e15be025d9e
Gerrit-PatchSet: 3
Gerrit-Project: chromiumos/chromite
Gerrit-Branch: master
Gerrit-Owner: Brian Harring <ferr...@chromium.org>
Gerrit-Branch: master
Gerrit-Owner: Brian Harring <ferr...@chromium.org>
Mike Frysinger (Code Review)
Oct 27, 2012, 3:29:53 AM10/27/12
to Brian Harring, David James, Ryan Cui, Mike Frysinger
Mike Frysinger has posted comments on this change.
Change subject: Rework RetryCommand so it's usable beyond just RunCommand targets.
......................................................................
Line 1568: except TerminateRunCommandError:
do_retry can also throw an exception. imo, this should be in do_retry since this is RunCommand specific.
--
To view, visit https://gerrit.chromium.org/gerrit/36607
To unsubscribe, visit https://gerrit.chromium.org/gerrit/settings
Change subject: Rework RetryCommand so it's usable beyond just RunCommand targets.
......................................................................
Patch Set 3: (2 inline comments)
....................................................
File lib/cros_build_lib.py
Line 1557: return_handler = stopper if return_handler is None else return_handler
it's not that i was ignoring identity, it was that i forgot that people could pass in stupid stuff like non-functions
....................................................
File lib/cros_build_lib.py
Line 1557: return_handler = stopper if return_handler is None else return_handler
Line 1568: except TerminateRunCommandError:
do_retry can also throw an exception. imo, this should be in do_retry since this is RunCommand specific.
--
To view, visit https://gerrit.chromium.org/gerrit/36607
To unsubscribe, visit https://gerrit.chromium.org/gerrit/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I117e399c34d16d86f3df6b7eac878e15be025d9e
Gerrit-PatchSet: 3
Gerrit-Change-Id: I117e399c34d16d86f3df6b7eac878e15be025d9e
Gerrit-PatchSet: 3
Gerrit-Project: chromiumos/chromite
Gerrit-Branch: master
Gerrit-Owner: Brian Harring <ferr...@chromium.org>
Gerrit-Branch: master
Gerrit-Owner: Brian Harring <ferr...@chromium.org>
Ryan Cui (Code Review)
Oct 29, 2012, 2:19:16 PM10/29/12
to Brian Harring, David James, Ryan Cui, Mike Frysinger
Ryan Cui has posted comments on this change.
Change subject: Rework RetryCommand so it's usable beyond just RunCommand targets.
......................................................................
Extra closing brace
Line 1568: except TerminateRunCommandError:
I agree, this seems better placed in a exc_handler() passed in by RetryCommand.
--
To view, visit https://gerrit.chromium.org/gerrit/36607
To unsubscribe, visit https://gerrit.chromium.org/gerrit/settings
Change subject: Rework RetryCommand so it's usable beyond just RunCommand targets.
......................................................................
Patch Set 3: (2 inline comments)
....................................................
File lib/cros_build_lib.py
Line 1536: functor: A callable to pass args and kargs to).
....................................................
File lib/cros_build_lib.py
Extra closing brace
Line 1568: except TerminateRunCommandError:
I agree, this seems better placed in a exc_handler() passed in by RetryCommand.
--
To view, visit https://gerrit.chromium.org/gerrit/36607
To unsubscribe, visit https://gerrit.chromium.org/gerrit/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I117e399c34d16d86f3df6b7eac878e15be025d9e
Gerrit-PatchSet: 3
Gerrit-Change-Id: I117e399c34d16d86f3df6b7eac878e15be025d9e
Gerrit-PatchSet: 3
Gerrit-Project: chromiumos/chromite
Gerrit-Branch: master
Gerrit-Owner: Brian Harring <ferr...@chromium.org>
Gerrit-Branch: master
Gerrit-Owner: Brian Harring <ferr...@chromium.org>
David James (Code Review)
Oct 31, 2012, 2:37:57 PM10/31/12
to Brian Harring, Ryan Cui, Mike Frysinger
David James has posted comments on this change.
Change subject: Rework RetryCommand so it's usable beyond just RunCommand targets.
......................................................................
This is unnecessary, since these exceptions inherit from BaseException, not Exception:
>>> isinstance(KeyboardInterrupt(), Exception)
False
>>> isinstance(SystemExit(), Exception)
False
Line 1581: exc_info = sys.exc_info()
Warning: Assigning the traceback return value to a local variable in a function that is handling an exception will cause a circular reference. (See http://effbot.org/pyref/sys.exc_info.htm)
--
To view, visit https://gerrit.chromium.org/gerrit/36607
To unsubscribe, visit https://gerrit.chromium.org/gerrit/settings
Change subject: Rework RetryCommand so it's usable beyond just RunCommand targets.
......................................................................
Patch Set 3: (2 inline comments)
....................................................
File lib/cros_build_lib.py
Line 1573: except (SystemExit, KeyboardInterrupt):
....................................................
File lib/cros_build_lib.py
This is unnecessary, since these exceptions inherit from BaseException, not Exception:
>>> isinstance(KeyboardInterrupt(), Exception)
False
>>> isinstance(SystemExit(), Exception)
False
Line 1581: exc_info = sys.exc_info()
Warning: Assigning the traceback return value to a local variable in a function that is handling an exception will cause a circular reference. (See http://effbot.org/pyref/sys.exc_info.htm)
--
To view, visit https://gerrit.chromium.org/gerrit/36607
To unsubscribe, visit https://gerrit.chromium.org/gerrit/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I117e399c34d16d86f3df6b7eac878e15be025d9e
Gerrit-PatchSet: 3
Gerrit-Change-Id: I117e399c34d16d86f3df6b7eac878e15be025d9e
Gerrit-PatchSet: 3
Gerrit-Project: chromiumos/chromite
Gerrit-Branch: master
Gerrit-Owner: Brian Harring <ferr...@chromium.org>
Gerrit-Branch: master
Gerrit-Owner: Brian Harring <ferr...@chromium.org>
Brian Harring (Code Review)
Nov 5, 2012, 6:22:43 PM11/5/12
to Brian Harring, David James, Ryan Cui, Mike Frysinger, Gerrit
Brian Harring has posted comments on this change.
Change subject: Rework RetryCommand so it's usable beyond just RunCommand targets.
......................................................................
Patch Set 4: Verified
--
To view, visit https://gerrit.chromium.org/gerrit/36607
To unsubscribe, visit https://gerrit.chromium.org/gerrit/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I117e399c34d16d86f3df6b7eac878e15be025d9e
Gerrit-PatchSet: 4
Change subject: Rework RetryCommand so it's usable beyond just RunCommand targets.
......................................................................
--
To view, visit https://gerrit.chromium.org/gerrit/36607
To unsubscribe, visit https://gerrit.chromium.org/gerrit/settings
Gerrit-Change-Id: I117e399c34d16d86f3df6b7eac878e15be025d9e
Gerrit-PatchSet: 4
Gerrit-Project: chromiumos/chromite
Gerrit-Branch: master
Gerrit-Owner: Brian Harring <ferr...@chromium.org>
Gerrit-Branch: master
Gerrit-Owner: Brian Harring <ferr...@chromium.org>
Gerrit-Reviewer: Brian Harring <ferr...@chromium.org>
Gerrit-Reviewer: David James <david...@chromium.org>
Gerrit-Reviewer: Gerrit <chrom...@google.com>
Gerrit-Reviewer: David James <david...@chromium.org>
Mike Frysinger (Code Review)
Nov 5, 2012, 10:41:16 PM11/5/12
to Brian Harring, David James, Ryan Cui, Mike Frysinger, Gerrit
Mike Frysinger has posted comments on this change.
Change subject: Rework RetryCommand so it's usable beyond just RunCommand targets.
......................................................................
Patch Set 4: Looks good to me, approved
--
To view, visit https://gerrit.chromium.org/gerrit/36607
To unsubscribe, visit https://gerrit.chromium.org/gerrit/settings
Change subject: Rework RetryCommand so it's usable beyond just RunCommand targets.
......................................................................
--
To view, visit https://gerrit.chromium.org/gerrit/36607
To unsubscribe, visit https://gerrit.chromium.org/gerrit/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I117e399c34d16d86f3df6b7eac878e15be025d9e
Gerrit-PatchSet: 4
Gerrit-Change-Id: I117e399c34d16d86f3df6b7eac878e15be025d9e
Gerrit-PatchSet: 4
Gerrit-Project: chromiumos/chromite
Gerrit-Branch: master
Gerrit-Owner: Brian Harring <ferr...@chromium.org>
Gerrit-Branch: master
Gerrit-Owner: Brian Harring <ferr...@chromium.org>
Brian Harring (Code Review)
Nov 6, 2012, 9:36:26 PM11/6/12
to Brian Harring, David James, Ryan Cui, Mike Frysinger, Gerrit
Brian Harring has posted comments on this change.
Change subject: Rework RetryCommand so it's usable beyond just RunCommand targets.
......................................................................
Patch Set 4: Ready
--
To view, visit https://gerrit.chromium.org/gerrit/36607
To unsubscribe, visit https://gerrit.chromium.org/gerrit/settings
Change subject: Rework RetryCommand so it's usable beyond just RunCommand targets.
......................................................................
--
To view, visit https://gerrit.chromium.org/gerrit/36607
To unsubscribe, visit https://gerrit.chromium.org/gerrit/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I117e399c34d16d86f3df6b7eac878e15be025d9e
Gerrit-PatchSet: 4
Gerrit-Change-Id: I117e399c34d16d86f3df6b7eac878e15be025d9e
Gerrit-PatchSet: 4
Gerrit-Project: chromiumos/chromite
Gerrit-Branch: master
Gerrit-Owner: Brian Harring <ferr...@chromium.org>
Gerrit-Branch: master
Gerrit-Owner: Brian Harring <ferr...@chromium.org>
Reply all
Reply to author
Forward
0 new messages