vy commented on code in PR #1961:
URL: https://github.com/apache/logging-log4j2/pull/1961#discussion_r1392146437
##########
log4j-api/src/main/java/org/apache/logging/log4j/spi/ThreadContextMapFactory.java:
##########
@@ -88,7 +89,7 @@ public static ThreadContextMap createThreadContextMap() {
try {
final Class<?> clazz = cl.loadClass(ThreadContextMapName);
if (ThreadContextMap.class.isAssignableFrom(clazz)) {
- result = (ThreadContextMap) clazz.newInstance();
+ result = (ThreadContextMap)
clazz.getDeclaredConstructor().newInstance();
Review Comment:
Why is this not a `LoaderUtil.newInstanceOf(clazz)`?
##########
log4j-api/src/main/java/org/apache/logging/log4j/util/Base64Util.java:
##########
@@ -53,7 +54,7 @@ public static String encode(final String str) {
if (str == null) {
return null;
}
- final byte [] data = str.getBytes();
+ final byte [] data = str.getBytes(Charset.defaultCharset());
Review Comment:
Shouldn't rather we getting the charset from the caller? _Default charset_
is an assumption and can very well be wrong.
##########
log4j-core/src/main/java/org/apache/logging/log4j/core/async/DisruptorUtil.java:
##########
@@ -82,35 +84,31 @@ static int calculateRingBufferSize(final String
propertyName) {
}
static ExceptionHandler<RingBufferLogEvent>
getAsyncLoggerExceptionHandler() {
- final String cls =
PropertiesUtil.getProperties().getStringProperty("AsyncLogger.ExceptionHandler");
- if (cls == null) {
- return new AsyncLoggerDefaultExceptionHandler();
- }
+ ExceptionHandler<RingBufferLogEvent> handler = null;
try {
- @SuppressWarnings("unchecked")
- final Class<? extends ExceptionHandler<RingBufferLogEvent>> klass =
- (Class<? extends ExceptionHandler<RingBufferLogEvent>>)
Loader.loadClass(cls);
- return klass.newInstance();
- } catch (final Exception ignored) {
- LOGGER.debug("Invalid AsyncLogger.ExceptionHandler value: error
creating {}: ", cls, ignored);
- return new AsyncLoggerDefaultExceptionHandler();
+ handler =
Review Comment:
You don't need a `handler` variable'; you can simply place a `return`
statement here and at the `catch` return the default.
##########
log4j-api/src/main/java/org/apache/logging/log4j/util/ProcessIdUtil.java:
##########
@@ -38,7 +40,7 @@ public static String getProcessId() {
final Object runtimeMXBean = getRuntimeMXBean.invoke(null);
final String name = (String) getName.invoke(runtimeMXBean);
//String name = ManagementFactory.getRuntimeMXBean().getName();
//JMX not allowed on Android
- return name.split("@")[0]; // likely works on most platforms
+ return name.split("@", -1)[0]; // likely works on most platforms
Review Comment:
Shouldn't this be `1` instead?
##########
log4j-core/src/main/java/org/apache/logging/log4j/core/impl/ContextDataInjectorFactory.java:
##########
@@ -66,21 +67,17 @@ public class ContextDataInjectorFactory {
* @see ContextDataInjector
*/
public static ContextDataInjector createInjector() {
- final String className =
PropertiesUtil.getProperties().getStringProperty("log4j2.ContextDataInjector");
- if (className == null) {
- return createDefaultInjector();
- }
+ ContextDataInjector injector = null;
try {
- final Class<? extends ContextDataInjector> cls =
Loader.loadClass(className).asSubclass(
- ContextDataInjector.class);
- return cls.newInstance();
- } catch (final Exception dynamicFailed) {
- final ContextDataInjector result = createDefaultInjector();
- StatusLogger.getLogger().warn(
- "Could not create ContextDataInjector for '{}', using
default {}: {}",
- className, result.getClass().getName(), dynamicFailed);
- return result;
+ injector =
+
LoaderUtil.newCheckedInstanceOfProperty(CONTEXT_DATA_INJECTOR_PROPERTY,
ContextDataInjector.class);
+ } catch (final ReflectiveOperationException e) {
+ StatusLogger.getLogger().warn("Could not create
ContextDataInjector: {}", e.getMessage(), e);
+ }
+ if (injector != null) {
+ return injector;
}
+ return createDefaultInjector();
Review Comment:
`injector` variable is not needed. See my earlier comments.
##########
log4j-api/src/main/java/org/apache/logging/log4j/util/Constants.java:
##########
@@ -107,7 +107,7 @@ private static int getMajorVersion() {
}
static int getMajorVersion(final String version) {
- final String[] parts = version.split("-|\\.");
+ final String[] parts = version.split("-|\\.", -1);
Review Comment:
We are only interested in first two parts, shouldn't this be `2` instead?
##########
log4j-core/src/main/java/org/apache/logging/log4j/core/async/DisruptorUtil.java:
##########
@@ -82,35 +84,31 @@ static int calculateRingBufferSize(final String
propertyName) {
}
static ExceptionHandler<RingBufferLogEvent>
getAsyncLoggerExceptionHandler() {
- final String cls =
PropertiesUtil.getProperties().getStringProperty("AsyncLogger.ExceptionHandler");
- if (cls == null) {
- return new AsyncLoggerDefaultExceptionHandler();
- }
+ ExceptionHandler<RingBufferLogEvent> handler = null;
try {
- @SuppressWarnings("unchecked")
- final Class<? extends ExceptionHandler<RingBufferLogEvent>> klass =
- (Class<? extends ExceptionHandler<RingBufferLogEvent>>)
Loader.loadClass(cls);
- return klass.newInstance();
- } catch (final Exception ignored) {
- LOGGER.debug("Invalid AsyncLogger.ExceptionHandler value: error
creating {}: ", cls, ignored);
- return new AsyncLoggerDefaultExceptionHandler();
+ handler =
+
LoaderUtil.newCheckedInstanceOfProperty(LOGGER_EXCEPTION_HANDLER_PROPERTY,
ExceptionHandler.class);
+ } catch (final ReflectiveOperationException e) {
+ LOGGER.debug("Invalid AsyncLogger.ExceptionHandler value: {}",
e.getMessage(), e);
+ }
+ if (handler != null) {
+ return handler;
}
+ return new AsyncLoggerDefaultExceptionHandler();
}
static ExceptionHandler<AsyncLoggerConfigDisruptor.Log4jEventWrapper>
getAsyncLoggerConfigExceptionHandler() {
- final String cls =
PropertiesUtil.getProperties().getStringProperty("AsyncLoggerConfig.ExceptionHandler");
- if (cls == null) {
- return new AsyncLoggerConfigDefaultExceptionHandler();
- }
+ ExceptionHandler<AsyncLoggerConfigDisruptor.Log4jEventWrapper> handler
= null;
try {
- @SuppressWarnings("unchecked")
- final Class<? extends
ExceptionHandler<AsyncLoggerConfigDisruptor.Log4jEventWrapper>> klass =
- (Class<? extends
ExceptionHandler<AsyncLoggerConfigDisruptor.Log4jEventWrapper>>)
Loader.loadClass(cls);
- return klass.newInstance();
- } catch (final Exception ignored) {
- LOGGER.debug("Invalid AsyncLoggerConfig.ExceptionHandler value:
error creating {}: ", cls, ignored);
- return new AsyncLoggerConfigDefaultExceptionHandler();
+ handler = LoaderUtil.newCheckedInstanceOfProperty(
Review Comment:
See my comment above. `handler` variable is not needed.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
