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

log4j config changes (ISPN-1749) #859

Closed
wants to merge 2 commits into from

Conversation

dvanbalen
Copy link
Contributor

...g system property in LOG4J_CONFIG environment variable

https://issues.jboss.org/browse/ISPN-1749

…ting system property in LOG4J_CONFIG environment variable
else
LOG4J_CONFIG=${ISPN_HOME}/etc/log4j.xml
fi
START_ARGS=( "${START_ARGS[@]}" "-Dlog4j.configuration=file:///$LOG4J_CONFIG" )
Copy link
Member

Choose a reason for hiding this comment

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

Isn't it better that the $LOG4J_CONFIG variable contain the full URL to the configuration, including the "file:///" part? This way someone can specify something like a "http://" URL to their log config as well.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The way this pull request is set up, user specified values for $LOG4J_CONFIG would contain a URI, since the user is required to specify the entire system property line (I.e. -Dlog4j.configuration=file:///foo/bar/baz/log4j.xml). Note that the changes to startServer.sh cause the entire contents of $LOG4J_CONFIG to be passed as a system property. Another option would be to move that last line in the functions.sh diff (the one starting with START_ARGS=) out of the "if" statement so that the user doesn't have to specify the system property, and can instead simply populate $LOG4J_CONFIG with a URI for their log4j configuration file without worrying about what system property it belongs in. There are probably advantages and disadvantages to both approaches, although the second would provide a more consistent use of the $LOG4J_CONFIG environment variable across the start scripts, so maybe I should revise this patch to take that approach.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Moving the "START_ARGS=" line out of the "if" would require moving the "file:///" part inside the "if" so that the user could still provide a non file-based URI (e.g. http://, as you mentioned).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

My reasoning for using $LOG4J_CONFIG in startServer.sh the way I did is that it is the first place the user will likely look, and having it added to the jvm args like I did should make it immediately obvious how to specify a different log4j configuration file.

Copy link
Member

Choose a reason for hiding this comment

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

Re: moving the START_ARGS bit to outside the if (and a file:/// inside it) is precisely what I was suggesting. :)

Re: the LOG4J_CONFIG being in startServer.sh is fine.

@maniksurtani
Copy link
Member

Thanks!

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