Only update the feed in one place#308
Conversation
google-prow-robot
added
the
size/M
|
/assign @adamharwayne For initial review. /assign evankanderson For approve. |
When there are multiple places a feed can be updated, resourceVersion conflicts become more likely. This splits Reconcile into two functions: - an inner function that reconciles the state of a feed - an outer function that loads the feed, calls the inner function if the feed exists, then updates the feed. This separation of responsibilities reduces the likelihood that a feed will be updated more than once per reconcile.
|
|
||
| if err = r.reconcile(feed); err != nil { | ||
| glog.Errorf("error reconciling Feed: %v", err) | ||
| } |
There was a problem hiding this comment.
Why is the feed being updated even if the reconcile failed? And, then after updating the feed, returning this error anyway.
There was a problem hiding this comment.
Since the reconcile is a sequence of steps, earlier steps may complete successfully but later steps may fail. The feed is updated on failure to preserve any useful status or metadata changes the non-failing steps made. Partial status is more useful than no status at all.
It could be argued that we should update status after every step as a checkpoint rather than at the end of reconcile. I believe conventional wisdom advises against that, but I don't have all the evidence at hand to make a complete argument. I'll create an issue in docs about writing that evidence down. 😄
Returning the error causes the feed to be requeued for another reconcile immediately (subject to backoff). Since we know the reconcile didn't complete successfully, we want to try again asap.
I'll add a version of this as a comment.
| Reason: t.Reason, | ||
| Message: t.Message, | ||
| }) | ||
| default: |
There was a problem hiding this comment.
The default case was dropped. Was that intended?
Although, looking at r.getFeedSource(feed), it can only return these two types of errors. Is it proper golang style to rely on that? Or should we have a default in case the implementation of getFeedSource changes?
There was a problem hiding this comment.
An earlier version of this had return err at https://github.com/knative/eventing/pull/308/files#diff-aadf2ebf3523fdeb1c45a8f0ae1ae27eR100 instead of return nil, so the default case did the same thing as the statement immediately after the switch. Now the default case would do something different, so it's probably worth putting it back in.
| return reconcile.Result{}, err | ||
| } | ||
|
|
||
| if feed.GetDeletionTimestamp() == nil { |
There was a problem hiding this comment.
It looks like this starts jobs that will act on the feed. Because we have delayed the updateFeed call until after this, is there a chance we are introducing a race condition (albeit a rare one, because this function and its caller should return long, long before the job is scheduled and actually starts running)?
There was a problem hiding this comment.
I don't think there's a race condition because the Job and its children don't depend on the status of the feed (currently). The Job pod gets all the information it needs through env vars.
If we later come up with a feedlet that depends on Feed status existing before the feedlet starts, then we may have to revisit.
Reconcile error handling is subtle and perhaps nonintuitive, so make it clear to new contributors.
This default case is currently unreachable, but future changes may start returning different error types from getFeedSource, so this is defensive programming.
|
The following is the coverage report on pkg/.
|
|
@adamharwayne RFAL. |
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: evankanderson, grantr The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
google-prow-robot
added
the
approved
* Only update the feed in one place When there are multiple places a feed can be updated, resourceVersion conflicts become more likely. This splits Reconcile into two functions: - an inner function that reconciles the state of a feed - an outer function that loads the feed, calls the inner function if the feed exists, then updates the feed. This separation of responsibilities reduces the likelihood that a feed will be updated more than once per reconcile. * Document Reconcile flow Reconcile error handling is subtle and perhaps nonintuitive, so make it clear to new contributors. * Add a default case for feed source errors This default case is currently unreachable, but future changes may start returning different error types from getFeedSource, so this is defensive programming.
5 participants
When there are multiple places a Feed can be updated, resourceVersion conflicts become more likely. This PR splits
Reconcileinto two functions:reconcilethat reconciles the state of a Feed without updating itReconcilethat loads the Feed, calls the inner function if theFeed exists, then updates the Feed.
This separation of responsibilities reduces the likelihood that a Feed will be updated more than once per reconcile.
@adamharwayne this might fix the problem you were having yesterday with Feeds not getting a status when EventSource and/or EventType were missing.