You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@logging.apache.org by "ppkarwasz (via GitHub)" <gi...@apache.org> on 2024/01/22 14:50:41 UTC
[PR] Simplify async logger in `main` (logging-log4j2)
ppkarwasz opened a new pull request, #2227:
URL: https://github.com/apache/logging-log4j2/pull/2227
Due to the introduction of `Recycler` in `main` and related changes, many async loggers can be simplified:
* no thread name caching is required,
* in many methods we can use `ReusableLogEvent` instead of concrete classes,
* many casts from `ReadOnlyStringMap` to `StringMap` are not necessary.
--
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] Simplify async logger in `main` (logging-log4j2)
Posted by "vy (via GitHub)" <gi...@apache.org>.
vy commented on code in PR #2227:
URL: https://github.com/apache/logging-log4j2/pull/2227#discussion_r1463035845
##########
log4j-core-test/src/test/java/org/apache/logging/log4j/core/async/QueueFullAbstractTest.java:
##########
@@ -302,9 +302,11 @@ protected static void assertAsyncLoggerConfig(final LoggerContext ctx, final int
final Configuration config = ctx.getConfiguration();
assertThat(config).isNotNull();
assertThat(config.getRootLogger()).isInstanceOf(AsyncLoggerConfig.class);
- final AsyncLoggerConfigDisruptor disruptor = (AsyncLoggerConfigDisruptor) config.getAsyncLoggerConfigDelegate();
- final Field sizeField = field(AsyncLoggerConfigDisruptor.class, "ringBufferSize");
- assertThat(sizeField.get(disruptor)).isEqualTo(expectedBufferSize);
+ final AsyncLoggerConfigDisruptor disruptorWrapper =
+ (AsyncLoggerConfigDisruptor) config.getAsyncLoggerConfigDelegate();
+ final Disruptor<?> disruptor = (Disruptor<?>)
+ field(AsyncLoggerConfigDisruptor.class, "disruptor").get(disruptorWrapper);
Review Comment:
Just a reminder that `AsyncLoggerConfigDisruptor` could have had a package-local `getDisruptor` to ease testing, just like `getWaitStrategy()`. I am fine with the currently implemented reflection approach too.
We have a mixture of both all over the code base. [Excluding the reflection usage when package-local has accessibility problems due to tests being located in a different package,] I wish we would have had a convention for this.
--
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] Simplify async logger in `main` (logging-log4j2)
Posted by "ppkarwasz (via GitHub)" <gi...@apache.org>.
ppkarwasz commented on code in PR #2227:
URL: https://github.com/apache/logging-log4j2/pull/2227#discussion_r1463246553
##########
log4j-core-test/src/test/java/org/apache/logging/log4j/core/async/QueueFullAbstractTest.java:
##########
@@ -302,9 +302,11 @@ protected static void assertAsyncLoggerConfig(final LoggerContext ctx, final int
final Configuration config = ctx.getConfiguration();
assertThat(config).isNotNull();
assertThat(config.getRootLogger()).isInstanceOf(AsyncLoggerConfig.class);
- final AsyncLoggerConfigDisruptor disruptor = (AsyncLoggerConfigDisruptor) config.getAsyncLoggerConfigDelegate();
- final Field sizeField = field(AsyncLoggerConfigDisruptor.class, "ringBufferSize");
- assertThat(sizeField.get(disruptor)).isEqualTo(expectedBufferSize);
+ final AsyncLoggerConfigDisruptor disruptorWrapper =
+ (AsyncLoggerConfigDisruptor) config.getAsyncLoggerConfigDelegate();
+ final Disruptor<?> disruptor = (Disruptor<?>)
+ field(AsyncLoggerConfigDisruptor.class, "disruptor").get(disruptorWrapper);
Review Comment:
I corrected this part of code in #2228.
--
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] Simplify async logger in `main` (logging-log4j2)
Posted by "ppkarwasz (via GitHub)" <gi...@apache.org>.
ppkarwasz merged PR #2227:
URL: https://github.com/apache/logging-log4j2/pull/2227
--
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