Add button to suspend worker #94
Conversation
|
I feel that it is better to changing |
|
I agree. I changed it in 205da11. |
| @@ -39,3 +42,18 @@ | |||
| role: 'button', class: 'btn btn-sm btn-default') | |||
| - else | |||
| ' | |||
| td | |||
| - if !worker.suspended | |||
| = button_to('Suspend', | |||
There was a problem hiding this comment.
(nit) how about give confirm dialog to user? 🤔
There was a problem hiding this comment.
If user suspend worker accidentally, the user can unsuspend the worker. So I think confirm dialog is unneeded.
There was a problem hiding this comment.
In my case, I don't want to suspend a worker accidentally, even though I know I can restore it with only one click. anyway, that's not a big problem(why I tagged nit) ;)
There was a problem hiding this comment.
I understand. I added confirm dialog in eb5520e.
|
Would you resolve the conflicts? |
takonomura
force-pushed
the
feature/suspend-worker
branch
from
2fa5b37 to
94fd0d3
Compare
|
I resolved conflicts by rebasing. |
| params: { suspended: true }, | ||
| role: 'button', | ||
| class: 'btn btn-xs btn-default', | ||
| data: { confirm: 'Continue suspending the worker? If not, use "Cancel" button.' }) |
There was a problem hiding this comment.
[nits] "If not, use "Cancel" button" sounds too obvious to me
| @@ -19,7 +19,8 @@ def initialize | |||
| elsif worker_id == (Command::Executor.num_workers - 1) | |||
| Command::Monitor.new(hostname: @hostname, worker_id: worker_id) | |||
| else | |||
| @worker = Worker.where(hostname: @hostname, worker_id: worker_id, queue: @queue).first_or_create! | |||
| @worker = Worker.where(hostname: @hostname, worker_id: worker_id, queue: @queue).first_or_initialize! | |||
| @worker.update_column(:suspendable, true) | |||
There was a problem hiding this comment.
update_column skips varidations and callbacks and doesn't update updated_at. Why did you choose update_column instead of update! ?
http://api.rubyonrails.org/classes/ActiveRecord/Persistence.html#method-i-update_column
| @@ -19,7 +19,8 @@ def initialize | |||
| elsif worker_id == (Command::Executor.num_workers - 1) | |||
| Command::Monitor.new(hostname: @hostname, worker_id: worker_id) | |||
| else | |||
| @worker = Worker.where(hostname: @hostname, worker_id: worker_id, queue: @queue).first_or_create! | |||
| @worker = Worker.where(hostname: @hostname, worker_id: worker_id, queue: @queue).first_or_initialize! | |||
| @worker.update_column(:suspendable, true) | |||
There was a problem hiding this comment.
first_or_initialize! method doesn't exist.
There was a problem hiding this comment.
Thanks, I fixed in 34839bc.
| @@ -19,7 +19,8 @@ def initialize | |||
| elsif worker_id == (Command::Executor.num_workers - 1) | |||
| Command::Monitor.new(hostname: @hostname, worker_id: worker_id) | |||
| else | |||
| @worker = Worker.where(hostname: @hostname, worker_id: worker_id, queue: @queue).first_or_create! | |||
| @worker = Worker.where(hostname: @hostname, worker_id: worker_id, queue: @queue).first_or_initialize | |||
There was a problem hiding this comment.
first_or_initialize doesn't create a new record, is it? Did you try command-executor locally?
There was a problem hiding this comment.
I did. first_or_initialize doesn't create a new record, but update! at next line create a new record if not exist.
There was a problem hiding this comment.
How about Worker.where(...).first_or_create!(suspendable: true) ?
I cloudn't get the reason why you split into first_or_initialize + update!.
There was a problem hiding this comment.
Worker.where(...).first_or_create!(suspendable: true) creates a new record with suspendable: true, but doesn't update an existing record if the record has suspendable: false.
There was a problem hiding this comment.
OK, but I think creating a new record by update! is confusing
Add button to suspend worker. It prevent executing new task on the worker when the worker has suspended.
Kuroko Workers page
Before:
After: