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.

Sign up for GitHub

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

Jump to bottom

[FEATURE] Add support for OPENSEARCH_JAVA_HOME #133

Merged
merged 3 commits into from
Feb 8, 2022
Merged

[FEATURE] Add support for OPENSEARCH_JAVA_HOME #133

Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
469f5a7
[FEATURE] Add support for OPENSEARCH_JAVA_HOME
reta Feb 7, 2022
7df1732
Simplified condition checks
reta Feb 7, 2022
de55ccc
Changing JAVA_HOME -> condition
reta Feb 7, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
17 changes: 11 additions & 6 deletions pa_bin/performance-analyzer-agent
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,15 +14,20 @@ else
OPENSEARCH_HOME=$1
fi

if [ -z "$2" ]; then
if [ -z "$JAVA_HOME" ]; then
echo "JAVA_HOME needs to be set or passed in as the second parameter."
exit 1
fi
else
if [ ! -z "$2" ]; then
JAVA_HOME=$2
elif [ ! -z "$OPENSEARCH_JAVA_HOME" ]; then
# Use OPENSEARCH_JAVA_HOME if present
JAVA_HOME=$OPENSEARCH_JAVA_HOME
elif [ -z "$JAVA_HOME" ]; then
# Nor OPENSEARCH_JAVA_HOME nor JAVA_HOME is present, failing
echo "OPENSEARCH_JAVA_HOME / JAVA_HOME needs to be set or passed in as the second parameter."
exit 1
fi
Copy link
Member

Choose a reason for hiding this comment

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

These nested ifs are getting very confusing. Maybe swap them?

  1. If $2 use that
  2. Else if OPENSEARCH_JAVA_HOME use that
  3. Else if JAVA_HOME use that
  4. Else fail?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

👍 done!


echo "Using JAVA_HOME: $JAVA_HOME"
export JAVA_HOME=$JAVA_HOME
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Interestingly, the JAVA_HOME is ignored right now and it works only because bin/performance-analyzer-rca picks java command line from the $PATH. It is easy to check and reproduce running fe:

JAVA_HOME=/this/path/does/not/exists OPENSEARCH_HOME=. ./pa_bin/performance-analyzer-agent

or

./pa_bin/performance-analyzer-agent . /this/path/does/not/exists

Adding export JAVA_HOME=$JAVA_HOME forces bin/performance-analyzer-rca to use the right JVM location from the environment: either OPENSEARCH_JAVA_HOME or JAVA_HOME

Copy link
Member

Choose a reason for hiding this comment

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

🤦


# Instead of the supervisor executing performance-analyzer-agent from the plugin location,
# we should move this to the reader. The entry-point script should be executing
# performance-analyzer-agent from the reader location.
Expand Down