-
Notifications
You must be signed in to change notification settings - Fork 8.8k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
optimize: spring boot compatible with file.conf and registry.conf (#6811) #6828
base: 2.x
Are you sure you want to change the base?
Conversation
...java/org/apache/seata/spring/boot/autoconfigure/provider/SeataFileConfigurationProvider.java
Outdated
Show resolved
Hide resolved
...e/src/main/java/org/apache/seata/spring/boot/autoconfigure/loader/SeataPropertiesLoader.java
Show resolved
Hide resolved
...src/test/java/org/apache/seata/spring/boot/autoconfigure/SeataCoreAutoConfigurationTest.java
Outdated
Show resolved
Hide resolved
...e/src/main/java/org/apache/seata/spring/boot/autoconfigure/loader/SeataPropertiesLoader.java
Outdated
Show resolved
Hide resolved
…orElse for setting system properties.
…ation loading test.
…ng from file.conf and registry.conf
if (result == null) { | ||
result = originalConfiguration.getConfig(rawDataId); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is this code being added?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
From an overall perspective, originalConfiguration.getConfig(rawDataId); is used to leverage getLatestConfig to fetch the configuration for rawDataId. After analysis, it indeed functionally covers getConfigFromSys. Therefore, it seems more appropriate to replace:
// 1. Get config value from the system property
result = originalConfiguration.getConfigFromSys(rawDataId);
with:
// 1. Get config value from the system property
result = originalConfiguration.getConfig(rawDataId);
instead of adding:
if (result == null) {
result = originalConfiguration.getConfig(rawDataId);
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Then why still keep originalConfiguration.getConfigFromSys(rawDataId);
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
你目前的代码破坏了原本代码的目的,这段代码是为了先读环境变量和-D参数,然后再尝试读取spring中的配置,而你目前的改动变成了先读环境变量,在读配置中心,再读spring了,而这个SpringBootConfigurationProvider只是为了读取基本配置,配置中心中的配置有CURRENT_FILE_INSTANCE进行读取,不需要通过ORIGIN_FILE_INSTANCE_REGISTRY
也就是SpringBootConfigurationProvider读取
Your current code breaks the original purpose. This section of the code is intended to first read environment variables and -D parameters, and then attempt to read the configuration from Spring. However, with your modification, it now first reads from environment variables, then from the configuration center, and finally from Spring. The SpringBootConfigurationProvider is only meant to read basic configurations, and configurations in the configuration center are read via CURRENT_FILE_INSTANCE, so there’s no need to use ORIGIN_FILE_INSTANCE_REGISTRY, i.e., the SpringBootConfigurationProvider, to read them.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Use SeataApplicationListener to set springConfigurableEnvironment,ensuring it's available during configuration retrieval.
This avoids the need for additional null checks and ensures proper loading timing.
…nvironment calls, addressing the loading timing issue.
Ⅰ. Describe what this PR did
This PR implements a new class
SeataFileConfigurationProvider
in theseata-spring-autoconfigure-core
module. This change ensures that bothseata-spring-boot-starter
andseata-spring-autoconfigure-server
can read legacy configuration files (file.conf
andregistry.conf
), addressing an issue where users upgrading from Spring to Spring Boot would only have their configuration loaded, causing confusion.Ⅱ. Does this pull request fix one issue?
fixed #6811
This PR addresses the issue where
seata-spring-boot-starter
fails to loadfile.conf
andregistry.conf
when users upgrade from Spring to Spring Boot.Ⅲ. Why don't you add test cases (unit test/integration test)?
org.apache.seata.spring.boot.autoconfigure.SeataCoreAutoConfigurationTest
Ⅳ. Describe how to verify it
seata-spring-boot-starter
in a Spring Boot application.file.conf
andregistry.conf
are correctly loaded alongside configurations.seata-spring-boot-starter
andseata-spring-autoconfigure-server
environments.Ⅴ. Special notes for reviews