logging-log4j2
logging-log4j2 copied to clipboard
Issue with Duration Parsing
I was working on something unrelated and needed to parse a string like 10s
or 10ms
, I did a quick search in my project and came up with:
https://github.com/apache/logging-log4j2/blob/b4986fe109d2a37d99b7b167bf50a460c0510c1e/log4j-api/src/main/java/org/apache/logging/log4j/util/PropertiesUtil.java#L639C1-L674C6
I'm not sure if it is a bug, but when it finds the first matching suffix it should immediately return, other wise it always falls through to SECONDS
for most of the covered cases and the order is slightly messed up as the SECONDS
could produce an early match for MINUTES
, HOURS
, and DAYS
.
@RapidTransit,
Thank you for catching this bug. As far as I can see, most invocations of this code (especially if the unit ends in s
) ends up with an uncaught NumberFormatException
.
@rgoers, this is only used in the Kubernetes plugin. Can we replace the entire logic with Duration.parse
?
Since the only users of PropertiesUtil#getDurationProperty
has been removed in #2218, I propose to replace the implementation of the method with a simple Duration#parse
.
@ppkarwasz, if there are no other system properties that need a TypeConverter<Duration>
, but the Kubernetes module, which is planned to be removed, remove this TypeConverter<Duration>
along with it too, please. Note that this implies no TypeConverter<Duration>
, not even one using Duration.parse(String)
.
@vy,
In Log4j Core there is no java.time.Duration
converter, there is however an o.a.l.l.c.a.rolling.action.Duration
converter created in the well-known spirit of bringing to Java 7 users Java 8 features today!
I can deprecate it and replace its usages with java.time.Duration
, but I'd rather do it in a separate PR.