[WIP] Zap logging#281
Conversation
google-prow-robot
added
the
size/XXL
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: n3wscott If they are not already assigned, you can assign the PR to them by writing 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 |
|
todo, merge with 540e451 |
|
@n3wscott: The following tests failed, say
Full PR test history. Your PR dashboard. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here. |
google-prow-robot
added
the
do-not-merge/work-in-progress
|
/assign @grantr Grant, Please take a look because this will change how the Feed controller is written when it gets there. |
| return channelsv1alpha.SchemeGroupVersion.WithKind("Flow") | ||
|
|
||
| default: | ||
| panic(fmt.Sprintf("Unsupported object type %T", obj)) |
There was a problem hiding this comment.
If a type is added or renamed to any of these groups, this switch needs to be updated. I'm concerned about that coupling. If an object ever gets in the queue that this switch doesn't know about, the panic here will cause the controller to crash loop.
IIUC, the purpose of this switch is to enable the following:
cntrl.NewControllerRef(obj)The alternative without the switch is:
metav1.NewControllerRef(obj, foov1alpha1.SchemeGroupVersion.WithKind("Bar"))I think I'd prefer to make the calling code more explicit and eliminate the panic.
There was a problem hiding this comment.
I think the point of not passing in the kind is you don't want/need to know what it is. Plus unit tests should catch the panic when a new type is added
| return channelsv1alpha.SchemeGroupVersion.WithKind("ClusterBus") | ||
| // Feeds | ||
| case *feedsv1alpha.ClusterEventType: | ||
| return channelsv1alpha.SchemeGroupVersion.WithKind("ClusterEventType") |
There was a problem hiding this comment.
This and following should be feedsv1alpha
| return channelsv1alpha.SchemeGroupVersion.WithKind("EventSource") | ||
| // Flows | ||
| case *flowsv1alpha.Flow: | ||
| return channelsv1alpha.SchemeGroupVersion.WithKind("Flow") |
There was a problem hiding this comment.
This should be flowsv1alpha1.
| // to status with this stale state. | ||
| } else if _, err := c.updateStatus(flow); err != nil { | ||
| glog.Warningf("Failed to update flow status: %v", err) | ||
| logger.Warnf("Failed to update flow status: %v", zap.Error(err)) |
There was a problem hiding this comment.
Instead of using format directives, uselogger.Warning("Failed to update flow status", zap.Error(err)) so the error is logged as a structured field.
I'd argue this should be usinglogger.Error instead of logger.Warning, but there might be disagreement about that.
…tive#281) Signed-off-by: Pierangelo Di Pilato <pierdipi@redhat.com>
3 participants
Related: #274
And I had to rewrite a bit of the controller for flow....
This might be too large to merge and we might want to refactor how Serving is doing the base controller class so we can leverage the shared code a little better. Please give comments.