-
Notifications
You must be signed in to change notification settings - Fork 898
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
If we can't update_attributes on a queue row, set state to error #14365
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -35,7 +35,7 @@ def thresholds_exceeded? | |
def get_message_via_drb | ||
loop do | ||
begin | ||
msg_id, lock_version = @worker_monitor_drb.get_queue_message(@worker.pid) | ||
msg_id, lock_version = worker_monitor_drb.get_queue_message(@worker.pid) | ||
rescue DRb::DRbError => err | ||
do_exit("Error communicating with WorkerMonitor because <#{err.message}>", 1) | ||
end | ||
|
@@ -64,6 +64,7 @@ def get_message_via_drb | |
_log.debug("#{log_prefix} #{MiqQueue.format_short_log_msg(msg)} stale, retrying...") | ||
next | ||
rescue => err | ||
msg.update_column(:state, MiqQueue::STATUS_ERROR) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What if this fails? Say, if the database is down? should we do some kind of There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't think it is needed. If we have got here, the database was running a second ago. If it is no longer running, we cannot do anything sensible at that point, thus we can fail horribly. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. So @Fryguy I was debating if I should just delete the row if this fails but I really can't understand how this line could throw an error if we retrieved the |
||
raise _("%{log} \"%{error}\" attempting to get next message") % {:log => log_prefix, :error => err} | ||
end | ||
end | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,24 @@ | ||
describe MiqQueueWorkerBase::Runner do | ||
context "#get_message_via_drb" do | ||
let(:server) { EvmSpecHelper.local_miq_server } | ||
let(:worker) { FactoryGirl.create(:miq_generic_worker, :miq_server => server, :pid => 123) } | ||
let(:runner) do | ||
allow_any_instance_of(MiqQueueWorkerBase::Runner).to receive(:sync_active_roles) | ||
allow_any_instance_of(MiqQueueWorkerBase::Runner).to receive(:sync_config) | ||
allow_any_instance_of(MiqQueueWorkerBase::Runner).to receive(:set_connection_pool_size) | ||
described_class.new(:guid => worker.guid) | ||
end | ||
|
||
it "sets message to 'error' and raises for unhandled exceptions" do | ||
# simulate what may happen if invalid yaml is deserialized | ||
allow_any_instance_of(MiqQueue).to receive(:args).and_raise(ArgumentError) | ||
q1 = FactoryGirl.create(:miq_queue) | ||
allow(runner) | ||
.to receive(:worker_monitor_drb) | ||
.and_return(double(:get_queue_message => [q1.id, q1.lock_version])) | ||
|
||
expect { runner.get_message_via_drb }.to raise_error(StandardError) | ||
expect(q1.reload.state).to eql(MiqQueue::STATUS_ERROR) | ||
end | ||
end | ||
end |
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.
We already have a
attr_reader
on the@worker_monitor_drb
so i'm using it here so it's easier to test.