Avoid raising and re-queueing when the remote resource is not found #17745
Conversation
| _log.info("Invoking task #{action} on collection #{collection_name}, object #{obj.id}, with args #{post_args}") | ||
| obj.send(action, post_args) | ||
| rescue RuntimeError => err | ||
| raise unless err.message =~ /couldn't find resource with 'id' \["\d+"\]/i |
There was a problem hiding this comment.
This regex feels too specific and the message could change a little and suddenly break this. Does it raise a useful exception class we can rescue instead? If not, is /couldn't find resource with 'id'/i enough?
There was a problem hiding this comment.
It does not raise a useful exception class. That's the entire error message.
There was a problem hiding this comment.
I feel like we want to raise a UnretryableException or something like that (better name) and have invoke_tasks_remote catch it, log it and move on. I think swallowing it here hides the fact that we're explicitly skipping re-queueing.
There was a problem hiding this comment.
Maybe we can rescue just around the collection.find(id) call on line 123?
Then we can avoid the conditional entirely I think.
There was a problem hiding this comment.
Maybe?
obj = collection.find_by(:id => id)
if obj
_log.info("Invoking task #{action} on collection #{collection_name}, object #{obj.id}, with args #{post_args}")
obj.send(action, post_args)
endThere was a problem hiding this comment.
@kbrock does the API client return nil for find_by rather than an exception?
There was a problem hiding this comment.
@carbonin aah - not sure. I was assuming it followed AR conventions - which is probably wrong
thanks
| _log.info("Invoking task #{action} on collection #{collection_name}, object #{obj.id}, with args #{post_args}") | ||
| obj.send(action, post_args) | ||
| rescue RuntimeError => err | ||
| raise unless err.message =~ /couldn't find resource with 'id' \["\d+"\]/i |
There was a problem hiding this comment.
Maybe?
obj = collection.find_by(:id => id)
if obj
_log.info("Invoking task #{action} on collection #{collection_name}, object #{obj.id}, with args #{post_args}")
obj.send(action, post_args)
end
| @@ -68,6 +68,8 @@ def invoke_tasks_remote(options) | |||
| begin | |||
| remote_connection = InterRegionApiMethodRelay.api_client_connection_for_region(region, remote_options[:userid]) | |||
| invoke_api_tasks(remote_connection, remote_options) | |||
| rescue ManageIQ::API::Client::ResourceNotFound => err | |||
There was a problem hiding this comment.
I think this is a good way to rescue the error, but wouldn't this bail out before all of the resources were processed in invoke_api_tasks?
Why not leave this rescue inside the resource loop in that method?
|
Checked commit bdunne@a6343cf with ruby 2.3.3, rubocop 0.52.1, haml-lint 0.20.0, and yamllint 1.10.0 |
|
I'm good with this, @carbonin was your comment addressed? |
Fixes https://bugzilla.redhat.com/show_bug.cgi?id=1599754