Introduce LS_JAVA_HOME environment variable by andsel · Pull Request #13204 · elastic/logstash

andsel · 2021-09-02T15:32:54Z

Release notes

Logstash's launch script support LS_JAVA_HOME as an override of environment variable over JAVA_HOME.
It's the path to an installed JDK to be used to launch Logstash's service and support tools.
JAVA_HOME is going to be removed, however if LS_JAVA_HOME is not specified then Logstash still looks for JAVA_HOME.

What does this PR do?

This PR modifies the launch scripts to take care of the LS_JAVA_HOME giving precedence over the JAVA_HOME, which is still used it the first is not found.

Why is it important/What is the impact to the user?

The existing JAVA_HOME is an environment variable globally used by all JVM applications present on a host.
With the introduction of LS_JAVA_HOME the user is able to customize the JDK used by Logstash without changing the global JAVA_HOME. It introduces a layer of separation, permitting a more fine grained control of the JDK customization for Logstash.

Checklist

My code follows the style guidelines of this project
~~I have commented my code, particularly in hard-to-understand areas~~
have made corresponding changes to the documentation
~~I have made corresponding change to the default configuration files (and/or docker env variables)~~
~~I have added tests that prove my fix is effective or that my feature works~~

Author's Checklist

verify it works setting LS_JAVA_HOME on Windows and Linux

How to test this PR locally

checkout this branch on your machine
on bash set the LS_JAVA_HOME export LS_JAVA_HOME=/path/to/jdk and run `bin/logstash -e "input{stdin{}} output {stdout{codec => rubydebug}}"
on Windows cmd set LS_JAVA_HOME="C:\path\to\JDK" and run `bin\logstash -e "input{stdin{}} output {stdout{codec => rubydebug}}"

In both cases it prints the the JDK used and must correspond to what was selected, verify also with rest API curl -XGET 'localhost:9600/_node/jvm?pretty'

Related issues

Use cases

Screenshots

Logs

roaksoax

The actual changes look good to me. I'll approve on that basis, however, could you please make sure all the comments about messages are addressed before landing?

roaksoax · 2021-09-03T14:18:15Z

+      if [ -x "$LS_JAVA_HOME/bin/java" ]; then
+        JAVACMD="$LS_JAVA_HOME/bin/java"
+        if [ -d "${LOGSTASH_HOME}/${BUNDLED_JDK_PART}" -a -x "${LOGSTASH_HOME}/${BUNDLED_JDK_PART}/bin/java" ]; then
+          echo "WARNING, using LS_JAVA_HOME while Logstash distribution comes with a bundled JDK"


I believe the format for warnings is "WARNING: ". You are using a ',' instead. So should be:

WARNING: Using LS_JAVA_HOME env variable while Logstash distribution comes with a bundled JDK ()"

I made a couple of minor adjustments. Would be nice to specify which version of the JDK is bundled (the full version). Lets use end of sentence punctuation here as well.

One way to print the full version of the bundled JDK could be to execute the "${LOGSTASH_HOME}/${BUNDLED_JDK_PART}/bin/java -version" . But this produces 3 lines of output after the "WARNING:" line, it's not inlined.

roaksoax · 2021-09-03T14:19:46Z


  if [ ! -x "$JAVACMD" ]; then
-    echo "could not find java; set JAVA_HOME or ensure java is in PATH"
+    echo "could not find java; set LS_JAVA_HOME or ensure java is in PATH"


