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

Also skip JAVA_TOOL_OPTIONS on Windows #25968

Merged
merged 3 commits into from Jul 31, 2017

Conversation

@jasontedor
Copy link
Member

commented Jul 30, 2017

On non-Windows platforms, we ignore the environment variable JAVA_TOOL_OPTIONS (this is an environment variable that the JVM respects by default for picking up extra JVM options). The primary reason that we ignore this because of the Jayatana agent on Ubuntu; a secondary reason is that it produces an annoying "Picked up JAVA_TOOL_OPTIONS: ..." output message. When the elasticsearch-env batch script was introduced for Windows, ignoring this environment variable was deliberately not carried over as the primary reason does not apply on Windows. However, after additional thinking, it seems that we should simply be consistent to the extent possible here (and also avoid that annoying "Picked up JAVA_TOOL_OPTIONS: ..." on Windows too). This commit causes the Windows version of elasticsearch-env to also ignore JAVA_TOOL_OPTIONS.

On non-Windows platforms, we ignore the environment variable
JAVA_TOOL_OPTIONS (this is an environment variable that the JVM respects
by default for picking up extra JVM options). The primary reason that we
ignore this because of the Jayatana agent on Ubuntu; a secondary reason
is that it produces an annoying "Picked up JAVA_TOOL_OPTIONS: ..."
output message. When the elasticsearch-env batch script was introduced
for Windows, ignoring this environment variable was deliberately not
carried over as the primary reason does not apply on Windows. However,
after additional thinking, it seems that we should simply be consistent
to the extent possible here (and also avoid that annoying "Picked up
JAVA_TOOL_OPTIONS: ..." on Windows too). This commit causes the Windows
version of elasticsearch-env to also ignore JAVA_TOOL_OPTIONS.
* master:
  Version option should display if snapshot
  Ignore JVM options before checking Java version
  Remove dead code for checking exclusive options
  Fix typo in Elasticsearch help
  Add version command to issue template
@jasontedor jasontedor force-pushed the jasontedor:windows-java-tool-options branch to c3278d8 Jul 31, 2017
Copy link
Member

left a comment

I left two comments

@@ -28,6 +28,13 @@ if not exist %JAVA% (
exit /B 1
)

rem don't let JAVA_TOOL_OPTIONS slip in (e.g., agents in Ubuntu so this works

This comment has been minimized.

Copy link
@tlrx

tlrx Jul 31, 2017

Member

I don't think we should talk about Ubuntu here

This comment has been minimized.

Copy link
@jasontedor

jasontedor Jul 31, 2017

Author Member

Okay, I removed it.

@@ -28,6 +28,13 @@ if not exist %JAVA% (
exit /B 1
)

rem don't let JAVA_TOOL_OPTIONS slip in (e.g., agents in Ubuntu so this works
rem around https://bugs.launchpad.net/ubuntu/+source/jayatana/+bug/1441487)
if not "%JAVA_TOOL_OPTIONS%" == "" (

This comment has been minimized.

Copy link
@tlrx

tlrx Jul 31, 2017

Member

I'd be more comfortable if it was documented in the "Install Elasticsearch with zip on Windows". We have few notes there already.

This comment has been minimized.

Copy link
@jasontedor

jasontedor Jul 31, 2017

Author Member

This is a good suggestion, I pushed more general docs.

@jasontedor

This comment has been minimized.

Copy link
Member Author

commented Jul 31, 2017

@tlrx I addressed your feedback, would you mind taking another look?

@tlrx
tlrx approved these changes Jul 31, 2017
Copy link
Member

left a comment

LGTM, thanks @jasontedor

@jasontedor jasontedor merged commit 540413b into elastic:master Jul 31, 2017
1 of 2 checks passed
1 of 2 checks passed
elasticsearch-ci Build triggered. sha1 is merged.
Details
CLA Commit author is a member of Elasticsearch
Details
jasontedor added a commit that referenced this pull request Jul 31, 2017
On non-Windows platforms, we ignore the environment variable
JAVA_TOOL_OPTIONS (this is an environment variable that the JVM respects
by default for picking up extra JVM options). The primary reason that we
ignore this because of the Jayatana agent on Ubuntu; a secondary reason
is that it produces an annoying "Picked up JAVA_TOOL_OPTIONS: ..."
output message. When the elasticsearch-env batch script was introduced
for Windows, ignoring this environment variable was deliberately not
carried over as the primary reason does not apply on Windows. However,
after additional thinking, it seems that we should simply be consistent
to the extent possible here (and also avoid that annoying "Picked up
JAVA_TOOL_OPTIONS: ..." on Windows too). This commit causes the Windows
version of elasticsearch-env to also ignore JAVA_TOOL_OPTIONS.

Relates #25968
jasontedor added a commit that referenced this pull request Jul 31, 2017
On non-Windows platforms, we ignore the environment variable
JAVA_TOOL_OPTIONS (this is an environment variable that the JVM respects
by default for picking up extra JVM options). The primary reason that we
ignore this because of the Jayatana agent on Ubuntu; a secondary reason
is that it produces an annoying "Picked up JAVA_TOOL_OPTIONS: ..."
output message. When the elasticsearch-env batch script was introduced
for Windows, ignoring this environment variable was deliberately not
carried over as the primary reason does not apply on Windows. However,
after additional thinking, it seems that we should simply be consistent
to the extent possible here (and also avoid that annoying "Picked up
JAVA_TOOL_OPTIONS: ..." on Windows too). This commit causes the Windows
version of elasticsearch-env to also ignore JAVA_TOOL_OPTIONS.

Relates #25968
@jasontedor

This comment has been minimized.

Copy link
Member Author

commented Jul 31, 2017

Thank you @tlrx.

@jasontedor jasontedor deleted the jasontedor:windows-java-tool-options branch Jul 31, 2017
@colings86 colings86 added v6.0.0-beta1 and removed v6.0.0 labels Aug 3, 2017
@lcawl lcawl removed the v6.1.0 label Dec 12, 2017
@jimczi jimczi added v7.0.0-beta1 and removed v7.0.0 labels Feb 7, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.