Repository navigation
[ISSUE #11431] fix(tools): attach the component loggers of mqadmin to the appender its configuration defines - #11432
Open
iloveeyjafjalla wants to merge 1 commit into
Conversation
…appender the tools config defines
RockteMQ-AI
reviewed
Oct 14, 2026
RockteMQ-AI
left a comment
Contributor
There was a problem hiding this comment.
Review by github-manager-bot
Summary
Fixes rmq.tools.logback.xml where RocketmqCommon, RocketmqStore, and RocketmqRemoting loggers were referencing RocketmqBrokerAppender — an appender that only exists in broker/proxy configs, not in the tools config. Changes all three appender-refs to RocketmqToolsAppender and adds a regression test.
Findings
- [Info]
tools/src/main/resources/rmq.tools.logback.xml:77-87— Clean, minimal fix. The three logger references now correctly point toRocketmqToolsAppenderwhich is defined in the same file. - [Info]
tools/src/test/java/.../LogbackConfigurationTest.java— Well-designed regression test. Uses the actualJoranConfiguratorExtto load the shipped config and validates that all appender-refs resolve. TheCollectingStatusManagerapproach to capture logback status errors is effective and will catch similar misconfigurations in the future.
Suggestions
- No blocking issues. The fix is correct and the test provides good coverage for this class of configuration errors.
Automated review by github-manager-bot
RockteMQ-AI
approved these changes
Oct 16, 2026
RockteMQ-AI
left a comment
Contributor
There was a problem hiding this comment.
Review by github-manager-bot
Summary
Fixes mqadmin logging so that component loggers (e.g. MQAdminImpl) are attached to the appender defined in mqadmin log4j configuration, instead of only the root logger.
Findings
- [Info] MQAdminStartup.java:248-251 — The fix correctly attaches component loggers to the same appender configured for the root logger, ensuring log output goes to the expected destination.
- [Info] MQAdminStartupTest.java — Tests verify that log output from component loggers appears in the configured appender.
Verdict
LGTM. Clean fix for a logging configuration issue.
Automated review by github-manager-bot
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Which Issue(s) This PR Fixes
Brief Description
rmq.tools.logback.xml(installed asconf/rmq.tools.logback.xmland selected bydistribution/bin/mqadminvia-Drmq.logback.configurationFile) attachedRocketmqCommon,RocketmqStoreandRocketmqRemotingtoRocketmqBrokerAppender, an appender that only exists in the broker and proxy configurations — logback resolvesappender-refs within the file being loaded, so the reference could never be satisfied: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 anmqadminrun 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 (namesrvusesRocketmqNamesrvAppenderfor exactly these loggers,RocketmqToolsin this file already usedRocketmqToolsAppender), 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-refto 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 ownJoranConfiguratorExt(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 thatRocketmqCommon,RocketmqStoreandRocketmqRemotingare each attached to at least one appender.Results on Temurin JDK 8 (the JDK CI uses), base
a7edeecfbplus this patch:java.lang.AssertionError: RocketmqCommon must be attached to an appenderLogbackConfigurationTest—Tests run: 1, Failures: 0, Errors: 0mvn -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:
QueryMsgTraceByIdSubCommandTestis excluded because it fails on the unmodified base as well (CODE: 208, verified at base commita7edeecfbin a separate worktree during an earlier round), andskipAfterFailureCount=1would 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.