Broker Ingress Tracing #1290
Broker Ingress Tracing #1290
Conversation
knative-prow-robot
added
the
do-not-merge/work-in-progress
knative-prow-robot
added
the
size/L
knative-prow-robot
added
the
approved
|
The following is the coverage report on pkg/.
|
|
The following is the coverage report on pkg/.
|
|
The following is the coverage report on pkg/.
|
|
The following is the coverage report on pkg/.
|
|
The following is the coverage report on pkg/.
|
knative-prow-robot
removed
the
do-not-merge/work-in-progress
|
The following is the coverage report on pkg/.
|
|
The following is the coverage report on pkg/.
|
|
Ping. |
|
The following is the coverage report on pkg/.
|
|
The following is the coverage report on pkg/.
|
| // func(*k8s.io/api/core/v1.ConfigMap) (... , error) | ||
| // | ||
| // These functions can return any type along with an error. | ||
| type DefaultConstructors map[*v1.ConfigMap]interface{} |
There was a problem hiding this comment.
Why is this a map? What are the semantics of a map keyed by struct pointers?
I think this would make more sense to me as:
type DefaultConstructorFunc func() *v1.ConfigMap
type DefaultConstructors map[string]DefaultConstructorFuncI think that would simplify this implementation since there's no need to store default configmaps.
There was a problem hiding this comment.
This is based on knative/pkg's configmap.Constructor:
type Constructors map[string]interface{}The suggestion drops the 'constructor' portion of this map. Essentially, what I am doing is changing the key from the name of the ConfigMap, to the entire ConfigMap.
As for pointer as key semantics, "Pointer values are comparable. Two pointer values are equal if they point to the same variable or if both have value nil. Pointers to distinct zero-size variables may or may not be equal.". So essentially, the pointer must be to the same place. The equality of what is being pointed at does not matter.
I tried to use non-pointers, but v1.ConfigMap's are "invalid map key type".
There was a problem hiding this comment.
I still don't understand why this needs to be a map with a pointer key. This would be equivalent, right?
type DefaultConstructor struct {
ConfigMap *v1.ConfigMap
Constructor func(*v1.ConfigMap) (interface{}, error)
}
func NewDefaultUntypedStore(..., defaultConstructors []DefaultConstructor, ...)I still like my version above better, but the array of structs makes more sense to me than the map.
WDYT @Harwayne? Do you like pkg's configmap.Constructor?
There was a problem hiding this comment.
Switched to slice of structs.
The reason I was using a map to begin with was to protect against the same thing being registered twice. But that protection didn't actually occur. Both because it was a pointer and because I really wanted the key to just be the name of the ConfigMap, not its whole value.
|
The following is the coverage report on pkg/.
|
|
Ping. |
|
It looks good to me, but I'll let @grantr to lgtm as he had some comments. |
cmd/broker/ingress/main.go
Outdated
| kc := kubernetes.NewForConfigOrDie(mgr.GetConfig()) | ||
| configMapWatcher := configmap.NewInformedWatcher(kc, env.Namespace) | ||
|
|
||
| zipkinServiceName := fmt.Sprintf("%s-broker-ingress.%s", env.Broker, env.Namespace) |
There was a problem hiding this comment.
I think I'd like this to be a function so it can be unit tested.
There was a problem hiding this comment.
Done. Even added a test :)
| // func(*k8s.io/api/core/v1.ConfigMap) (... , error) | ||
| // | ||
| // These functions can return any type along with an error. | ||
| type DefaultConstructors map[*v1.ConfigMap]interface{} |
There was a problem hiding this comment.
I still don't understand why this needs to be a map with a pointer key. This would be equivalent, right?
type DefaultConstructor struct {
ConfigMap *v1.ConfigMap
Constructor func(*v1.ConfigMap) (interface{}, error)
}
func NewDefaultUntypedStore(..., defaultConstructors []DefaultConstructor, ...)I still like my version above better, but the array of structs makes more sense to me than the map.
WDYT @Harwayne? Do you like pkg's configmap.Constructor?
|
The following is the coverage report on pkg/.
|
|
|
||
| import "fmt" | ||
|
|
||
| // BrokerIngressNameArgs are the arguments needed to generate the BrokerIngressName. |
There was a problem hiding this comment.
I guess the reasoning for this struct is that it names the arguments so they can be passed in any order and are self documenting. I'd probably use positional parameters instead, but I'm fine with this too.
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: grantr, Harwayne The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
Helps with #704.
Proposed Changes
Release Note