Should the first letter be upper cased? (It is a new sentence(. Also, I believe we should use proper punctuation, so should likely need a '.' at the end of all sentences.

roaksoax · 2021-09-03T14:19:57Z

+          echo "WARNING, using LS_JAVA_HOME while Logstash distribution comes with a bundled JDK"
+        fi
+      else
+        echo "Invalid LS_JAVA_HOME, doesn't contain bin/java executable"


missing end of sentence punctuation.

roaksoax · 2021-09-03T14:20:11Z

+  set JAVA="%LS_JAVA_HOME%\bin\java.exe"
+  echo Using LS_JAVA_HOME defined java: %LS_JAVA_HOME%
+  if exist "%LS_HOME%\jdk" (
+    echo WARNING, using LS_JAVA_HOME while Logstash distribution comes with a bundled JDK


Incorrect format, plus missing punctuation.

roaksoax · 2021-09-03T14:20:55Z

    JAVACMD_TEST=`command -v java`
-    if [ -n "$JAVA_HOME" ]; then
+    if [ -n "$LS_JAVA_HOME" ]; then
+      echo "Using LS_JAVA_HOME defined java: ${LS_JAVA_HOME}"


same here, punctuation?

karenzone

Thanks for these doc updates, @andsel I left some comments inline for your consideration.

karenzone · 2021-09-03T15:00:47Z

+If {ls} doesn't find `LS_JAVA_HOME` it tries to fall back on `JAVA_HOME` but this behaviour
+could be subject to be removed.


Suggested change

If {ls} doesn't find `LS_JAVA_HOME` it tries to fall back on `JAVA_HOME` but this behaviour

could be subject to be removed.

If {ls} doesn't find `LS_JAVA_HOME`, it tries to fall back to `JAVA_HOME`. Note that this behavior is experimental and could be subject to removal in later releases.

karenzone · 2021-09-03T15:01:04Z

-On some Linux systems, you may need to have the `JAVA_HOME` environment
+If {ls} doesn't find `LS_JAVA_HOME` it tries to fall back on `JAVA_HOME` but this behaviour
+could be subject to be removed.
+On some Linux systems, you may need to have the `LS_JAVA_HOME` environment


Suggested change

On some Linux systems, you may need to have the `LS_JAVA_HOME` environment

On some Linux systems, you may need to have the `LS_JAVA_HOME` environment

Suggest return to break up text block for easier parsing.

andsel · 2021-09-03T15:06:12Z

@karenzone may I ask you a quick eye on the docs part of this PR, located in file docs/static/jvm.asciidoc.
This PR changes the Logstash's launch scripts to that a user can specify the customization of the JDK he wants to use also with LS_JAVA_HOME. This approach has the advantage to don't force the user to change the globally used JAVA_HOME and interfere with other JVM application he could run on the host.

Logstash leverage JAVA_HOME environment variable to give the user the ability to explicitly select a JDK he woudl use. JAVA_HOME is a variable shared by all applccation that use a JDK, so to avoid conflicts with other services and let the user to select a specific JDK only for Logstash execution, this commit introduce the LS_JAVA_HOME that takes precedence over the standard variable.

This commit modifies the launch scripts to take care of the LS_JAVA_HOME giving precedence over the JAVA_HOME, which is still used it the first is not found.

Backport of #13204 #13207 to 7.x branch, introduces the LS_JAVA_HOME as preferred environment variable over JAVA_HOME which is deprecated.

andsel mentioned this pull request Sep 2, 2021

[Meta ] Use LS_JAVA_HOME in favor of JAVA_HOME #12725

Closed

4 tasks

andsel added the java 16 support label Sep 2, 2021

roaksoax added the status:work-in-progress label Sep 2, 2021

andsel added the enhancement label Sep 3, 2021

andsel marked this pull request as ready for review September 3, 2021 12:39

andsel requested review from karenzone and removed request for karenzone September 3, 2021 12:40

andsel added status:needs-review and removed status:work-in-progress labels Sep 3, 2021

roaksoax approved these changes Sep 3, 2021

View reviewed changes

roaksoax added status:approved and removed status:needs-review labels Sep 3, 2021

karenzone reviewed Sep 3, 2021

View reviewed changes

andsel mentioned this pull request Sep 3, 2021

Print the JDK bundled version on reporting bash script launch errors #13205

Closed

andsel added 5 commits September 6, 2021 09:48

Updated docs for new LS_JAVA_HOME

6da5261

Fixed punctuation in scripts' console messages

f8107f0

[Doc] Better shaped the doc changes

71649d4

[Docs] better expressed that JAVA_HOME would be removed

d9beb59

andsel force-pushed the feature/add_ls_java_home branch from 128bd61 to d9beb59 Compare September 6, 2021 07:48

andsel merged commit 979ea21 into elastic:master Sep 6, 2021

andsel mentioned this pull request Sep 8, 2021

[backport 7.x] Use LS_JAVA_HOME in favor of JAVA_HOME #12725 #13213

Merged

andsel added a commit that referenced this pull request Sep 8, 2021

[backport 7.x] Use LS_JAVA_HOME in favor of JAVA_HOME #12725 (#13213)

82993fa

Backport of #13204 #13207 to 7.x branch, introduces the LS_JAVA_HOME as preferred environment variable over JAVA_HOME which is deprecated.

andsel added the v7.16.0 label Sep 8, 2021

andsel mentioned this pull request Sep 8, 2021

Removed JAVA_HOME using only LS_JAVA_HOME #13214

Merged

4 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Introduce LS_JAVA_HOME environment variable#13204

Introduce LS_JAVA_HOME environment variable#13204
andsel merged 5 commits into
elastic:masterfrom
andsel:feature/add_ls_java_home

andsel commented Sep 2, 2021 •

edited

Loading

Uh oh!

roaksoax left a comment

Uh oh!

roaksoax Sep 3, 2021

Uh oh!

andsel Sep 3, 2021

Uh oh!

roaksoax Sep 3, 2021

Uh oh!

roaksoax Sep 3, 2021

Uh oh!

roaksoax Sep 3, 2021

Uh oh!

roaksoax Sep 3, 2021

Uh oh!

karenzone left a comment

Uh oh!

karenzone Sep 3, 2021

Uh oh!

karenzone Sep 3, 2021

Uh oh!

karenzone Sep 3, 2021

Uh oh!

andsel commented Sep 3, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

		If {ls} doesn't find `LS_JAVA_HOME` it tries to fall back on `JAVA_HOME` but this behaviour
		could be subject to be removed.

	If {ls} doesn't find `LS_JAVA_HOME` it tries to fall back on `JAVA_HOME` but this behaviour
	could be subject to be removed.
	If {ls} doesn't find `LS_JAVA_HOME`, it tries to fall back to `JAVA_HOME`. Note that this behavior is experimental and could be subject to removal in later releases.

	On some Linux systems, you may need to have the `LS_JAVA_HOME` environment

	On some Linux systems, you may need to have the `LS_JAVA_HOME` environment

Conversation

andsel commented Sep 2, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Release notes

What does this PR do?

Why is it important/What is the impact to the user?

Checklist

Author's Checklist

How to test this PR locally

Related issues

Use cases

Screenshots

Logs

Uh oh!

roaksoax left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

karenzone left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

andsel commented Sep 3, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

andsel commented Sep 2, 2021 •

edited

Loading