vy commented on code in PR #2396:
URL: https://github.com/apache/logging-log4j2/pull/2396#discussion_r1539971265
##########
log4j-1.2-api/src/main/java/org/apache/log4j/config/PropertiesConfigurationFactory.java:
##########
@@ -45,18 +45,26 @@ public class PropertiesConfigurationFactory extends
ConfigurationFactory {
*/
protected static final String DEFAULT_PREFIX = "log4j";
+ private final boolean enabled;
+
+ public PropertiesConfigurationFactory() {
+ this(PropertyEnvironment.getGlobal());
+ }
+
+ private PropertiesConfigurationFactory(final PropertyEnvironment props) {
+ this.enabled = props.getBooleanProperty(LOG4J1_EXPERIMENTAL)
+ || props.getStringProperty(LOG4J1_CONFIGURATION_FILE_PROPERTY)
!= null;
+ }
+
@Override
protected String[] getSupportedTypes() {
- if (!PropertiesUtil.getProperties()
- .getBooleanProperty(ConfigurationFactory.LOG4J1_EXPERIMENTAL,
Boolean.FALSE)) {
- return null;
- }
- return new String[] {FILE_EXTENSION};
+ return enabled ? new String[] {FILE_EXTENSION} : null;
}
@Override
public Configuration getConfiguration(final LoggerContext loggerContext,
final ConfigurationSource source) {
- final int interval =
PropertiesUtil.getProperties().getIntegerProperty(Log4j1Configuration.MONITOR_INTERVAL,
0);
+ final int interval =
+
PropertyEnvironment.getGlobal().getIntegerProperty(Log4j1Configuration.MONITOR_INTERVAL,
0);
Review Comment:
Shouldn't this be using `loggerContext.getPropertyEnvironment()` instead?
##########
log4j-layout-template-json/src/main/java/org/apache/logging/log4j/layout/template/json/JsonTemplateLayoutDefaults.java:
##########
@@ -20,14 +20,13 @@
import java.nio.charset.StandardCharsets;
import java.util.Locale;
import java.util.TimeZone;
-import org.apache.logging.log4j.util.PropertiesUtil;
-import org.apache.logging.log4j.util.PropertyEnvironment;
+import org.apache.logging.log4j.kit.env.PropertyEnvironment;
public final class JsonTemplateLayoutDefaults {
private JsonTemplateLayoutDefaults() {}
- private static final PropertyEnvironment PROPERTIES =
PropertiesUtil.getProperties();
+ private static final PropertyEnvironment PROPERTIES =
PropertyEnvironment.getGlobal();
Review Comment:
Shouldn't this class be replaced with a `JsonTemplateLayoutKeys` instead?
##########
log4j-1.2-api/src/test/java/org/apache/log4j/config/PropertiesConfigurationFactoryTest.java:
##########
@@ -32,9 +32,7 @@ public class PropertiesConfigurationFactoryTest {
@BeforeClass
public static void beforeClass() {
- System.setProperty(
-
ConfigurationFactory.LOG4J1_CONFIGURATION_FILE_PROPERTY.getSystemKey(),
- "target/test-classes/log4j1-file-1.properties");
+ System.setProperty(LOG4J1_CONFIGURATION_FILE_PROPERTY,
"target/test-classes/log4j1-file-1.properties");
Review Comment:
I don't want to create more work than what is necessary, but maybe
`@SetSystemProperty` does work here out of the box.
##########
log4j-core/src/main/java/org/apache/logging/log4j/core/LoggerContext.java:
##########
@@ -201,11 +204,14 @@ public static void checkMessageFactory(final
ExtendedLogger logger, final Messag
}
}
- @Override
public PropertyEnvironment getEnvironment() {
return environment;
}
+ private CoreKeys.LoggerContext contextProperties() {
Review Comment:
I am in love with the new type-safe property injection. Can we also try to
follow a certain class naming convention there too, please? For instance,
consider this example: it returns a class called `LoggerContext`.
:exploding_head: I wish, it would be something like
`CoreProperties.LoggerContextProperties`. So when `LoggerContextProperties`
gets imported on demand, its name would not conflict with the `LoggerContext`
and also even in isolation would make great sense. (I am not a big fan of
_keys_, since they are certainly not _keys_, but _values_. IIRC, Spring Boot
uses `Properties`-suffixed class names for `@ConfigurationProperties`
-annotated classes too.)
##########
log4j-api-test/src/main/java/org/apache/logging/log4j/test/junit/Resources.java:
##########
@@ -19,12 +19,37 @@
import org.junit.jupiter.api.parallel.ResourceLock;
/**
- * Constants to use the the {@link ResourceLock} annotation.
- *
+ * Constants to use the {@link ResourceLock} annotation.
*/
-public class Resources {
+public final class Resources {
Review Comment:
*Nit:* Could you use some other class name than _resources_, please? It is a
term harassed in all code bases and doesn't communicate any test-related
context. Maybe `JUnitResourceNames`? `TestResourceNames`?
`SharedTestResourceNames`?
##########
log4j-1.2-api/src/main/java/org/apache/log4j/xml/XmlConfigurationFactory.java:
##########
@@ -46,18 +46,26 @@ public class XmlConfigurationFactory extends
ConfigurationFactory {
*/
protected static final String DEFAULT_PREFIX = "log4j";
+ private final boolean enabled;
+
+ public XmlConfigurationFactory() {
+ this(PropertyEnvironment.getGlobal());
+ }
+
+ private XmlConfigurationFactory(final PropertyEnvironment props) {
+ this.enabled =
props.getBooleanProperty(LOG4J1_CONFIGURATION_FILE_PROPERTY)
+ || props.getStringProperty(LOG4J1_CONFIGURATION_FILE_PROPERTY)
!= null;
+ }
+
@Override
protected String[] getSupportedTypes() {
- if (!PropertiesUtil.getProperties()
- .getBooleanProperty(ConfigurationFactory.LOG4J1_EXPERIMENTAL,
Boolean.FALSE)) {
- return null;
- }
- return new String[] {FILE_EXTENSION};
+ return enabled ? new String[] {FILE_EXTENSION} : null;
}
@Override
public Configuration getConfiguration(final LoggerContext loggerContext,
final ConfigurationSource source) {
- final int interval =
PropertiesUtil.getProperties().getIntegerProperty(Log4j1Configuration.MONITOR_INTERVAL,
0);
+ final int interval =
+
PropertyEnvironment.getGlobal().getIntegerProperty(Log4j1Configuration.MONITOR_INTERVAL,
0);
Review Comment:
Shouldn't this be using `loggerContext.getPropertyEnvironment()` instead?
##########
log4j-1.2-api/src/test/java/org/apache/log4j/config/AutoConfigTest.java:
##########
@@ -27,19 +27,21 @@
import org.apache.log4j.bridge.AppenderAdapter;
import org.apache.log4j.spi.LoggingEvent;
import org.apache.logging.log4j.core.Appender;
+import org.apache.logging.log4j.core.LoggerContext;
import org.apache.logging.log4j.core.config.Configuration;
-import org.apache.logging.log4j.core.test.junit.LegacyLoggerContextSource;
import org.junit.jupiter.api.Test;
+import org.junitpioneer.jupiter.SetSystemProperty;
/**
* Test configuration from XML.
*/
public class AutoConfigTest {
@Test
- @LegacyLoggerContextSource("log4j.xml")
- public void testListAppender(final
org.apache.logging.log4j.core.LoggerContext context) {
+ @SetSystemProperty(key = "log4j.configuration", value = "log4j.xml")
Review Comment:
Can't we use `ConfigurationFactory.LOG4J1_CONFIGURATION_FILE_PROPERTY` for
`key`?
##########
log4j-core/src/main/java/org/apache/logging/log4j/core/impl/Log4jPropertyKey.java:
##########
Review Comment:
Thank you for nuking this madness! :heart:
##########
log4j-api-test/src/main/java/org/apache/logging/log4j/test/junit/Tags.java:
##########
@@ -14,25 +14,20 @@
* See the License for the specific language governing permissions and
* limitations under the License.
*/
-package org.apache.logging.log4j.core.annotation;
-
-import java.lang.annotation.Documented;
-import java.lang.annotation.ElementType;
-import java.lang.annotation.Retention;
-import java.lang.annotation.RetentionPolicy;
-import java.lang.annotation.Target;
-import org.apache.logging.log4j.core.impl.Log4jPropertyKey;
-import org.apache.logging.log4j.plugins.condition.Conditional;
+package org.apache.logging.log4j.test.junit;
/**
- * Checks if a Log4j property is present or matches a specific non-empty value.
+ * A set of tags for JUnit 5 tests.
*/
-@Retention(RetentionPolicy.RUNTIME)
-@Target({ElementType.TYPE, ElementType.ANNOTATION_TYPE, ElementType.METHOD})
-@Documented
-@Conditional(OnPropertyKeyCondition.class)
-public @interface ConditionalOnPropertyKey {
- Log4jPropertyKey key();
+public final class Tags {
Review Comment:
*Nit:* Similar to `Resources`, `Tags` don't communicate any context.
I think we already talked about this earlier. I am against `Tags`. Over time
people tend to forget its existence and it is destined to become obsolete.
##########
log4j-jctools/src/main/java/org/apache/logging/log4j/jctools/JCToolsRecyclerFactoryProvider.java:
##########
@@ -52,7 +51,7 @@ public String getName() {
@Override
public RecyclerFactory createForEnvironment(final PropertyEnvironment
environment) {
requireNonNull(environment, "environment");
- final int capacity =
environment.getIntegerProperty(LoggingSystemProperty.RECYCLER_CAPACITY,
DEFAULT_CAPACITY);
+ final int capacity =
environment.getIntegerProperty("Recycler.capacity", DEFAULT_CAPACITY);
Review Comment:
Will there be a recycler-specific `*Keys` class?
##########
log4j-api-test/src/main/java/org/apache/logging/log4j/test/junit/Tags.java:
##########
@@ -14,25 +14,20 @@
* See the License for the specific language governing permissions and
* limitations under the License.
*/
-package org.apache.logging.log4j.core.annotation;
-
-import java.lang.annotation.Documented;
-import java.lang.annotation.ElementType;
-import java.lang.annotation.Retention;
-import java.lang.annotation.RetentionPolicy;
-import java.lang.annotation.Target;
-import org.apache.logging.log4j.core.impl.Log4jPropertyKey;
-import org.apache.logging.log4j.plugins.condition.Conditional;
+package org.apache.logging.log4j.test.junit;
/**
- * Checks if a Log4j property is present or matches a specific non-empty value.
+ * A set of tags for JUnit 5 tests.
*/
-@Retention(RetentionPolicy.RUNTIME)
-@Target({ElementType.TYPE, ElementType.ANNOTATION_TYPE, ElementType.METHOD})
-@Documented
-@Conditional(OnPropertyKeyCondition.class)
-public @interface ConditionalOnPropertyKey {
- Log4jPropertyKey key();
+public final class Tags {
+
+ /**
+ * Marks tests that don't modify the global environment.
+ * <p>
+ * These tests can safely be run in parallel.
+ * </p>
+ */
+ public static final String PARALLEL = "parallel";
Review Comment:
Word `parallel` certainly doesn't communicate `tests that don't modify the
global environment`. Maybe `THREAD_SAFE`? I dunno...
--
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-unsubscr...@logging.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
