Skip to content

[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
apache:developfrom
iloveeyjafjalla:codex/tools-logback-appender
Open

iloveeyjafjalla wants to merge 1 commit into
apache:developfrom
iloveeyjafjalla:codex/tools-logback-appender

Conversation

@iloveeyjafjalla

Copy link
Copy Markdown
Contributor

Which Issue(s) This PR Fixes

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-refs within the file being loaded, so the reference could never be satisfied:

<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.

Copilot AI balanced review requested due to automatic review settings October 14, 2026 05:04

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

@RockteMQ-AI RockteMQ-AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 to RocketmqToolsAppender which is defined in the same file.
  • [Info] tools/src/test/java/.../LogbackConfigurationTest.java — Well-designed regression test. Uses the actual JoranConfiguratorExt to load the shipped config and validates that all appender-refs resolve. The CollectingStatusManager approach 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 RockteMQ-AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Bug] rmq.tools.logback.xml references an appender that is not defined in it, so mqadmin silently drops the Common/Store/Remoting logs

3 participants