-
Notifications
You must be signed in to change notification settings - Fork 2.9k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add workqueue example #65
Conversation
Thanks for your pull request. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). 📝 Please follow instructions at https://github.com/kubernetes/kubernetes/wiki/CLA-FAQ to sign the CLA. Once you've signed, please reply here (e.g. "I signed it!") and we'll verify. Thanks.
Instructions 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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll let @caesarxuchao do a review too but these are some things I noticed while glancing through.
return false | ||
} | ||
// Tell the queue that we are done with processing this key. This unblocks the key for other workers | ||
// This allows save parallel processing because two pods with the same key are never processed in |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
s/save/safe/
} else { | ||
queue.Forget(key) | ||
} | ||
glog.Fatalf("Invalid key %s", key.(string)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this line seems wrong
|
||
if !exists { | ||
// Below we will warm up our cache with a Pod, so that we will see a delete for one pod | ||
fmt.Printf("Pod %s does not exist anymore\n", key.(string)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Use glog for log messages (or the built-in log package).
// Wait forever | ||
for { | ||
time.Sleep(60000 * time.Millisecond) | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
select {} is a more elegant way to block forever
type ControllerFunc func(cache.Indexer, workqueue.RateLimitingInterface) bool | ||
|
||
func (c *Controller) Run(threadiness int, stopCh chan struct{}) { | ||
defer NewPanicCatcher() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This line isn't doing what you think it's doing :)
// Create the connection | ||
config, err := clientcmd.BuildConfigFromFlags(master, kubeconfig) | ||
if err != nil { | ||
panic(err.Error()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
glog.Fatalf
import ( | ||
"flag" | ||
"fmt" | ||
"github.com/golang/glog" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
add blank lines so that there's three sections: builtin packages, k8s.io packages, 3rd party dependencies
thanks for the example! |
Thx for the review. Should have addressed all issues, but I am still not sure what the best practice is when we can't retrieve an item from the store in the process loop. Review status: 0 of 2 files reviewed at latest revision, 7 unresolved discussions, some commit checks failed. examples/workqueue/main.go, line 22 at r1 (raw file): Previously, lavalamp (Daniel Smith) wrote…
Done. examples/workqueue/main.go, line 64 at r1 (raw file): Previously, lavalamp (Daniel Smith) wrote…
Done. examples/workqueue/main.go, line 89 at r1 (raw file): Previously, lavalamp (Daniel Smith) wrote…
Done. examples/workqueue/main.go, line 138 at r1 (raw file): Previously, lavalamp (Daniel Smith) wrote…
Done. examples/workqueue/main.go, line 156 at r1 (raw file): Previously, lavalamp (Daniel Smith) wrote…
Should be more correct now. Still don't know if this is a good practice what I do here ... examples/workqueue/main.go, line 162 at r1 (raw file): Previously, lavalamp (Daniel Smith) wrote…
Done. examples/workqueue/main.go, line 188 at r1 (raw file): Previously, lavalamp (Daniel Smith) wrote…
Done. Comments from Reviewable |
func init() { | ||
flag.StringVar(&kubeconfig, "kubeconfig", "", "absolute path to the kubeconfig file") | ||
flag.StringVar(&master, "master", "", "master url") | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why not put flags and global variables into main()
? Global state hurts code reusability/etc.
|
||
func (c *Controller) Run(threadiness int, stopCh chan struct{}) { | ||
// Don't crash the loop on a panic | ||
defer catchPanic() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IMHO this is not relevant for this example. Also, again IMHO, it is not a great design choice :) Panic means something is completely wrong and there is no way to handle it. Why catch it?
@k8s-ci-robot I signed it |
Review status: 0 of 2 files reviewed at latest revision, 9 unresolved discussions. examples/workqueue/main.go, line 44 at r2 (raw file): Previously, ash2k (Mikhail Mazurskiy) wrote…
Done. examples/workqueue/main.go, line 66 at r2 (raw file): Previously, ash2k (Mikhail Mazurskiy) wrote…
Agreed that it is not important for the example, removed it. Had it there because it was in https://github.com/kubernetes/community/blob/master/contributors/devel/controllers.md. IMHO, if catching panics is important or not, depends on how you do logging and deployment, it basically depends on how your infra looks like. Another common example are REST applications. Very often they catch a panic on the outer rim of the application, log the stacktrace, do some notification, return 500 and go on ... I would say it is more important that you don't try to handle it in the business logic. Comments from Reviewable |
@caesarxuchao @lavalamp anything more you want me to change? |
@mbohlool it would be great if you can also take a look at the pull. |
@rmohr @caesarxuchao Should this PR sent against main repo vs client-go repo? |
The main repo please. It will be copied over to client-go. [edit] but the robot is down currently, so it might take some time to sync. |
Ok, will do a PR against the main repo 👍 |
@rmohr Thanks. Let me know when you send that PR, and I will review it with priority. |
workqueue is not in client-go 2.0, which kind of queue should I use? |
@ymruan is it possible to use the workqueue in k8s.io/kubernetes release-1.5? I guess it will work. |
Demonstrates how to compose a controller out of cache.Controller, cache.Indexer and a workqueue.
Tested on kubernetes 1.4.5.