Exposed a PluginManager.isPluginInstalled(String name) method for easier embedded use #5566
Conversation
… method to make it a bit cleaner using the PluginManager embedded (when running elastic embedded). Did a bit of a refactor to reduce some duplication (that the above change introduced - and little bit of existing). Not sure if it was intentional - but there was a bit of mixed usage of IllegalArgumentException and ElasticsearchIllegalArgumentException - so I standardised on ElasticsearchIllegalArgumentException. There was also some validation on the name when removing a plugin - that is worthwhile having when installing a plugin - so I made that common.
| @@ -543,4 +421,170 @@ static PluginHandle parse(String name) { | |||
| } | |||
| } | |||
|
|
|||
| class Plugin { | |||
There was a problem hiding this comment.
class should itself be static if possible (hard to tell from diffs, but it looks like everything is passed into it)
There was a problem hiding this comment.
At the moment it accesses environment (AFAIR). But yes, it could easily be static by passing that in.
There was a problem hiding this comment.
Actually, no theres quite a bit of stuff it accesses - but one method I just noticed should move to Plugin. You could pass the PluginManager in - but might be better leaving as a non-static inner class.
There was a problem hiding this comment.
Personally, I don't think that the Plugin should need to know about the PluginManager; the environment made sense to pass in, but I don't know if it's worth it to you all to refactor it to change the manager-to-plugin relationship. I was hoping that it would be an easy change.
There was a problem hiding this comment.
I don't think having the non-static inner class is perfect, but neither do I think extracting has been a net negative impact.
After reviewing elastic#5566. Closes elastic#5565. Closes elastic#5566.
|
Hey @nickminutello! Thanks for the PR. I rebased it on latest master branch and added a new commit to make Closing this PR in favor of #5977. Let's continue this thread there if needed. Thanks again! |
|
Hey @nickminutello I just took a look at this PR and have a couple of questions. You said you are running elasticsearch in embedded mode. This means it is easy for you to access the running instance. Wouldnt it be easier, if you ran the nodes info API and checked the plugin results in there to find out which plugins have been loaded. What sounds like a huge task, compared to your implementation, actually has one huge benefit: If the loading of a plugin fails (maybe due to missing dependencies), your implemented method will return true, because the directory exists. However the nodes info response will include the expected information. Does that make sense or do you have some specific use-case, which I am not seeing at the moment and where your approach makes more sense? |
|
Hi @spinscale, sorry I didnt see your question till now. For prod-env deployments, we will have to include the plugin in the deployment zip, since elastic wont have access to the internet. |
Exposed a PluginManager.isPluginInstalled(String name) method to make it a bit cleaner using the PluginManager embedded (when running elastic embedded).
Did a bit of a refactor to reduce some duplication (that the above change introduced - and little bit of existing).
Not sure if it was intentional - but there was a bit of mixed usage of IllegalArgumentException and ElasticsearchIllegalArgumentException - so I standardised on ElasticsearchIllegalArgumentException.
There was also some validation on the name when removing a plugin - that is worthwhile having when installing a plugin - so I made that common.
Issue #5565