-
Notifications
You must be signed in to change notification settings - Fork 67
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
[FEATURE] Add support for OPENSEARCH_JAVA_HOME #133
Conversation
Signed-off-by: Andriy Redko <andriy.redko@aiven.io>
fi | ||
else | ||
JAVA_HOME=$2 | ||
fi | ||
|
||
echo "Using JAVA_HOME: $JAVA_HOME" | ||
export JAVA_HOME=$JAVA_HOME |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🤦
exit 1 | ||
fi | ||
else | ||
JAVA_HOME=$OPENSEARCH_JAVA_HOME | ||
fi | ||
else | ||
JAVA_HOME=$2 | ||
fi |
There was a problem hiding this comment.
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?
- If $2 use that
- Else if OPENSEARCH_JAVA_HOME use that
- Else if JAVA_HOME use that
- Else fail?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 done!
Signed-off-by: Andriy Redko <andriy.redko@aiven.io>
pa_bin/performance-analyzer-agent
Outdated
elif [ ! -z "$OPENSEARCH_JAVA_HOME" ]; then | ||
# Use OPENSEARCH_JAVA_HOME if present | ||
JAVA_HOME=$OPENSEARCH_JAVA_HOME | ||
elif [ -z "JAVA_HOME" ]; then |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
$JAVA_HOME?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep, fixed, thx!
Signed-off-by: Andriy Redko <andriy.redko@aiven.io>
Is the test failure in https://github.com/opensearch-project/performance-analyzer/runs/5100788184?check_suite_focus=true a fluke? Open an issue? |
Seems like, yeah, opened #134 |
@sruti1312 take a look pls? |
Signed-off-by: Andriy Redko andriy.redko@aiven.io
Is your feature request related to a problem? Please provide an existing Issue # , or describe.
Closes #132
Describe the solution you are proposing
Add support for OPENSEARCH_JAVA_HOME (see please opensearch-project/OpenSearch#1872), as an alternative to JAVA_HOME (both should work just fine).
Describe alternatives you've considered
N/A
Additional context
See please opensearch-project/OpenSearch#1872
Check List
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.