Skip to content
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

Open
wants to merge 16 commits into
base: 2.x
Choose a base branch
from

Conversation

lyl2008dsg
Copy link
Contributor

@lyl2008dsg lyl2008dsg commented Sep 7, 2024

  • I have registered the PR changes.

Ⅰ. Describe what this PR did

This PR implements a new class SeataFileConfigurationProvider in the seata-spring-autoconfigure-core module. This change ensures that both seata-spring-boot-starter and seata-spring-autoconfigure-server can read legacy configuration files (file.conf and registry.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 load file.conf and registry.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

  1. Use the seata-spring-boot-starter in a Spring Boot application.
  2. Ensure that configurations from file.conf and registry.conf are correctly loaded alongside configurations.
  3. Verify the behavior in both seata-spring-boot-starter and seata-spring-autoconfigure-server environments.

Ⅴ. Special notes for reviews

  • This change is backward-compatible and allows users upgrading from Spring to Spring Boot to continue using their legacy configuration files without disruption.
  • It would be helpful to review the relocation of SeataPropertiesLoader to the seata-spring-autoconfigure-core module, ensuring that legacy configuration files (file.conf and registry.conf) are still properly loaded in both seata-spring-boot-starter and seata-spring-autoconfigure-server environments. Additionally, please verify that this change maintains backward compatibility and ensures a smooth transition for users moving from Spring to Spring Boot.

@lyl2008dsg lyl2008dsg changed the title compatible with file.conf and registry.conf (#6811) spring 吧、oot compatible with file.conf and registry.conf (#6811) Sep 7, 2024
@lyl2008dsg lyl2008dsg changed the title spring 吧、oot compatible with file.conf and registry.conf (#6811) spring boot compatible with file.conf and registry.conf (#6811) Sep 7, 2024
Copy link

codecov bot commented Sep 8, 2024

Codecov Report

Attention: Patch coverage is 25.31646% with 59 lines in your changes missing coverage. Please review.

Project coverage is 52.63%. Comparing base (60a81b1) to head (e4af159).

Files with missing lines Patch % Lines
...n/java/org/apache/seata/common/store/LockMode.java 0.00% 16 Missing ⚠️
...ava/org/apache/seata/common/store/SessionMode.java 0.00% 16 Missing ⚠️
...ot/autoconfigure/loader/SeataPropertiesLoader.java 20.00% 12 Missing ⚠️
...org/apache/seata/server/session/SessionHolder.java 44.44% 4 Missing and 1 partial ⚠️
...toconfigure/listener/SeataApplicationListener.java 71.42% 2 Missing and 2 partials ⚠️
...he/seata/server/cluster/raft/RaftStateMachine.java 33.33% 2 Missing ⚠️
...e/seata/server/coordinator/DefaultCoordinator.java 33.33% 0 Missing and 2 partials ⚠️
...ache/seata/server/coordinator/RaftCoordinator.java 0.00% 1 Missing ⚠️
...apache/seata/server/lock/LockerManagerFactory.java 0.00% 0 Missing and 1 partial ⚠️
Additional details and impacted files

Impacted file tree graph

@@             Coverage Diff              @@
##                2.x    #6828      +/-   ##
============================================
- Coverage     52.65%   52.63%   -0.03%     
- Complexity     6571     6595      +24     
============================================
  Files          1126     1129       +3     
  Lines         39942    39962      +20     
  Branches       4680     4680              
============================================
+ Hits          21031    21032       +1     
- Misses        16911    16928      +17     
- Partials       2000     2002       +2     
Files with missing lines Coverage Δ
...e/seata/server/cluster/raft/RaftServerManager.java 86.13% <100.00%> (ø)
...org/apache/seata/server/session/SessionHelper.java 49.37% <ø> (ø)
...ava/org/apache/seata/server/store/StoreConfig.java 65.78% <ø> (-9.54%) ⬇️
...ache/seata/server/coordinator/RaftCoordinator.java 0.00% <0.00%> (ø)
...apache/seata/server/lock/LockerManagerFactory.java 84.21% <0.00%> (ø)
...he/seata/server/cluster/raft/RaftStateMachine.java 33.03% <33.33%> (ø)
...e/seata/server/coordinator/DefaultCoordinator.java 46.05% <33.33%> (+0.31%) ⬆️
...toconfigure/listener/SeataApplicationListener.java 71.42% <71.42%> (ø)
...org/apache/seata/server/session/SessionHolder.java 53.94% <44.44%> (+1.31%) ⬆️
...ot/autoconfigure/loader/SeataPropertiesLoader.java 62.85% <20.00%> (ø)
... and 2 more

... and 8 files with indirect coverage changes

@funky-eyes funky-eyes changed the title spring boot compatible with file.conf and registry.conf (#6811) optimize: spring boot compatible with file.conf and registry.conf (#6811) Sep 9, 2024
@funky-eyes funky-eyes added this to the 2.3.0 milestone Sep 24, 2024
@lyl2008dsg lyl2008dsg closed this Oct 13, 2024
@lyl2008dsg lyl2008dsg reopened this Oct 13, 2024
@lyl2008dsg lyl2008dsg closed this Oct 13, 2024
@lyl2008dsg lyl2008dsg reopened this Oct 13, 2024
Comment on lines 71 to 73
if (result == null) {
result = originalConfiguration.getConfig(rawDataId);
}
Copy link
Contributor

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?

Copy link
Contributor Author

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); 
}

Copy link
Contributor

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);?

Copy link
Contributor

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.

Copy link
Contributor Author

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.

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

Successfully merging this pull request may close these issues.

seata-spring-boot-starter compatible with file.conf and registry.conf configurations
3 participants