pkg/sdk: Make number of workers configurable #418
Conversation
pkg/generator/generator_test.go
Outdated
| logrus.Infof("Watching %s, %s, %s, %d", resource, kind, namespace, resyncPeriod) | ||
| sdk.Watch(resource, kind, namespace, resyncPeriod) | ||
| sdk.Watch(resource, kind, namespace, resyncPeriod, numWorkers) |
There was a problem hiding this comment.
I think we should make sdk.Watch to accept optional arguments like the Get API:
func Watch(apiVersion, kind, namespace string, resyncPeriod int, opts ...WatchOption)
Then one of the WatchOption can be:
func WithNumberOfWorkers(int numWorkers) WatchOption.
In that way, we don't break the backward compatibility.
There was a problem hiding this comment.
I would also like to understand how this might change with regards to controller runtime. Especially, before making a backward incompatible change and then maybe needing to do another after the controller runtime refactor.
There was a problem hiding this comment.
There was a problem hiding this comment.
@shawn-hurley
I would also like to understand how this might change with regards to controller runtime
Currently, we are not sure how controller runtime affects the overall API change. we will deal with it when it comes.
There was a problem hiding this comment.
@shawn-hurley The controller-runtime supports configuring the number of workers out of the box via the controller options:
https://github.com/kubernetes-sigs/controller-runtime/blob/master/pkg/controller/controller.go#L32-L34
So this feature should be supported eventually once we use the controller-runtime.
But in the meantime if we can still support this without breaking backwards compatibility then it's worth adding right now since this has been requested for quite some time.
|
Now follows the functional options pattern |
pkg/sdk/watch-opt.go
Outdated
| type WatchOption func(*WatchOp) | ||
|
|
||
| // WithWatchOptions sets the number of workers for the Watch() operation. | ||
| func WithWatchOptions(numWorkers int) WatchOption { |
There was a problem hiding this comment.
You can name this function to match the meaning of the wanted option for readability. for example,
func WithNumOfWorks(numWorkers int) WatchOption is probably more intuitive.
|
lgtm defer @hasbro17 |
|
We need to document the usage of this Watch Option and what it does. Ideally we could have a separate doc for it in For now @theishshah can you just make a small note of this option in the user guide section for Watch. |
|
can this code change with the functional options be used to pass labels to the watch? I know that the goal is to have the controller-runtime do that nested watching but it seems we are far off from that refactor and I think some of us are blocked on things like finalizers without them. Can we have a ticketed (issue) conversation about using this? |
|
@CSdread Can you please open up a separate issue for that. If I understand correctly, your use case for passing labels to sdk.Watch() is to filter out events for that label selector. That would be done with predicates in the controller-runtime in the future. But like you pointed out, that refactor will still take some time. Lets discuss this in a separate issue to see if we can do something else in the meanwhile. |
doc/user-guide.md
Outdated
| sdk.Handle(stub.NewHandler()) | ||
| sdk.Run(context.TODO()) | ||
| } | ||
| ``` |
There was a problem hiding this comment.
Note: You can configure the number of concurrent informer workers with an additional Watch option:
sdk.Watch("cache.example.com/v1alpha1", "Memcached", "default", 5, sdk.WithNumWorkers(n))There was a problem hiding this comment.
Also mention that the default is 1 over here.
doc/user-guide.md
Outdated
| @@ -58,11 +58,20 @@ By default, the memcached-operator watches `Memcached` resource events as shown | |||
|
|
|||
| ```Go | |||
| func main() { | |||
| // Default number of worker to 1 | |||
There was a problem hiding this comment.
Remove this. And mention it in the note below.
pkg/sdk/watch-opt.go
Outdated
| package sdk | ||
|
|
||
| // WatchOp wraps all the options for Watch(). | ||
| type WatchOp struct { |
There was a problem hiding this comment.
In the past we've needlessly exported structs and utility functions when they aren't meant to be exposed outside the sdk pkg, e.g we do this with the query and action options with NewQueryOps() etc.
But for this PR let's keep that to a minimum. We only need to export WithNumWorkers(). Everything else is internal to the pkg. So keep this unexported:
type watchOp struct {
pkg/sdk/watch-opt.go
Outdated
| } | ||
|
|
||
| // NewWatchOp create a new deafult WatchOp | ||
| func NewWatchOp() *WatchOp { |
There was a problem hiding this comment.
unexport this as well:
func newWatchOp() *watchOp {
|
LGTM |
1 similar comment
|
LGTM |
The informer struct now has a field for numWorkers which is populated in
main.goof a generated operator. Currently defaults to 1Issue #140