-
Notifications
You must be signed in to change notification settings - Fork 8.3k
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 condition of enqueue and unit test cases for task.Queue #113
Conversation
@@ -51,6 +51,11 @@ func (t *Queue) Run(period time.Duration, stopCh <-chan struct{}) { | |||
|
|||
// Enqueue enqueues ns/name of the given api object in the task queue. | |||
func (t *Queue) Enqueue(obj interface{}) { | |||
if t.IsShuttingDown() { |
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.
Good point
q.Shutdown() | ||
s := q.IsShuttingDown() | ||
if !s { | ||
t.Fatalf("queue shoule be shutdown") |
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.
shoule > should
/lgtm |
@aledbf: you can't LGTM a PR unless you are an assignee. In response to this comment:
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. |
/lgtm |
@chentao1596 Thanks! |
Hi, all the codes are related with task.Queue, so i merge them into one PR with two commits.
For the first commit:
I think it's necessary to add checking action before enqueue, because:
1) no need to execute this codes before "t.queue.Add(key)" if queue has been closes
2) log is needed if failed to enqueue
For the second commit:
add unit test cases
thank you!