-
Notifications
You must be signed in to change notification settings - Fork 8
Add multiple tasks at once. #51
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 multiple tasks at once. #51
Conversation
| // EXEC | ||
| watchingKeys := []string{ | ||
| b.tasksKey(queue.UID.String()), | ||
| b.taskKey(queue.UID.String(), newTask.UID), |
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.
新しいタスクをwatchする必要はないんじゃないかと思うので、消しています。もし間違ってそうだったら指摘いただけますと助かります。
shioshiota
left a comment
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.
LGTM
| Interface("task", task). | ||
| Msg("Task added to queue") | ||
| } | ||
| taskSpecs = append(taskSpecs, taskSpec) |
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.
ここでvalidateTaskSpecできると、挙動が変わらず良いかと思ったんですが、backendのinterface変更が必要そうですし、このままで良いかも知れないですね。
shioshiota
left a comment
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.
@ryotarai
CI落ちてる部分確認お願いします 🙏
shioshiota
left a comment
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.
LGTM
Benchmark