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

Enable es_include at init #15173

Merged
merged 2 commits into from Mar 10, 2016

Conversation

Projects
None yet
4 participants
@rhoml
Copy link
Contributor

rhoml commented Dec 2, 2015

es_include is not exported at init so it is not loaded from the right /etc/default/elasticsearch when users have custom default files causing gc.log file not to load.

@rhoml

This comment has been minimized.

Copy link
Contributor Author

rhoml commented Dec 2, 2015

Mmm I just signed the agreement

@nik9000

This comment has been minimized.

Copy link
Contributor

nik9000 commented Feb 10, 2016

Hi @rhoml! I'm sorry this sat for so long.

It looks like setting ES_INCLUDE opts you out from sourcing /usr/share/elasticsearch/elasticsearch.in.sh or similar files. I suspect setting it is more likely to break things than not. My understanding is that the right place to put parameters like ES_JVM_ARGS is /etc/defaults/elasticsearch in Debian/Ubunutu. Would that work for you?

@nik9000 nik9000 self-assigned this Feb 10, 2016

@rjernst

This comment has been minimized.

Copy link
Member

rjernst commented Feb 10, 2016

parameters like ES_JVM_ARGS

I think using JAVA_OPTS is better, since it is the standard environment variable for this purpose? At some point we should clean up the env vars we accept, ES_JVM_ARGS shouldn't be necessary.

@rhoml

This comment has been minimized.

Copy link
Contributor Author

rhoml commented Feb 11, 2016

@nik9000 the problem is that by default /usr/share/elasticsearch/elasticsearch.in.sh gets loaded but if you change the name of the file to /usr/share/elasticsearch/logstash.elasticsearch.in.sh for example it never gets loaded. Causing problem in situations like when you have multiple Elasticsearch instances running on the same box.

@nik9000

This comment has been minimized.

Copy link
Contributor

nik9000 commented Feb 11, 2016

Causing problem in situations like when you have multiple Elasticsearch instances running on the same box.

Now I understand. Could you make the same change in the init script for the rpm and maybe the systemd files? We have these Vagrant tests where you can test it but I imagine they are a bit stale lately because we haven't been diligent about getting jenkins running them....

@rhoml

This comment has been minimized.

Copy link
Contributor Author

rhoml commented Feb 11, 2016

Yes, I will do it right away

@rhoml

This comment has been minimized.

Copy link
Contributor Author

rhoml commented Feb 11, 2016

I've already added it to DEB, and RPM.

@clintongormley clintongormley added the >bug label Feb 13, 2016

@clintongormley

This comment has been minimized.

Copy link
Member

clintongormley commented Mar 10, 2016

@nik9000 could you take a look when you have a moment, please?

@nik9000

This comment has been minimized.

Copy link
Contributor

nik9000 commented Mar 10, 2016

OK - the change looks good to me. I'll see about merging to 2.x where the bats tests are still happy and validate there and then merge into master.

I'd love to have a bats tests are still happy but that is a large ask.

@nik9000

This comment has been minimized.

Copy link
Contributor

nik9000 commented Mar 10, 2016

I'd love to have a bats tests are still happy but that is a large ask.

Er, what I meant to say is:
I'd love to have a bats test for this but that is a large ask.

nik9000 added a commit that referenced this pull request Mar 10, 2016

@nik9000 nik9000 merged commit 48191a4 into elastic:master Mar 10, 2016

1 check passed

CLA Commit author has signed the CLA
Details
@nik9000

This comment has been minimized.

Copy link
Contributor

nik9000 commented Mar 10, 2016

OK! Merged to master and cherry picked to 2.x with cd7a5e5 and 2915157. Thanks @rhoml ! Sorry this took so long.

@rhoml

This comment has been minimized.

Copy link
Contributor Author

rhoml commented Apr 5, 2016

No worries @nik9000 thanks

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.