Add to controller framework; use in scheduler #6546
Conversation
|
|
||
| // Run the controller and run it until we close stop. | ||
| stop := make(chan struct{}) | ||
| controller.Run(stop) |
There was a problem hiding this comment.
Shouldn't this be wrapped in a goroutine?
There was a problem hiding this comment.
Run itself starts two goroutines.
There was a problem hiding this comment.
I figured. My point was that, as I understand, the general Go pattern is
for the caller to create the goroutine, rather than a library function.
It's all perspective, i guess, but as a consumer of this library I would
expect to say go controller.Run(stop) Not a huge deal.
On Tue, Apr 7, 2015 at 5:16 PM, Daniel Smith notifications@github.com
wrote:
In pkg/controller/framework/controller_test.go
#6546 (comment)
:
key, err := framework.DeletionHandlingMetaNamespaceKeyFunc(obj)if err != nil {key = "oops something went wrong with the key"}
// Record some output when items are deleted.outputSetLock.Lock()defer outputSetLock.Unlock()outputSet.Insert(key)},},- )
- // Run the controller and run it until we close stop.
- stop := make(chan struct{})
- controller.Run(stop)
Run itself starts two goroutines.
—
Reply to this email directly or view it on GitHub
https://github.com/GoogleCloudPlatform/kubernetes/pull/6546/files#r27933403
.
There was a problem hiding this comment.
Yeah, I don't have a strong opinion, but I wasn't aware of that pattern and I think we're (I am) breaking it all over the place... hm ok I can change this.
|
Looks nice. |
|
shippable is a flake, I filed #6548 |
|
Hm, I want to extend the testing a bit before this merges. Will update this tomorrow. |
|
OK, this is ready to go. @davidopp |
|
Shippable is an unrelated flake. |
|
Rebased |
|
Subscribe so I can track this merge to know when to update namespace controller so deletion is watch based and not just periodic synch |
| @@ -74,6 +77,10 @@ func MetaNamespaceKeyFunc(obj interface{}) (string, error) { | |||
| return meta.Name(), nil | |||
| } | |||
|
|
|||
| // ExplicitKey can be passed to MetaNamespaceKeyFunc if you have the key for | |||
| // the object but not the object itself. | |||
| type ExplicitKey string | |||
There was a problem hiding this comment.
Move this above definition of MetaNamespaceKeyFunc() to improve readability?
|
Comments addressed, rebased, some commits squashed-- PTAL |
lavalamp
force-pushed
the
fix
branch
2 times, most recently
from
89038c5 to
a44e81b
Compare
|
Just a couple of remaining nits. BTW, after you have submitted everything for controller framework, can you write up a doc explaining how to use it? The comments are good but I think having a HOWTO kind of thing would help people who want to write their own controllers. |
|
Comments addressed-- I changed the update function, since it's not hard and seems useful. Also added another test. PTAL-- this is blocking a number of others, so I'd like to fix further comments in a followup PR if you don't want anything major changed. |
|
And yes, I can write up a doc. BTW, the Example() functions will show up in godoc. |
| // change. OnUpdate is also called when a re-list happens, and it will | ||
| // get called even if nothing changed. This is useful for periodically | ||
| // evaluating or syncing something. | ||
| // * OnDelete will get the final state of the item if it is known, otherwise |
There was a problem hiding this comment.
In a followup PR, it might be good to add some detail to this comment explaining under what circumstances the final state would be known and when it wouldn't. That will help a client know when it can depend on getting this.
|
I made some comments/questions/suggestions, none of them are blockers for merging but it seems that Travis is failing on v1beta3 with a race condition related to this PR, so you should probably look into that before we merge. |
|
Actually this race is possible without this PR. I have a fix, but it relies on a PR that hasn't merged yet. |
|
If you think this PR doesn't make things worse, feel free to merge. |
|
OK, I've fixed the race. |
|
Hm, still another test problem, it's too slow... looking at it. |
|
OK, let's see if travis still chokes on this. |
|
OK, I will add the comments you requested in a follow-up. |
Add to controller framework; use in scheduler
| v := reflect.New(reflect.TypeOf(source)) | ||
|
|
||
| buff := &bytes.Buffer{} | ||
| enc := gob.NewEncoder(buff) |
There was a problem hiding this comment.
Creating an encoder on the fly makes this much more expensive than it has to be, because all the reflections have to be recopied. The encoder should be static (assuming we have a finite number of types) and that will make this much less expensive.
Fixes #6524