iloveeyjafjalla opened a new pull request, #11432:
URL: https://github.com/apache/rocketmq/pull/11432
### Which Issue(s) This PR Fixes
- Fixes #11431
### Brief Description
`rmq.tools.logback.xml` (installed as `conf/rmq.tools.logback.xml` and
selected by `distribution/bin/mqadmin` via `-Drmq.logback.configurationFile`)
attached `RocketmqCommon`, `RocketmqStore` and `RocketmqRemoting` to
`RocketmqBrokerAppender`, an appender that only exists in the broker and proxy
configurations — logback resolves `appender-ref`s within the file being loaded,
so the reference could never be satisfied:
```xml
<logger name="RocketmqCommon" additivity="false" level="INFO">
<appender-ref ref="RocketmqBrokerAppender"/> <!-- not defined in this
file -->
</logger>
```
Because those loggers also set `additivity="false"`, dropping the reference
left them attached to **no appender**, so everything logged through the
client/store/remoting layers during an `mqadmin` run was discarded — and this
logback build reports no warning for the unresolved reference, so the loss was
silent. The sibling configurations keep the component loggers on an appender of
their own (`namesrv` uses `RocketmqNamesrvAppender` for exactly these loggers,
`RocketmqTools` in this file already used `RocketmqToolsAppender`), which is
the pattern restored here.
The change points the three loggers at `RocketmqToolsAppender` — the
appender this file defines, writing to
`${user.home}/logs/rocketmqlogs/tools.log`.
This is the same class of configuration mistake that was fixed in #11177 (an
appender registered under a name that did not match its definition). I also
checked every other shipped logback configuration for an `appender-ref` to an
appender it does not define: only this file had one.
### How Did You Test This Change?
New test
`LogbackConfigurationTest#testShippedToolsLogbackConfigResolvesEveryAppenderRef`:
it loads the shipped resource with the project's own `JoranConfiguratorExt`
(the loader the runtime uses, including the shaded-class rewrite) into a fresh
logger context with a status collector, asserts the load reports no errors, and
asserts that `RocketmqCommon`, `RocketmqStore` and `RocketmqRemoting` are each
attached to at least one appender.
Results on Temurin JDK 8 (the JDK CI uses), base `a7edeecfb` plus this patch:
- before the fix: `java.lang.AssertionError: RocketmqCommon must be attached
to an appender`
- after the fix: `LogbackConfigurationTest` — `Tests run: 1, Failures: 0,
Errors: 0`
- `mvn -B -ntp -pl tools clean package
-Dtest='!QueryMsgTraceByIdSubCommandTest'`: `BUILD SUCCESS`, 160 tests / 0
failures / 0 errors / 5 skipped, `You have 0 Checkstyle violations.`,
`BugInstance size is 0`.
Disclosure: `QueryMsgTraceByIdSubCommandTest` is excluded because it fails
on the unmodified base as well (`CODE: 208`, verified at base commit
`a7edeecfb` in a separate worktree during an earlier round), and
`skipAfterFailureCount=1` would otherwise hide the rest of the module.
This change was prepared with AI assistance (analysis, patch and test
drafting); the diff and every result above were reproduced locally by running
the module's tests against the base commit and the patched tree.
--
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]
