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 2022/07/22 15:41:46 UTC
[GitHub] [logging-log4j2] bigmarvin opened a new pull request, #973: LOG4J2-3558: make default priority configurable
bigmarvin opened a new pull request, #973:
URL: https://github.com/apache/logging-log4j2/pull/973
I filed [LOG4J2-3558](https://issues.apache.org/jira/browse/LOG4J2-3558) to describe the context behind.
Essentially, we want to check how things work when loggers run in lower priority, and we find it's not feasible until some change like this is made.
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@logging.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] LOG4J2-3558: make default priority configurable (logging-log4j2)
Posted by "vy (via GitHub)" <gi...@apache.org>.
vy closed pull request #973: LOG4J2-3558: make default priority configurable
URL: https://github.com/apache/logging-log4j2/pull/973
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@logging.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [logging-log4j2] bigmarvin commented on pull request #973: LOG4J2-3558: make default priority configurable
Posted by GitBox <gi...@apache.org>.
bigmarvin commented on PR #973:
URL: https://github.com/apache/logging-log4j2/pull/973#issuecomment-1201837955
@ppkarwasz , may I ask for your review?
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@logging.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] LOG4J2-3558: make default priority configurable (logging-log4j2)
Posted by "ppkarwasz (via GitHub)" <gi...@apache.org>.
ppkarwasz commented on PR #973:
URL: https://github.com/apache/logging-log4j2/pull/973#issuecomment-1449446582
This was closed automatically by Github because we renamed the `release-2.x` branch to `2.x`. Feel free to resubmit to the `2.x` branch.
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@logging.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [logging-log4j2] bigmarvin commented on pull request #973: LOG4J2-3558: make default priority configurable
Posted by GitBox <gi...@apache.org>.
bigmarvin commented on PR #973:
URL: https://github.com/apache/logging-log4j2/pull/973#issuecomment-1215142820
@garydgregory / @remkop / @rgoers , may I ask for your review? I double checked CONTRIBUTING.md while I'm still not really sure what's the missing piece that blocks this PR from getting some feedback. You are picked based on the contribution to this repository, but please feel free to let me know if some other reviewer is more appropriate. Thanks!
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@logging.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [logging-log4j2] bigmarvin commented on pull request #973: LOG4J2-3558: make default priority configurable
Posted by GitBox <gi...@apache.org>.
bigmarvin commented on PR #973:
URL: https://github.com/apache/logging-log4j2/pull/973#issuecomment-1218238927
Thank you for the feedbacks! I've just pushed few more changes to address the issues. Could you please take a look?
The story behind would be (presumed) scheduling issue to be tuned. When the load gets high, a few things are observed --
1. The duration of various tasks (say `Runnable`) behind REST and gRPC traffic gets expanded
2. The end-to-end duration of gRPC traffic gets expanded a little more than REST traffic on server side
3. The client-side e2e duration of gRPC traffic gets expanded way further than its server-side e2e duration
Item 1 is expected as application just has more work to do. Item 2 is also kind of understandable, as a REST call usually have 2 tasks for I/O and 1 task for processing while a gRPC call would need more tasks. There can be extra delay when we break a single task into multiple, even when run by same `Executor` in sequence.
Item 3 is something concerning. For example, when the latency rises from 5 ms to 6 on server side, it can be from 7 to 20 ms on client side. According to some stats, the duration of tasks in gRPC traffic doesn't rise much and `Executor`s behind don't run out.
The promising theory at present would be some delay in scheduling, e.g. tasks doesn't get executed in time or executing ones don't get as many CPU resources as original because more supporting tasks (e.g. stats aggregation, notifications, logging) are populated and competing for resources.
The changes in this PR facilitate the tunning of thread priorities based on the theory above. Please feel free to let me know what you think, as it's just some theory.
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@logging.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [logging-log4j2] vy commented on a diff in pull request #973: LOG4J2-3558: make default priority configurable
Posted by GitBox <gi...@apache.org>.
vy commented on code in PR #973:
URL: https://github.com/apache/logging-log4j2/pull/973#discussion_r946925348
##########
log4j-core/src/main/java/org/apache/logging/log4j/core/util/Log4jThreadFactory.java:
##########
@@ -94,4 +109,8 @@ public Thread newThread(final Runnable runnable) {
return thread;
}
+ private static int getThreadPriority() {
+ int result = PropertiesUtil.getProperties().getIntegerProperty(THREAD_PRIORITY, Thread.NORM_PRIORITY);
+ return Math.min(Math.max(Thread.MIN_PRIORITY, result), Thread.MAX_PRIORITY);
Review Comment:
I would do the following instead:
```suggestion
final String property = "log4j2.thread.priority";
int priority = PropertiesUtil.getProperties().getIntegerProperty(property, Thread.NORM_PRIORITY);
if (priority < Thread.MIN_PRIORITY || priority > Thread.MAX_PRIORITY) {
StatusLogger.getLogger().warn("invalid {}: {}", property, priority);
priority = Thread.NORM_PRIORITY;
}
return priority;
```
There are many other places in Log4j where threads are created. We might want to choose another name for the property. Further, it might better to pull this into one of the `Constants` classes.
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@logging.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [logging-log4j2] remkop commented on pull request #973: LOG4J2-3558: make default priority configurable
Posted by GitBox <gi...@apache.org>.
remkop commented on PR #973:
URL: https://github.com/apache/logging-log4j2/pull/973#issuecomment-1217984987
@bigmarvin Have you tried forking the project and trying your changes there?
Can you explain what benefits you saw in your application when you used it with your customized Log4j fork with configurable priorities?
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@logging.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [logging-log4j2] vy commented on a diff in pull request #973: LOG4J2-3558: make default priority configurable
Posted by GitBox <gi...@apache.org>.
vy commented on code in PR #973:
URL: https://github.com/apache/logging-log4j2/pull/973#discussion_r946927369
##########
log4j-core/src/main/java/org/apache/logging/log4j/core/util/Log4jThreadFactory.java:
##########
@@ -26,6 +28,7 @@
* @since 2.7
*/
public class Log4jThreadFactory implements ThreadFactory {
+ public static final String THREAD_PRIORITY = "log4j2.thread.priority";
Review Comment:
```suggestion
```
##########
log4j-core/src/main/java/org/apache/logging/log4j/core/util/Log4jThreadFactory.java:
##########
@@ -94,4 +109,8 @@ public Thread newThread(final Runnable runnable) {
return thread;
}
+ private static int getThreadPriority() {
+ int result = PropertiesUtil.getProperties().getIntegerProperty(THREAD_PRIORITY, Thread.NORM_PRIORITY);
+ return Math.min(Math.max(Thread.MIN_PRIORITY, result), Thread.MAX_PRIORITY);
Review Comment:
I would do the following instead:
```suggestion
final String property = "log4j2.thread.priority";
int priority = PropertiesUtil.getProperties().getIntegerProperty(property, Thread.NORM_PRIORITY);
if (priority < Thread.MIN_PRIORITY || priority > Thread.MAX_PRIORITY) {
StatusLogger.getLogger().warn("invalid {}: {}", property, priority);
priority = Thread.NORM_PRIORITY;
}
return priority;
```
There are many other places in Log4j where threads are created. We might want to choose another name for the property.
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@logging.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [logging-log4j2] remkop commented on pull request #973: LOG4J2-3558: make default priority configurable
Posted by GitBox <gi...@apache.org>.
remkop commented on PR #973:
URL: https://github.com/apache/logging-log4j2/pull/973#issuecomment-1219134990
@bigmarvin Thanks for the clarification.
My first concern is that thread priority may not make any difference for your application, and then we end up having added public API to Log4j that does not help anyone (and probably confuses users). And for backwards compatibility we would not want to remove it. We don't want to end up in that situation.
What I would suggest is that you actually try this in your application.
You already have a fork of Log4j (that is where this PR is coming from), so just build it so you have the binaries (SNAPSHOT binaries of the next version), and run your application with those Log4j SNAPSHOT binaries.
You can then configure the priority, and confirm whether or not it makes a difference.
Once you have confirmed that it makes a difference, then you can argue that this change will benefit many users and that you think it should be included in the Log4j library.
That does not necessarily mean that your proposal will be accepted, my impression is that thread priority is not a very reliable mechanism, perhaps an outdated mechanism, and may work differently (or not at all) in different environments, but until you have evidence that it at least made a difference in _your_ environment it is unlikely that this PR will be accepted.
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@logging.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org