lhotari commented on code in PR #25515:
URL: https://github.com/apache/pulsar/pull/25515#discussion_r3080575880
##########
conf/log4j2.yaml:
##########
@@ -66,27 +66,48 @@ Configuration:
# Rolling file appender configuration
RollingFile:
- name: RollingFile
- fileName: "${sys:pulsar.log.dir}/${sys:pulsar.log.file}"
- filePattern:
"${sys:pulsar.log.dir}/${sys:pulsar.log.file}-%d{MM-dd-yyyy}-%i.log.gz"
- immediateFlush: ${sys:pulsar.log.immediateFlush}
- PatternLayout:
- Pattern: "%d{ISO8601_OFFSET_DATE_TIME_HHMM} [%t] %-5level %logger{36}
- %msg %X%n"
- Policies:
- TimeBasedTriggeringPolicy:
- interval: 1
- modulate: true
- SizeBasedTriggeringPolicy:
- size: 1 GB
- # Delete file older than 30days
- DefaultRolloverStrategy:
- Delete:
- basePath: ${sys:pulsar.log.dir}
- maxDepth: 2
- IfFileName:
- glob: "${sys:pulsar.log.file}*log.gz"
- IfLastModified:
- age: 30d
+ - name: RollingFile
+ fileName: "${sys:pulsar.log.dir}/${sys:pulsar.log.file}"
+ filePattern:
"${sys:pulsar.log.dir}/${sys:pulsar.log.file}-%d{MM-dd-yyyy}-%i.log.gz"
+ immediateFlush: ${sys:pulsar.log.immediateFlush}
+ PatternLayout:
+ Pattern: "%d{ISO8601_OFFSET_DATE_TIME_HHMM} [%t] %-5level
%logger{36} - %msg %X%n"
+ Policies:
+ TimeBasedTriggeringPolicy:
+ interval: 1
+ modulate: true
+ SizeBasedTriggeringPolicy:
+ size: 1 GB
+ # Delete file older than 30days
+ DefaultRolloverStrategy:
+ Delete:
+ basePath: ${sys:pulsar.log.dir}
+ maxDepth: 2
+ IfFileName:
+ glob: "${sys:pulsar.log.file}*log.gz"
+ IfLastModified:
+ age: 30d
+ - name: RollingFileJson
+ fileName: "${sys:pulsar.log.dir}/${sys:pulsar.log.file}"
+ filePattern:
"${sys:pulsar.log.dir}/${sys:pulsar.log.file}-%d{MM-dd-yyyy}-%i.log.gz"
+ immediateFlush: ${sys:pulsar.log.immediateFlush}
+ JsonTemplateLayout:
+ eventTemplateUri: ${sys:pulsar.log.console.json.template}
+ Policies:
+ TimeBasedTriggeringPolicy:
+ interval: 1
+ modulate: true
+ SizeBasedTriggeringPolicy:
+ size: 1 GB
+ # Delete file older than 30days
+ DefaultRolloverStrategy:
+ Delete:
+ basePath: ${sys:pulsar.log.dir}
+ maxDepth: 2
+ IfFileName:
+ glob: "${sys:pulsar.log.file}*log.gz"
+ IfLastModified:
+ age: 30d
Review Comment:
Claude suggested using a different type of solution:
Log4j2 2.19+ introduced Arbiter — compile-time conditional blocks evaluated
once at configuration load. SystemPropertyArbiter checks a system property and
includes/excludes config sections accordingly:
```yaml
name: RollingFile
fileName: "${sys:pulsar.log.dir}/${sys:pulsar.log.file}"
filePattern:
"${sys:pulsar.log.dir}/${sys:pulsar.log.file}-%d{MM-dd-yyyy}-%i.log.gz"
immediateFlush: ${sys:pulsar.log.immediateFlush}
SystemPropertyArbiter:
- propertyName: pulsar.log.format
propertyValue: json
JsonTemplateLayout:
eventTemplateUri: ${sys:pulsar.log.console.json.template}
- propertyName: pulsar.log.format
propertyValue: text
PatternLayout:
Pattern: "%d{ISO8601_OFFSET_DATE_TIME_HHMM} [%t] %-5level
%logger{36} - %msg %X%n"
```
The concern about the solution was this:
Both RollingFile and RollingFileJson appenders point to the same fileName
and filePattern. Log4j2
initializes all appenders defined in the config at startup, regardless of
whether they're referenced by any logger. This means
both appenders will attempt to open the same file. Log4j2 uses a
FileManager keyed by file path, so both appenders will share the
same underlying FileManager instance — they won't conflict on file locks.
However, there are subtle concerns:
1. Both appenders are initialized — even the unused one opens the file and
registers its rollover policies. The unused
TimeBasedTriggeringPolicy and SizeBasedTriggeringPolicy will still be
active on the shared FileManager, potentially triggering
unnecessary rollovers.
2. Mixed format risk — if someone misconfigures and both appenders end up
active (e.g., one via RoutingAppender default and
another via pulsar.log.appender), the same file would contain mixed text +
JSON lines.
--
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]
