Skip to content

Conversation

@xixingya
Copy link
Contributor

@xixingya xixingya commented Aug 7, 2023

What is the purpose of the change

fix #12779

Brief changelog

Verifying this change

Checklist

  • Make sure there is a GitHub_issue field for the change (usually before you start working on it). Trivial changes like typos do not require a GitHub issue. Your pull request should address just this issue, without pulling in other changes - one PR resolves one issue.
  • Each commit in the pull request should have a meaningful subject line and body.
  • Write a pull request description that is detailed enough to understand what the pull request does, how, and why.
  • Check if is necessary to patch to Dubbo 3 if you are work on Dubbo 2.7
  • Write necessary unit-test to verify your logic correction, more mock a little better when cross module dependency exist. If the new feature or significant change is committed, please remember to add sample in dubbo samples project.
  • Add some description to dubbo-website project if you are requesting to add a feature.
  • GitHub Actions works fine on your own branch.
  • If this contribution is large, please follow the Software Donation Guide.

@xixingya
Copy link
Contributor Author

xixingya commented Aug 7, 2023

fix #12779 make this propertis load first so that spring boot default config will not configed

@AlbumenJ
Copy link
Member

AlbumenJ commented Aug 9, 2023

In order to fix #12779, we should remove the hard code of qos-enable=true when parsing spring proeprties.

@xixingya
Copy link
Contributor Author

In order to fix #12779, we should remove the hard code of qos-enable=true when parsing spring proeprties.

But where can we set default true value when user doesn't set qos-enable

@xixingya
Copy link
Contributor Author

In order to fix #12779, we should remove the hard code of qos-enable=true when parsing spring proeprties.

if we let dubbo.properties load before application.yml,the default value will not set

@AlbumenJ
Copy link
Member

In order to fix #12779, we should remove the hard code of qos-enable=true when parsing spring proeprties.

if we let dubbo.properties load before application.yml,the default value will not set

This is a breaking change, and if it can only fix qos-enable is not worthable. We should fix it in application.yml.

@AlbumenJ
Copy link
Member

org.apache.dubbo.spring.boot.env.DubboDefaultPropertiesEnvironmentPostProcessor#createDefaultProperties

@AlbumenJ
Copy link
Member

org.apache.dubbo.spring.boot.env.DubboDefaultPropertiesEnvironmentPostProcessor#createDefaultProperties

Remove setDubboApplicationQosEnableProperty in createDefaultProperties is enough

@xixingya xixingya changed the title make dubbo.properties load first remove spring default qos-enable Aug 14, 2023
@xixingya
Copy link
Contributor Author

@AlbumenJ PTAL

@xixingya
Copy link
Contributor Author

but the other spring default props also not valid in dubbo.properties such as dubbo.config.multiple, because the application.yml is load before dubbo.properties @AlbumenJ

@AlbumenJ
Copy link
Member

image
Please also fix test cases

@codecov-commenter
Copy link

codecov-commenter commented Aug 15, 2023

Codecov Report

Merging #12861 (5c39436) into 3.2 (ceae48e) will decrease coverage by 0.38%.
Report is 6 commits behind head on 3.2.
The diff coverage is n/a.

@@             Coverage Diff              @@
##                3.2   #12861      +/-   ##
============================================
- Coverage     69.82%   69.45%   -0.38%     
+ Complexity      341        2     -339     
============================================
  Files          3523     1648    -1875     
  Lines        167515    68379   -99136     
  Branches      28043     9990   -18053     
============================================
- Hits         116971    47491   -69480     
+ Misses        40459    16299   -24160     
+ Partials      10085     4589    -5496     

see 1933 files with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@AlbumenJ
Copy link
Member

image

@sonarqubecloud
Copy link

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

Copy link
Member

@AlbumenJ AlbumenJ left a comment

Choose a reason for hiding this comment

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

LGTM @chickenlj PTAL

Copy link
Contributor

@chickenlj chickenlj left a comment

Choose a reason for hiding this comment

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

LGTM.

@chickenlj chickenlj merged commit 7290ae5 into apache:3.2 Aug 16, 2023
@strangelookingnerd strangelookingnerd mentioned this pull request Nov 21, 2024
4 tasks
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.

通过外部配置文件的“dubbo.application.qos-enable=false”配置来关闭QOS不生效

4 participants