Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove unused java opts/es java opts from plugin manager call #12801

Conversation

spinscale
Copy link
Contributor

When calling the plugin manager on java 7 with additional JAVA_OPTS
that change heap configuration compared to what is set at the plugin
manager shell script. This resulted in errors.

This commit removes the JAVA_OPTS and ES_JAVA_OPTS from the plugin
manager call to prevent those settings.

Closes #12479

@spinscale spinscale added review :Core/Infra/Plugins Plugin API and infrastructure v2.0.0 labels Aug 11, 2015
@spinscale
Copy link
Contributor Author

I tested the deb/rpm manually, and ran the vagrant tests as well as regular mvn verify. However there might still be cases, that we are missing here, like the puppet module or something.

@electrical can you run your tests against this branch to see if this breaks the plugin manager somewhere in your test suite?

Also, whoever is reviewing this, might want to test this with some more custom esoteric plugin paths...

@electrical
Copy link

@spinscale Im afraid i can't directly test of a branch yet or custom build snapshot packages. I still need to work on that. That said I don't directly see any issues with the proposed change.

@clintongormley clintongormley changed the title Plugins: Remove unused java opts/es java opts from plugin manager call Remove unused java opts/es java opts from plugin manager call Aug 11, 2015
@spinscale
Copy link
Contributor Author

tested further, also bin/plugin install lmenezes/elasticsearch-kopf -Des.path.plugins=/tmp/foo/ works as expected.

Also works as expected: Changing path.plugins in config/elasticsearch.yml and calling bin/plugin install lmenezes/elasticsearch-kopf

Also works as expected: Using CONF_DIR environment variables: CONF_DIR=/tmp bin/plugin install lmenezes/elasticsearch-kopf

@spinscale
Copy link
Contributor Author

ping @nik9000 or @tlrx for review

@tlrx
Copy link
Member

tlrx commented Aug 17, 2015

Shouldn't we udpate plugin.bat too? Otherwise LGTM and it also fixes issues like #11623

@spinscale spinscale force-pushed the 1508-packaging-remove-opts-from-pluginmanager-call-issue-12479 branch from 00186cb to fedadac Compare August 31, 2015 06:12
@spinscale
Copy link
Contributor Author

changed the plugin.batfile as well, can you test that as well, just want to make sure the -D syntax is still working

@s1monw
Copy link
Contributor

s1monw commented Aug 31, 2015

while we are at it can we remove the Xmx and Xms settings and force a -client jvm? it would just make way more sense to me? We can totally do that in a different PR

@spinscale
Copy link
Contributor Author

+1 for the -client parameter, will fix. I printed out Runtime.getMemory() stats (free, total, max) at the beginning and end of a plugin manager run. When we omit Xms/Xms we get far higher numbers here. Are we still good that change then?

With current default (or with -client set, numbers are the same)

# bin/plugin list
total/free/max: 15.5mb/12.2mb/57mb
total/free/max: 19.5mb/10.5mb/57mb
# bin/plugin install file:///Users/alr/devel/elasticsearch/plugins/analysis-icu/target/releases/analysis-icu-2.1.0-SNAPSHOT.zip
total/free/max: 15.5mb/12.2mb/57mb
total/free/max: 27.5mb/5.7mb/57mb
# bin/plugin list
total/free/max: 15.5mb/12.2mb/57mb
total/free/max: 19.5mb/10.4mb/57mb
# bin/plugin remove analysis-icu
total/free/max: 15.5mb/12.2mb/57mb
total/free/max: 19.5mb/10.4mb/57mb

With -client and no -Xmx and -Xms
# bin/plugin list
total/free/max: 245.5mb/239mb/3.5gb
total/free/max: 245.5mb/227.5mb/3.5gb
# bin/plugin install file:///Users/alr/devel/elasticsearch/plugins/analysis-icu/target/releases/analysis-icu-2.1.0-SNAPSHOT.zip
total/free/max: 245.5mb/239mb/3.5gb
total/free/max: 245.5mb/200.6mb/3.5gb
# bin/plugin remove analysis-icu
total/free/max: 245.5mb/239mb/3.5gb
total/free/max: 245.5mb/227.5mb/3.5gb

@spinscale spinscale force-pushed the 1508-packaging-remove-opts-from-pluginmanager-call-issue-12479 branch from fedadac to 556cf38 Compare September 7, 2015 14:32
@spinscale
Copy link
Contributor Author

as @clintongormley has had problems installing a bigger plugin in beta1, I now switched to use the client VM and removed the heap size configuration... @s1monw can you take another look?

Ran mvn clean verify with all the integration tests locally and it passed

@nik9000
Copy link
Member

nik9000 commented Sep 15, 2015

LGTM. I can run the vagrant tests with this applied as super double extra check but I think its fine.

@nik9000
Copy link
Member

nik9000 commented Sep 15, 2015

Ran vagrant tests with this. Everything is good.

... and run as client VM.

Reasoning: When calling the plugin manager on java 7 with additional JAVA_OPTS
that change heap configuration compared to what is set at the plugin
manager shell script. This resulted in errors.

This commit removes the JAVA_OPTS and ES_JAVA_OPTS from the plugin
manager call to prevent those settings.

Closes elastic#12479
@spinscale spinscale force-pushed the 1508-packaging-remove-opts-from-pluginmanager-call-issue-12479 branch from 556cf38 to 1e209e3 Compare September 15, 2015 15:04
@spinscale spinscale merged commit 1e209e3 into elastic:master Sep 15, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
>bug :Core/Infra/Plugins Plugin API and infrastructure v2.0.0-rc1
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants