You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@logging.apache.org by GitBox <gi...@apache.org> on 2020/09/21 09:41:17 UTC
[GitHub] [logging-log4j2] wuqian0808 opened a new pull request #425: Update KafkaManager.java
wuqian0808 opened a new pull request #425:
URL: https://github.com/apache/logging-log4j2/pull/425
Avoid kafka thread leak during loggercontext reconfigure multiple times.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [logging-log4j2] gengyuanzhe commented on pull request #425: Update KafkaManager.java
Posted by GitBox <gi...@apache.org>.
gengyuanzhe commented on pull request #425:
URL: https://github.com/apache/logging-log4j2/pull/425#issuecomment-696154623
In my opition, `org.apache.logging.log4j.kafka.appender.KafkaManager#releaseSub` should close the kafka producer when `reconfigure()`. The `close()` should not be invoked in`startup ()`, it is confused.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [logging-log4j2] gengyuanzhe commented on pull request #425: Update KafkaManager.java
Posted by GitBox <gi...@apache.org>.
gengyuanzhe commented on pull request #425:
URL: https://github.com/apache/logging-log4j2/pull/425#issuecomment-696154623
In my opition, `org.apache.logging.log4j.kafka.appender.KafkaManager#releaseSub` should close the kafka producer when `reconfigure()`. The `close()` should not be invoked in`startup ()`, it is confused.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [logging-log4j2] wuqian0808 commented on pull request #425: Update KafkaManager.java
Posted by GitBox <gi...@apache.org>.
wuqian0808 commented on pull request #425:
URL: https://github.com/apache/logging-log4j2/pull/425#issuecomment-698675698
> I tried your case, and you are right, the KafkaAppender do have the problem of thead leak. But **close when startup** is not the correct way to solve this problem. Try to find why the **count in abstractmanager** increase each time when reconfigure(), fix it and make sure the kafkaManager close when prev.stop(). You'd better open an [issue](https://issues.apache.org/jira/browse/LOG4J2) to track this problem. And if you can provide unit test that failed without your PR, and success with your PR, it will help a lot to make this PR accepted ASAP.
Sure. I will find a better way to fix this issue
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [logging-log4j2] wuqian0808 commented on pull request #425: Update KafkaManager.java
Posted by GitBox <gi...@apache.org>.
wuqian0808 commented on pull request #425:
URL: https://github.com/apache/logging-log4j2/pull/425#issuecomment-696016546
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [logging-log4j2] gengyuanzhe commented on pull request #425: Update KafkaManager.java
Posted by GitBox <gi...@apache.org>.
gengyuanzhe commented on pull request #425:
URL: https://github.com/apache/logging-log4j2/pull/425#issuecomment-698385158
I tried your case, and you are right, the KafkaAppender do have the problem of thead leak. But **close when startup** is not the correct way to solve this problem. Try to find why the **count in abstractmanager** increase each time when reconfigure(), fix it and make sure the kafkaManager close when prev.stop(). You'd better open an [issue](https://issues.apache.org/jira/browse/LOG4J2) to track this problem. And if you can provide unit test that failed without your PR, and success with your PR, it will help a lot to make this PR accepted ASAP.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [logging-log4j2] gengyuanzhe edited a comment on pull request #425: Update KafkaManager.java
Posted by GitBox <gi...@apache.org>.
gengyuanzhe edited a comment on pull request #425:
URL: https://github.com/apache/logging-log4j2/pull/425#issuecomment-696154623
In my opition, `org.apache.logging.log4j.kafka.appender.KafkaManager#releaseSub` should close the kafka producer when `reconfigure()`. The `close()` should not be invoked in`startup()`, it is confused.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [logging-log4j2] wuqian0808 commented on pull request #425: Update KafkaManager.java
Posted by GitBox <gi...@apache.org>.
wuqian0808 commented on pull request #425:
URL: https://github.com/apache/logging-log4j2/pull/425#issuecomment-696016546
Configure kafkaappender in log4j2.xml, then call below steps serveral times. The kafka thread will increase. LoggerContext.getContext(false).reconfigure();
LoggerContext.getContext(false).reconfigure();
LoggerContext.getContext(false).reconfigure();
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [logging-log4j2] wuqian0808 closed pull request #425: Update KafkaManager.java
Posted by GitBox <gi...@apache.org>.
wuqian0808 closed pull request #425:
URL: https://github.com/apache/logging-log4j2/pull/425
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [logging-log4j2] wuqian0808 closed pull request #425: Update KafkaManager.java
Posted by GitBox <gi...@apache.org>.
wuqian0808 closed pull request #425:
URL: https://github.com/apache/logging-log4j2/pull/425
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [logging-log4j2] gengyuanzhe commented on pull request #425: Update KafkaManager.java
Posted by GitBox <gi...@apache.org>.
gengyuanzhe commented on pull request #425:
URL: https://github.com/apache/logging-log4j2/pull/425#issuecomment-698385158
I tried your case, and you are right, the KafkaAppender do have the problem of thead leak. But **close when startup** is not the correct way to solve this problem. Try to find why the **count in abstractmanager** increase each time when reconfigure(), fix it and make sure the kafkaManager close when prev.stop(). You'd better open an [issue](https://issues.apache.org/jira/browse/LOG4J2) to track this problem. And if you can provide unit test that failed without your PR, and success with your PR, it will help a lot to make this PR accepted ASAP.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [logging-log4j2] wuqian0808 commented on pull request #425: Update KafkaManager.java
Posted by GitBox <gi...@apache.org>.
wuqian0808 commented on pull request #425:
URL: https://github.com/apache/logging-log4j2/pull/425#issuecomment-698699334
> I tried your case, and you are right, the KafkaAppender do have the problem of thead leak. But **close when startup** is not the correct way to solve this problem. Try to find why the **count in abstractmanager** increase each time when reconfigure(), fix it and make sure the kafkaManager close when prev.stop(). You'd better open an [issue](https://issues.apache.org/jira/browse/LOG4J2) to track this problem. And if you can provide unit test that failed without your PR, and success with your PR, it will help a lot to make this PR accepted ASAP.
The count is correct. It means the count of appenders using this manager. As we can see, the AbstarctManager will store a map of all static log4j2 managers. So during reconfigure, if managers is used by new config, the manager shouldn't be closed. Instead I just added a logic to check whether new kafka thread should be opended.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [logging-log4j2] wuqian0808 commented on pull request #425: Update KafkaManager.java
Posted by GitBox <gi...@apache.org>.
wuqian0808 commented on pull request #425:
URL: https://github.com/apache/logging-log4j2/pull/425#issuecomment-696485108
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [logging-log4j2] gengyuanzhe edited a comment on pull request #425: Update KafkaManager.java
Posted by GitBox <gi...@apache.org>.
gengyuanzhe edited a comment on pull request #425:
URL: https://github.com/apache/logging-log4j2/pull/425#issuecomment-696154623
In my opition, `org.apache.logging.log4j.kafka.appender.KafkaManager#releaseSub` should close the kafka producer when `reconfigure()`. The `close()` should not be invoked in`startup()`, it is confused.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [logging-log4j2] wuqian0808 commented on pull request #425: Update KafkaManager.java
Posted by GitBox <gi...@apache.org>.
wuqian0808 commented on pull request #425:
URL: https://github.com/apache/logging-log4j2/pull/425#issuecomment-698675698
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org