Expose a PluginManager.isPluginInstalled(String name) method #5977

dadoonet · 2014-04-29T10:51:51Z

@nickminutello did a nice work to expose isInstalled(String) method in PR #5566.

After reviewing it, I made the Plugin class static.
This changes needs also to be reviewed :)

…ier embedded use 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. Closes elastic#5565.

After reviewing elastic#5566. Closes elastic#5565. Closes elastic#5566.

pickypg · 2014-04-29T16:07:11Z

src/main/java/org/elasticsearch/plugins/PluginManager.java

+            File pluginFile = distroFile();
+            File extractLocation = extractedDir();
+            ZipFile zipFile = null;
+            try {


Can change this to the new autoclose functionality in Java 7 now that the codebase is on it:

try (ZipFile zipFile = new ZipFile(pluginFile)) { // ... } catch (Exception e) { // ... }

Thereby dropping the entire zipFile-related code from within the finally block.

Agreed. Thanks.

dadoonet · 2014-05-07T12:25:55Z

Hey @pickypg!

Thanks for the review. I applied most of proposed changes. Looks better now.
Some changes are not applied here as they concern IMHO another issue.

Let me know what you think.

pickypg · 2014-05-08T02:24:50Z

src/main/java/org/elasticsearch/plugins/PluginManager.java

+            if (outputMode != OutputMode.SILENT) System.out.println(line);
+        }
+    }
+
    private void debug(String line) {


Helper method is no longer needed now that Logger.debug exists.

pickypg · 2014-05-08T02:51:35Z

src/main/java/org/elasticsearch/plugins/PluginManager.java

@@ -69,6 +69,7 @@

    private String url;
    private OutputMode outputMode;
+    private Logger logger;


Should probably make these final where possible.

pickypg · 2014-05-08T02:55:17Z

@dadoonet LGTM. It makes sense that those comments will not be addressed here. I have some other really minor comments that should just help you drop a few lines of code and keep it tidier.

Use final attributes

s1monw · 2014-06-12T13:02:28Z

@dadoonet can you update the PR title it's very hard to tell what this is about?

@spinscale are you going to review this, can I remove the review tag?

s1monw · 2014-06-18T19:00:49Z

ping?

spinscale · 2014-06-19T06:10:04Z

@s1monw will do it today

spinscale · 2014-06-19T10:23:06Z

First: Please adapt the forbidden API config, so this compiles with maven again.

The change itself LGTM, but I am still in favor of a bigger refactorting of the PluginManager, which IMO needs some cleanup (can be done after this got in of course):

No need for several DownloadProgress impls, if you hand over the logger appropriately (also no need for the outputMode then, when the logger is created on startup
Unify the usage of the plugin class compared with a lot of File s being passed around.
Make the HttpDownloadHelper a bit more self-contained (quite a few arguments for downloading)

@dadoonet Is there a differente between /bin/plugin and /bin/plugin -v? Just setting -s seems to have an affect? Or is that some BWC option, we could drop in a major release?

clintongormley · 2014-07-11T09:52:44Z

@dadoonet ping?

dadoonet · 2014-07-23T15:49:26Z

@spinscale -v is a DEBUG mode. So it adds lot of information.
-s is the exact opposite. It does not output anything. In that mode you could only rely on exit code.

If you don't set -v or -s you will get back some information while the plugin is installing, like progress bar and success message.

BTW, I'll look at your comments, update the PR and ping you again when ready. Thanks for the review so far!

clintongormley · 2014-10-20T13:10:24Z

@dadoonet any progress on this?

clintongormley · 2015-06-04T19:15:48Z

@tlrx this PR is over a year old. Does your plugin work already cover a check for whether a plugin is installed or not? Can we close this PR?

clintongormley · 2015-06-05T12:24:35Z

Chatted to @tlrx - this functionality is included in #9998, so closing this one.

nickminutello and others added 2 commits April 29, 2014 12:46

Make Plugin inner class static

cd730c1

After reviewing elastic#5566. Closes elastic#5565. Closes elastic#5566.

dadoonet added enhancement labels Apr 29, 2014

dadoonet self-assigned this Apr 29, 2014

This was referenced Apr 29, 2014

Exposed a PluginManager.isPluginInstalled(String name) method for easier embedded use #5566

Closed

Make it easier to use PluginManager programmatically - specifically query if a plugin is already installed #5565

Closed

pickypg reviewed Apr 29, 2014
View reviewed changes

pickypg mentioned this pull request May 2, 2014

bin/plugin tests for missing plugin name when passing --url #6013

Closed

Update after @pickypg review

a7be4cf

pickypg reviewed May 8, 2014
View reviewed changes

pickypg mentioned this pull request May 8, 2014

bin/plugin should allow plugin files to be maintained for debugging purposes #6090

Closed

pickypg reviewed May 8, 2014
View reviewed changes

Remove unused log method as we have a helper

503fe07

Use final attributes

spinscale assigned spinscale and unassigned dadoonet May 12, 2014

dadoonet removed the v1.2.0 label May 12, 2014

dadoonet changed the title ~~Update code after reviewing PR #5566~~ Expose a PluginManager.isPluginInstalled(String name) method Jun 12, 2014

s1monw removed the review label Jun 18, 2014

clintongormley added the :Core/Infra/Plugins Plugin API and infrastructure label Nov 11, 2014

drewr force-pushed the master branch from dcc3da0 to 7c20a8a Compare February 20, 2015 16:48

clintongormley closed this Jun 5, 2015

clintongormley removed the v2.0.0-beta1 label Jun 7, 2015

dadoonet deleted the pr/5566-plugin-isinstalled branch August 18, 2015 11:27

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Expose a PluginManager.isPluginInstalled(String name) method #5977

Expose a PluginManager.isPluginInstalled(String name) method #5977

dadoonet commented Apr 29, 2014

pickypg Apr 29, 2014

dadoonet May 7, 2014

dadoonet commented May 7, 2014

pickypg May 8, 2014

pickypg May 8, 2014

pickypg commented May 8, 2014

s1monw commented Jun 12, 2014

s1monw commented Jun 18, 2014

spinscale commented Jun 19, 2014

spinscale commented Jun 19, 2014

clintongormley commented Jul 11, 2014

dadoonet commented Jul 23, 2014

clintongormley commented Oct 20, 2014

clintongormley commented Jun 4, 2015

clintongormley commented Jun 5, 2015

Expose a PluginManager.isPluginInstalled(String name) method #5977

Expose a PluginManager.isPluginInstalled(String name) method #5977

Conversation

dadoonet commented Apr 29, 2014

pickypg Apr 29, 2014

Choose a reason for hiding this comment

dadoonet May 7, 2014

Choose a reason for hiding this comment

dadoonet commented May 7, 2014

pickypg May 8, 2014

Choose a reason for hiding this comment

pickypg May 8, 2014

Choose a reason for hiding this comment

pickypg commented May 8, 2014

s1monw commented Jun 12, 2014

s1monw commented Jun 18, 2014

spinscale commented Jun 19, 2014

spinscale commented Jun 19, 2014

clintongormley commented Jul 11, 2014

dadoonet commented Jul 23, 2014

clintongormley commented Oct 20, 2014

clintongormley commented Jun 4, 2015

clintongormley commented Jun 5, 2015