retry build errors

[bparees]

For now this will retry retryable failures forever on a tight loop. I'm still contemplating switching it to something like 100. The problem is that without backoff (which the current retrycontroller logic does not offer for good reason), if there's a temporary API outage we'll burn through any reasonable number of retries rapidly.

[bparees]

[test]

[openshift-bot]

continuous-integration/openshift-jenkins/test SUCCESS (https://ci.openshift.redhat.com/jenkins/job/test_pull_requests_openshift3/1238/)

[smarterclayton]

I don't think this message belongs here - retry decisions belong in a higher layer.

[bparees]

the controller is the bit that has the logic to know whether a particular event can/should be retried or not (based on what state has been modified or would be duplicated if the event is replayed) so i don't see how you can(well, should) defer the decision to retry out of the controller. The fact is the controller is making the decision to retry or not based on the type of error it returns and knowing the logic of the retry controller anyway. There's really no avoiding the fact that the controller needs to have some knowledge of the types of errors the retry handler is expecting and how it will handle them. the less of that there is, the better, imho. (which is why I preferred the approach that just returned RetryableError vs FatalError wrapping the real error)

[smarterclayton]

Higher layer = your retry handler. That's where you make a decision about retry. Adding it here too confuses the issue

[bparees]

@smarterclayton updated per your comments. thoughts? notable and debatable design points:

1. retryable errors are retried forever (max attempts == -1) in a tight loop.

• i'm not averse to reducing this to 10 or 100, but since it's a tight loop my guess is anything that fails once will fail N times because the error condition won't be corrected fast enough for it to have a chance of succeeding later (eg the api endpoint becomes available again)

2. all errors (retried or not) go through kutil.HandleError

• if something is spinning in a retry loop it seems important to make that apparent in the logs, particularly since we're not going to eventually perm-fail it (see Add link to GOPATH doc and fixup typo #1)

[smarterclayton]

Godoc

[smarterclayton]

Minor comment, otherwise this is fine.

[bparees]

godoc'd. [merge]

[openshift-bot]

continuous-integration/openshift-jenkins/merge SUCCESS (https://ci.openshift.redhat.com/jenkins/job/merge_pull_requests_openshift3/1115/) (Image: devenv-fedora_973)

[openshift-bot]

Evaluated for origin up to 2841124

[smarterclayton]

post-hoc LGTM

----- Original Message -----

godoc'd. [merge]

---

Reply to this email directly or view it on GitHub:
#1194 (comment)