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

"[FLUME-3067][Shell]Defalut JAVA_OPTS "-Xmx20m" may be conflicted with cuctom defines." #112

Open
wants to merge 4 commits into
base: trunk
Choose a base branch
from

Conversation

chenjianwei3
Copy link

When write a Flume bootstrap shell, set the environment variable FLUME_JAVA_OPTS with the defined values as the min(-Xms)
and max(-Xmx) heap size for the JVM, then call "bin/flime-ng" to start Flume.
Succeed to start, but the defalut "-Xmx20m" is still in process JVM options.

Copy link
Contributor

@adenes adenes left a comment

Choose a reason for hiding this comment

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

Thank you for the patch. I added one comment otherwise looks good to me, 👍

bin/flume-ng Outdated
@@ -211,6 +211,10 @@ run_flume() {
if [ ${CLEAN_FLAG} -ne 0 ]; then
set -x
fi

if [[ $FLUME_JAVA_OPTS =~ "Xmx" ]]; then
Copy link
Contributor

Choose a reason for hiding this comment

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

I think it might be good to rename the $JAVA_OPTS to $JAVA_HEAP_MAX or add this as a new variable (as in Hadoop: https://github.com/apache/hadoop-common/blob/trunk/hadoop-common-project/hadoop-common/src/main/bin/hadoop-functions.sh#L544).
The reason is that if in the future new default parameters will be added to the $JAVA_OPTS passing in -Xmx will clear them too, resulting in unexpected behaviour (unless we do this change when we add the new parameters).

bin/flume-ng Outdated
@@ -222,7 +226,8 @@ run_flume() {
# set default params
FLUME_CLASSPATH=""
FLUME_JAVA_LIBRARY_PATH=""
JAVA_OPTS="-Xmx20m"
JAVA_OPTS=""
JAVA_HEAP_MAX ="-Xmx20m"
Copy link
Contributor

Choose a reason for hiding this comment

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

Please remove the extra space before the =, it results in runtime error (bin/flume-ng: line 230: JAVA_HEAP_MAX: command not found). Thank you.

Copy link
Author

Choose a reason for hiding this comment

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

so careless, thank you very much.

bin/flume-ng Outdated
@@ -211,7 +211,11 @@ run_flume() {
if [ ${CLEAN_FLAG} -ne 0 ]; then
set -x
fi
$EXEC $JAVA_HOME/bin/java $JAVA_OPTS $FLUME_JAVA_OPTS "${arr_java_props[@]}" -cp "$FLUME_CLASSPATH" \

if [[ $FLUME_JAVA_OPTS =~ "Xmx" ]] || [[ $JAVA_OPTS =~ "Xmx" ]]; then
Copy link
Contributor

@adenes adenes Mar 7, 2017

Choose a reason for hiding this comment

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

We could make it more bulletproof by using $FLUME_JAVA_OPTS =~ -Xmx[0-9]+ check to make sure that there is a dash before and at least one number after the Xmx.
Not sure that this is necessary though, what do you think, @chenjianwei3 ?

Copy link
Contributor

@adenes adenes left a comment

Choose a reason for hiding this comment

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

Thank you for the patch and being responsive during the review. LGTM, 👍

@asfgit
Copy link

asfgit commented Aug 17, 2018

Can one of the admins verify this patch?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants