You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@seata.apache.org by "finalskiller (via GitHub)" <gi...@apache.org> on 2026/10/04 14:31:07 UTC
[PR] bugfix: load logstash/metric/kafka appenders from external config dir when LOG_BASH_DIR is set [incubator-seata]
finalskiller opened a new pull request, #8295:
URL: https://github.com/apache/incubator-seata/pull/8295
- [x] I have read the [CONTRIBUTING.md](https://github.com/apache/incubator-seata/blob/2.x/CONTRIBUTING.md) guidelines.
- [ ] I have registered the PR [changes](https://github.com/apache/incubator-seata/tree/2.x/changes).
### Ⅰ. Describe what this PR did
In `server/src/main/resources/logback-spring.xml`, when an external config directory is set via
`spring.config.additional-location` (the `LOG_BASH_DIR` property), the `<else>` branch loaded the
logstash/metric/kafka appender definitions from the jar classpath (`resource="logback/..."`) instead
of the external directory, so the external directory could not override these three appenders.
The code commented out before #7259 used `file="${LOG_BASH_DIR}/logback/..."`, which suggests the
current behavior is unintentional.
This PR restores external-directory loading for the three extended appenders, making the `<else>`
branch consistent with the existing console/file appenders. 3 lines changed.
### Ⅱ. Does this pull request fix one issue?
Related to #4648, supplements #7259.
(Not using "fixes" because #4648 is a broader task; this PR only addresses the leftover issue from #7259.)
### Ⅲ. Why don't you add test cases (unit test/integration test)?
The change is pure Logback XML wiring (3 `include` attributes). The existing `AppenderTest` already
covers appender assembly for the three extended appenders; `include file=` resolution itself is
Logback startup behavior. The standard deployment always exercises this path: the launch script sets
`spring.config.additional-location` to the conf dir and the distribution ships the full `logback/`
directory (see `distribution/release-seata.xml`), so `file=` resolves safely there.
### Ⅳ. Describe how to verify it
1. `logback-spring.xml` still parses as well-formed XML after the change.
2. The two branches are now symmetric: `<then>` (no external dir) loads all five appender XMLs from
the jar via `resource=`; `<else>` (external dir) loads all five via `file="${LOG_BASH_DIR}/logback/..."`,
same as console/file.
3. Manual: copy `conf/logback/` to an external dir, modify the pattern in `kafka-appender.xml`, start
seata-server with `--spring.config.additional-location=file:/path/to/dir/` and
`logging.extend.kafka-appender.enabled=true`; the Kafka appender now uses the modified pattern.
### Ⅴ. Special notes for reviews
The `<then>` branch is intentionally untouched — loading from the jar is correct when no external
directory is configured. This change only aligns the three extended appenders with the external-dir
contract that console/file already follow.
--
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: notifications-unsubscribe@seata.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@seata.apache.org
For additional commands, e-mail: notifications-help@seata.apache.org
Re: [PR] bugfix: load logstash/metric/kafka appenders from external config dir when LOG_BASH_DIR is set [incubator-seata]
Posted by "finalskiller (via GitHub)" <gi...@apache.org>.
finalskiller commented on code in PR #8295:
URL: https://github.com/apache/incubator-seata/pull/8295#discussion_r4398730058
##########
server/src/main/resources/logback-spring.xml:
##########
@@ -74,21 +74,21 @@
<!-- logstash-appender: off by default -->
<if condition='property("LOGSTASH_APPENDER_ENABLED").equals("true")'>
<then>
- <include resource="logback/logstash-appender.xml"/>
+ <include file="${LOG_BASH_DIR}/logback/logstash-appender.xml"/>
Review Comment:
@slievrly I'd like to add some background on why I ended up with the listener + flag approach, in case there is a better way I'm not seeing.
Option 1 — a direct file-exists condition (tried, does not work at runtime)
The natural writing would be:
<if condition='new java.io.File(property("LOG_BASH_DIR") + "/logback/kafka-appender.xml").exists()'>
I first verified it in an isolated Logback 1.3.14 + Janino harness and it worked (external file → then-branch, missing → else-branch). But it fails on the actual runtime version: Spring Boot 4.0.6 resolves Logback 1.5.32 (overriding the 1.3.14 in dependencies/pom.xml), and Logback 1.5 explicitly rejects it — the context fails to start with:
ERROR in IfModelHandler - The 'condition' attribute may not contain the 'new' operator.
See also https://logback.qos.ch/codes.html#conditionNew
A related pitfall I hit while testing: putting `${LOG_BASH_DIR}` directly inside the condition string also breaks on Windows, because the substituted backslashes form invalid Java string escapes and Janino fails to compile the condition (Logback then silently skips the whole `<if>` block).
Option 2 — Logback 1.5 `<condition>` / PropertyCondition (considered, not chosen)
This would move the check into a dedicated Java class, but it is a newer Logback-1.5-only API, and it still requires Java-side logic plus per-appender wiring, so it did not seem clearly better for this case.
Current approach and its known trade-off
So the listener precomputes `*_APPENDER_EXTERNAL` flags and the XML keeps the existing `property(...).equals(...)` style. The weakness I'm aware of: the appender list is maintained in two places (`EXTERNAL_APPENDER_NAMES` in the listener and the includes in logback-spring.xml), and the listener takes on a job that is not really "listening".
An alternative I sketched but did not apply: instead of the hardcoded name array, derive the flags from the actual contents of the external `logback/` directory, so the file system becomes the single source of truth.
Since I could not find a cleaner option that works on the runtime Logback version, I went with the current one. If you feel the direction should be different (e.g. the directory-derived flags, or any existing project pattern I missed), I'm happy to rework it.
--
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: notifications-unsubscribe@seata.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@seata.apache.org
For additional commands, e-mail: notifications-help@seata.apache.org
Re: [PR] bugfix: load logstash/metric/kafka appenders from external config dir when LOG_BASH_DIR is set [incubator-seata]
Posted by "finalskiller (via GitHub)" <gi...@apache.org>.
finalskiller commented on PR #8295:
URL: https://github.com/apache/incubator-seata/pull/8295#issuecomment-5981093618
@YoWuwuuu @xuxiaowei-com-cn This PR supplements #7259, could you please take a look? Thanks!
--
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: notifications-unsubscribe@seata.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@seata.apache.org
For additional commands, e-mail: notifications-help@seata.apache.org
Re: [PR] bugfix: load logstash/metric/kafka appenders from external config dir when LOG_BASH_DIR is set [incubator-seata]
Posted by "xuxiaowei-com-cn (via GitHub)" <gi...@apache.org>.
xuxiaowei-com-cn commented on PR #8295:
URL: https://github.com/apache/incubator-seata/pull/8295#issuecomment-6028786806
1. 请处理现有冲突
2. 建议下列文件使用相同的方法处理
- namingserver/src/main/resources/logback-spring.xml
- namingserver/src/main/resources/logback-spring-jvm.xml
--
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: notifications-unsubscribe@seata.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@seata.apache.org
For additional commands, e-mail: notifications-help@seata.apache.org
Re: [PR] bugfix: load logstash/metric/kafka appenders from external config dir when LOG_BASH_DIR is set [incubator-seata]
Posted by "slievrly (via GitHub)" <gi...@apache.org>.
slievrly commented on code in PR #8295:
URL: https://github.com/apache/incubator-seata/pull/8295#discussion_r4303125360
##########
server/src/main/resources/logback-spring.xml:
##########
@@ -74,21 +74,21 @@
<!-- logstash-appender: off by default -->
<if condition='property("LOGSTASH_APPENDER_ENABLED").equals("true")'>
<then>
- <include resource="logback/logstash-appender.xml"/>
+ <include file="${LOG_BASH_DIR}/logback/logstash-appender.xml"/>
Review Comment:
[P2] Preserve the classpath fallback when an external appender file is absent
An existing deployment can have only `console-appender.xml` and `file-appender.xml` in its external `conf/logback/` directory and enable Logstash, Metric, or Kafka through application properties. Previously, these extended appenders were loaded from the jar. With this change, a missing external XML produces a warning and the corresponding appender is no longer attached, so upgrading can stop log forwarding or metric collection. The same issue applies to lines 84 and 91. Shipping all XML files in the standard distribution does not cover existing partial external configuration directories.
Could we prefer the external file when it exists and otherwise retain the classpath default, with a regression test for a partial external directory? I reproduced the include behavior with Logback 1.3.14: all three appenders attach with the previous resource includes, none attach with the new file includes when those external files are missing, and all attach again after supplying the files. This was an isolated include test, not a full Seata startup. The existing `AppenderTest` does not set `spring.config.additional-location`, so it does not exercise this branch.
--
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: notifications-unsubscribe@seata.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@seata.apache.org
For additional commands, e-mail: notifications-help@seata.apache.org
Re: [PR] bugfix: load logstash/metric/kafka appenders from external config dir when LOG_BASH_DIR is set [incubator-seata]
Posted by "codecov[bot] (via GitHub)" <gi...@apache.org>.
codecov[bot] commented on PR #8295:
URL: https://github.com/apache/incubator-seata/pull/8295#issuecomment-5982047913
## [Codecov](https://app.codecov.io/gh/apache/incubator-seata/pull/8295?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
:white_check_mark: All modified and coverable lines are covered by tests.
:white_check_mark: Project coverage is 73.87%. Comparing base ([`4d7ba2e`](https://app.codecov.io/gh/apache/incubator-seata/commit/4d7ba2edfe51d8ede76ebf0d9965346abd00d824?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)) to head ([`dba45c8`](https://app.codecov.io/gh/apache/incubator-seata/commit/dba45c83694138b1a3dbaa2a5fee053bf90293ad?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)).
:warning: Report is 2 commits behind head on 2.x.
<details><summary>Additional details and impacted files</summary>
```diff
@@ Coverage Diff @@
## 2.x #8295 +/- ##
============================================
- Coverage 73.89% 73.87% -0.03%
- Complexity 1146 1147 +1
============================================
Files 1156 1156
Lines 42396 42396
Branches 5067 5067
============================================
- Hits 31329 31320 -9
- Misses 8578 8587 +9
Partials 2489 2489
```
[see 8 files with indirect coverage changes](https://app.codecov.io/gh/apache/incubator-seata/pull/8295/indirect-changes?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)
[](https://app.codecov.io/gh/apache/incubator-seata/pull/8295?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)
</details>
<details><summary> :rocket: New features to boost your workflow: </summary>
- :snowflake: [Test Analytics](https://docs.codecov.com/docs/test-analytics): Detect flaky tests, report on failures, and find test suite problems.
- :package: [JS Bundle Analysis](https://docs.codecov.com/docs/javascript-bundle-analysis): Save yourself from yourself by tracking and limiting bundle sizes in JS merges.
</details>
--
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: notifications-unsubscribe@seata.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@seata.apache.org
For additional commands, e-mail: notifications-help@seata.apache.org
Re: [PR] bugfix: load logstash/metric/kafka appenders from external config dir when LOG_BASH_DIR is set [incubator-seata]
Posted by "finalskiller (via GitHub)" <gi...@apache.org>.
finalskiller commented on PR #8295:
URL: https://github.com/apache/incubator-seata/pull/8295#issuecomment-6053007720
> 1. 请处理现有冲突
> 2. 建议下列文件使用相同的方法处理
>
> * namingserver/src/main/resources/logback-spring.xml
> * namingserver/src/main/resources/logback-spring-jvm.xml
冲突已处理完毕。关于第 2 点，我对照检查了这两个文件：
1. namingserver/src/main/resources/logback-spring-jvm.xml（53-64 行）：外部配置目录模式下的 <else> 分支，console/file appender 已经使用file="${LOG_BASH_DIR}/logback/..."从外部目录加载，与 #8295 采用的方法一致，无需修改；
2. namingserver/src/main/resources/logback-spring.xml（49、52 行）：它是 GraalVM native image / 直接 java -jar使用的简化版，按设计不包含 <if>（native image 不支持 Janino 运行时编译），全部从 classpath 加载，不存在 <else> 分支，因此也不存在 resource/file 混用的问题；
3. namingserver 没有 logstash/metric/kafka 三个扩展 appender（namingserver/pom.xml中未引入相关依赖），所以"扩展 appender 从外部目录还是从 jar 加载"这个问题在 namingserver 上不存在。
综上，这两个文件当前已与 #8295 的方法保持一致，本次未做修改。如果我理解有偏差，请再具体指出需要调整的位置，我来修改。
--
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: notifications-unsubscribe@seata.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@seata.apache.org
For additional commands, e-mail: notifications-help@seata.apache.org
Re: [PR] bugfix: load logstash/metric/kafka appenders from external config dir when LOG_BASH_DIR is set [incubator-seata]
Posted by "finalskiller (via GitHub)" <gi...@apache.org>.
finalskiller commented on code in PR #8295:
URL: https://github.com/apache/incubator-seata/pull/8295#discussion_r4356628887
##########
server/src/main/resources/logback-spring.xml:
##########
@@ -74,21 +74,21 @@
<!-- logstash-appender: off by default -->
<if condition='property("LOGSTASH_APPENDER_ENABLED").equals("true")'>
<then>
- <include resource="logback/logstash-appender.xml"/>
+ <include file="${LOG_BASH_DIR}/logback/logstash-appender.xml"/>
Review Comment:
@slievrly Thanks for the detailed review. Updated as suggested.
External file preferred, classpath fallback retained
- SystemPropertyLoggerContextListener now checks which appender files exist in the external conf directory and publishes *_APPENDER_EXTERNAL flags into the logback context.
- logback-spring.xml then picks the source accordingly: the external file= when the flag is true, otherwise the classpath resource= copy.
- A direct new java.io.File(...).exists() condition is not possible: Logback 1.5 (resolved through Spring Boot 4.0.6) forbids the `new` operator in <if>, so the flag is computed in the listener and the XML keeps the existing property(...).equals(...) style.
- console / file are unchanged, as they were already loaded from the external directory before this PR.
Regression test for a partial external directory
- AppenderExternalFallbackTest points spring.config.additional-location to a temp directory containing only console-appender.xml and file-appender.xml, enables logstash/kafka/metric, and asserts that all three are still attached through the classpath fallback, while CONSOLE is provably loaded from the external directory (a probe pattern is written into the external copy).
- SystemPropertyLoggerContextListenerTest unit-tests the flag computation (file present / absent / directory not configured).
Verified locally with Logback 1.5.32: AppenderTest, AppenderExternalFallbackTest and SystemPropertyLoggerContextListenerTest pass; spotless and checkstyle are clean.
--
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: notifications-unsubscribe@seata.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@seata.apache.org
For additional commands, e-mail: notifications-help@seata.apache.org