(SDI-1827) Fix #1128 Loading collector twice causes task failure

[marcin-krolik]

Fixes #1128

Summary of changes:

• after plugin response is received, additional check added to look if plugin already exists in loaded plugins table

Testing done:

• small
• legacy

@intelsdi-x/snap-maintainers

[IRCody]

Are you able to reproduce the issue from #1128 on master? I tried again just now and wasn't able to. Curious what steps I might be missing, can you provide the steps so I can properly test the PR?

[marcin-krolik]

@IRCody I can't reproduce this issue either on current master or on df51bec which is the last commit before the issue was reported. Weird ...

[IRCody]

I was able to repro it at the time so that is very weird. I don't feel confident merging this fix though if we can't reproduce the original error and verify it's fixed.

[marcin-krolik]

@IRCody I got your point, but at the same time if plugin is already loaded, this change helps to stop loading early, right after response is received.

[IRCody]

@marcin-krolik: I think we still need to track down why we can't reproduce the issue and find out what's going on.

[marcin-krolik]

@IRCody I used git bisect to track down the issue. This is quite messy, because at that time we were working on simplification of interaction between scheduler and control, and there are Matuesz's commits, mine and Joel's and it is hard to track it, but here is what I found:

• Last commit with issue reproducible 3e91e77
• Next commit is Matuesz commit and I could not compile snap with it
• Following commit has this issue resolved 2afb26b

[IRCody]

@marcin-krolik: Thanks for tracking it down. Do you think this PR is still needed? Is the advantage that it would fail earlier?

[jcooklin]

After subscription groups was landed this issue was avoided because we get the loaded plugin through the pluginManager here.

The fact that metricType has a reference to a loaded plugin that can potentially be invalid is still a problem and should be fixed.

@marcin-krolik What do you think about changing the type of metricType.Plugin from *loadedPlugin to core.CatalogedPlugin. This will remove the the fact that the catalog has a reference to an invalid loadedPlugin (invalid in the sense that the path to the plugin is wrong). Your fix avoids hitting this with an invalid loadedPlugin but it might be cleaner to not store a reference to the loadedPlugin in the metricCatalog.

[marcin-krolik]

@jcooklin I like this idea. I'm going to start working on it.

[IRCody]

@marcin-krolik: I'm going to go ahead and close this PR. Feel free to re-open this one when you're ready for a review.