Fix #1211: Return better error for plugin not found when starting a task #1242
Conversation
| @@ -383,7 +377,7 @@ func (s *subscriptionGroup) subscribePlugins(id string, | |||
| for i, sub := range plugins { | |||
| plg, err := s.pluginManager.get(key(sub)) | |||
| if err != nil { | |||
| serrs = append(serrs, serror.New(err)) | |||
| serrs = append(serrs, pluginNotFoundError(sub)) | |||
There was a problem hiding this comment.
PluginNotFound is not the only error that can be returned from the pluginManager.get call. Do we need to worry about badkey errors also?
There was a problem hiding this comment.
I'm looking through the code to see if there any chance the code could hit that error.
There was a problem hiding this comment.
Looking at the code, it's seems the check for a bad key is only if the plugin is not found and breaking the key apart to see if the latest plugin needs to be grabbed. The same error bad key is returned for multiple scenarios, so it's never clear what part of the key is bad.
pluginManager.get could return a clearer error on what plugin was not found, so then creating this error in subscription_group.go would then not be needed.
There was a problem hiding this comment.
Another example in the code base of treating any error returned from pluginManager.get as plugin not found. https://github.com/intelsdi-x/snap/blob/master/control/plugin_manager.go#L509-L518
There was a problem hiding this comment.
Do you think pluginManager.get should be in charge of returning better errors?
There was a problem hiding this comment.
After looking at the code, I would think the error handling would be better suited in pluginManager.get instead of loadedPlugins.get. At that point, loadedPlugins.get could possibly be simplified to return the loadedPlugin or nil, and let pluginManager.get decide what error to return: pluginNotFound or invalid key when no plugin is returned.
There was a problem hiding this comment.
Is that something you were going to do as part of this PR?
There was a problem hiding this comment.
I feel refactoring that code would be better in another PR.
|
LGTM |
Fixes #1211
Summary of changes:
Testing done:
@intelsdi-x/snap-maintainers