Skip to content

SOLR-15477: Logging Performance#389

Draft
markrmiller wants to merge 1 commit intoapache:mainfrom
markrmiller:SOLR-15477
Draft

SOLR-15477: Logging Performance#389
markrmiller wants to merge 1 commit intoapache:mainfrom
markrmiller:SOLR-15477

Conversation

@markrmiller
Copy link
Member

@markrmiller markrmiller commented Nov 3, 2021

LOG4J_CONFIG=()
if [ -n "$LOG4J_PROPS" ]; then
LOG4J_CONFIG+=("-Dlog4j.configurationFile=$LOG4J_PROPS")
LOG4J_CONFIG+=("-Dlog4j.configurationFile=$LOG4J_PROPS -Dlog4j2.is.webapp=false -Dlog4j2.garbagefreeThreadContextMap=true -Dlog4j2.enableDirectEncoders=true -Dlog4j2.enable.threadlocals=true")
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you add a comment with a link to some docs or anything else explaining where these properties come from and how we found them? I'm trying to search for them and coming up short. Maybe I'll dive into log4j code base next, although I'd like to avoid that if I can.

Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Contributor

@malliaridis malliaridis left a comment

Choose a reason for hiding this comment

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

With the recent changes in the CLI scripts, I believe we could update this one and get it merged?

For my understanding, is the configuration -Dlog4j2.is.webapp=false ok also in case of our webapp? Or do we have to set it to true for the webapp explicitly?

@@ -1973,7 +1973,7 @@ fi

LOG4J_CONFIG=()
Copy link
Contributor

Choose a reason for hiding this comment

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

If the log4j options should always be loaded as default, we should consider adding them here instead, since the LOG4J_PROPS is only populated in examples where a custom log4j2.xml configuration is mandatory.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants