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

Export the hostname as environment variable #9474

Merged
merged 1 commit into from Apr 17, 2015

Conversation

AndreKR
Copy link
Contributor

@AndreKR AndreKR commented Jan 29, 2015

and provide an example on how to use it in the config

Closes #8470

@clintongormley clintongormley added :Delivery/Packaging RPM and deb packaging, tar and zip archives, shell and batch scripts review labels Jan 29, 2015
@clintongormley
Copy link

Hi @AndreKR

Thanks for the PR. The only bit I'm not sure about is adding it to the config file, because using the hostname there can be problematic if you run more than one ES instance on the same node.

Perhaps it should be added to the reference docs instead, with the appropriate warning: http://www.elasticsearch.org/guide/en/elasticsearch/reference/current/setup-configuration.html#node-name

@AndreKR
Copy link
Contributor Author

AndreKR commented Jan 30, 2015

Done.

@@ -33,6 +33,9 @@ FOR /F "usebackq tokens=1* delims= " %%A IN (!params!) DO (
)
)

set HOSTNAME=
for /f "delims=" %%a in ('hostname') do @set HOSTNAME=%%a
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you please set instructions in uppercase to stick with the rest of the file?

Also, I'm not sure this works on older Windows like XP. Do you think you can get it work on it? That would be nice.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Argh, didn't see the uppercase, fixed.
Well, I wasn't successful running elasticsearch on Windows 2000 ("Unsupported major.minor version 51.0") but I can confirm that the solution for setting the hostname works:
2015-02-06_2117

@tlrx
Copy link
Member

tlrx commented Feb 6, 2015

@AndreKR Thanks for your contribution.

I made some comments. Can you also please squash all the commits together? Thanks.

@AndreKR
Copy link
Contributor Author

AndreKR commented Feb 6, 2015

Done.

@tlrx
Copy link
Member

tlrx commented Feb 27, 2015

I just tested on Windows XP and unfortunately it does not work and prevents Elasticsearch from starting.

I think it can be fixed like this:

SET ES_HOSTNAME=''
FOR /F "delims=" %%A IN ('hostname') DO (
    SET ES_HOSTNAME=%%A
)

What do you think? Does it work on your platforms?

@AndreKR AndreKR force-pushed the export-hostname-for-config branch 2 times, most recently from 2d91501 to 3a4d3d9 Compare March 11, 2015 19:08
@AndreKR
Copy link
Contributor Author

AndreKR commented Mar 11, 2015

Ok, I looked into the issue and indeed on Windows XP it didn't work, even though on Windows 2000 and Windows 7 it did. The actual problem were the missing quotes, with them it works on all three platforms. I pushed the change (and rebased the branch).

@clintongormley
Copy link

@tlrx please could you take a look

@tlrx
Copy link
Member

tlrx commented Apr 15, 2015

@AndreKR it takes some time but be sure that I'd like to merge this pull request :) Have you considered using %COMPUTERNAME% variable instead of hostname? Do you think it's better?

@AndreKR
Copy link
Contributor Author

AndreKR commented Apr 15, 2015

By using %COMPUTERNAME%, are you trying to solve a specific problem or just considering it as a cleaner approach? Honestly, I hadn't considered that, because I implemented the Linux variant first and then just looked into a parallel approach for Windows.
It would certainly have the advantage of making the batch file easier to read, so I'm going to implement that change.
Do you mean on Windows people should just use COMPUTERNAME or do we want the batch file to copy it over into HOSTNAME so that the elasticsearch config file and documentation is portable across platforms?

@tlrx
Copy link
Member

tlrx commented Apr 15, 2015

By using %COMPUTERNAME%, are you trying to solve a specific problem or just considering it as a cleaner approach?

Just a cleaner approach since %COMPUTERNAME% seems to be consistent accross windows versions.

Do you mean on Windows people should just use COMPUTERNAME or do we want the batch file to copy it over into HOSTNAME so that the elasticsearch config file and documentation is portable across platforms?

Copying it into HOSTNAME sounds good.

Thanks for making this change! I'll test it again and merge the pr.

@AndreKR
Copy link
Contributor Author

AndreKR commented Apr 17, 2015

Changed.

tlrx added a commit that referenced this pull request Apr 17, 2015
Export the hostname as environment variable
@tlrx tlrx merged commit a806314 into elastic:master Apr 17, 2015
@kevinkluge kevinkluge removed the review label Apr 17, 2015
tlrx added a commit that referenced this pull request Apr 17, 2015
@tlrx
Copy link
Member

tlrx commented Apr 17, 2015

@AndreKR Finally merged, thanks! I changed the wording a bit in b3d91b1

jaymode added a commit to jaymode/elasticsearch that referenced this pull request May 28, 2015
In elastic#9474, we exported the hostname in the bin/elasticsearch scripts so that it
could be used as a variable in the elasticsearch.yml file but did not do the same
for plugin manager. When using the hostname variable in elasticsearch.yml and
trying to use the plugin manager, initialization will fail because the property could
not be resolved. This change will allow the hostname to be resolved in the same
manner as the service scripts.

Closes elastic#10902
jaymode added a commit to jaymode/elasticsearch that referenced this pull request May 28, 2015
In elastic#9474, we exported the hostname in the bin/elasticsearch scripts so that it
could be used as a variable in the elasticsearch.yml file but did not do the same
for plugin manager. When using the hostname variable in elasticsearch.yml and
trying to use the plugin manager, initialization will fail because the property could
not be resolved. This change will allow the hostname to be resolved in the same
manner as the service scripts.

Closes elastic#10902
jaymode added a commit that referenced this pull request May 28, 2015
In #9474, we exported the hostname in the bin/elasticsearch scripts so that it
could be used as a variable in the elasticsearch.yml file but did not do the same
for plugin manager. When using the hostname variable in elasticsearch.yml and
trying to use the plugin manager, initialization will fail because the property could
not be resolved. This change will allow the hostname to be resolved in the same
manner as the service scripts.

Closes #10902
@agirbal
Copy link

agirbal commented May 28, 2015

awesome thanks for fixing!

@mark-vieira mark-vieira added the Team:Delivery Meta label for Delivery team label Nov 11, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
:Delivery/Packaging RPM and deb packaging, tar and zip archives, shell and batch scripts >enhancement Team:Delivery Meta label for Delivery team v1.6.0 v2.0.0-beta1
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Set default node name to hostname
6 participants