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

Lazy load ConfigUtil #3864

Merged
merged 1 commit into from
Aug 4, 2021
Merged

Lazy load ConfigUtil #3864

merged 1 commit into from
Aug 4, 2021

Conversation

lonre
Copy link
Contributor

@lonre lonre commented Jul 31, 2021

prevent ConfigUtil init when ApolloApplicationContextInitializer initialized,
That is too early.

This issue caused by commit 6d50ca8

What's the purpose of this PR

XXXXX

Which issue(s) this PR fixes:

Fixes #

Brief changelog

XXXXX

Follow this checklist to help us incorporate your contribution quickly and easily:

  • Read the Contributing Guide before making this pull request.
  • Write a pull request description that is detailed enough to understand what the pull request does, how, and why.
  • Write necessary unit tests to verify the code.
  • Run mvn clean test to make sure this pull request doesn't break anything.
  • Update the CHANGES log.

prevent ConfigUtil init when ApolloApplicationContextInitializer initialized,
That is too early.

This issue caused by commit 6d50ca8
@codecov-commenter
Copy link

Codecov Report

Merging #3864 (0d6c29b) into master (02c43dd) will decrease coverage by 0.04%.
The diff coverage is 100.00%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master    #3864      +/-   ##
============================================
- Coverage     50.18%   50.14%   -0.05%     
+ Complexity     2468     2464       -4     
============================================
  Files           483      483              
  Lines         14921    14921              
  Branches       1520     1520              
============================================
- Hits           7488     7482       -6     
- Misses         6908     6910       +2     
- Partials        525      529       +4     
Impacted Files Coverage Δ
...ring/boot/ApolloApplicationContextInitializer.java 94.33% <100.00%> (ø)
...apollo/spring/config/PropertySourcesProcessor.java 92.72% <100.00%> (ø)
...rk/apollo/spring/property/SpringValueRegistry.java 83.33% <0.00%> (-5.56%) ⬇️
...nfigservice/service/AccessKeyServiceWithCache.java 82.65% <0.00%> (-2.05%) ⬇️
.../framework/apollo/spring/property/SpringValue.java 87.71% <0.00%> (-1.76%) ⬇️
.../apollo/internals/RemoteConfigLongPollService.java 77.71% <0.00%> (-0.61%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 02c43dd...0d6c29b. Read the comment docs.

@lonre
Copy link
Contributor Author

lonre commented Jul 31, 2021

since #3800 introduced,

Spring Boot project start failed with

18:53:31.374 [main] WARN com.ctrip.framework.foundation.internals.provider.DefaultApplicationProvider - app.id is not available from System Property and /META-INF/app.properties. It is set to null
18:53:31.386 [main] INFO com.ctrip.framework.foundation.internals.provider.DefaultServerProvider - Environment is set to null. Because it is not available in either (1) JVM system property 'env', (2) OS env variable 'ENV' nor (3) property 'env' from the properties InputStream.
18:53:31.386 [main] DEBUG com.ctrip.framework.foundation.internals.provider.DefaultServerProvider - Data Center is set to null. Because it is not available in either (1) JVM system property 'idc', (2) OS env variable 'IDC' nor (3) property 'idc' from the properties InputStream.
2021-07-31 18:53:32.939  WARN [,,] 4429 --- [           main] c.c.f.a.internals.ConfigServiceLocator   : Located config services from apollo.config-service configuration: http://xxx.xxx.xxx.xxx, will not refresh config services from remote meta service!
2021-07-31 18:53:32.940  WARN [,,] 4429 --- [           main] c.c.f.a.i.LocalFileConfigRepository      : Sync config from upstream repository class com.ctrip.framework.apollo.internals.RemoteConfigRepository failed, reason: Load Apollo Config failed - appId: ApolloNoAppIdPlaceHolder, cluster: default, namespace: application, url: http://xxx.xxx.xxx.xxx/configs/ApolloNoAppIdPlaceHolder/default/application?ip=0.0.0.0 [Cause: [status code: 404] Could not find config for namespace - appId: ApolloNoAppIdPlaceHolder, cluster: default, namespace: application, please check whether the configs are released in Apollo!]
2021-07-31 18:53:32.940  WARN [,,] 4429 --- [           main] c.c.f.a.i.LocalFileConfigRepository      : Sync config from upstream repository class com.ctrip.framework.apollo.internals.RemoteConfigRepository failed, reason: Load Apollo Config failed - appId: ApolloNoAppIdPlaceHolder, cluster: default, namespace: application, url: http://xxx.xxx.xxx.xxx/configs/ApolloNoAppIdPlaceHolder/default/application?ip=0.0.0.0 [Cause: [status code: 404] Could not find config for namespace - appId: ApolloNoAppIdPlaceHolder, cluster: default, namespace: application, please check whether the configs are released in Apollo!]

@@ -62,7 +62,7 @@

private final ConfigPropertySourceFactory configPropertySourceFactory = SpringInjector
.getInstance(ConfigPropertySourceFactory.class);
private final ConfigUtil configUtil = ApolloInjector.getInstance(ConfigUtil.class);
private ConfigUtil configUtil;
Copy link
Member

Choose a reason for hiding this comment

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

Is it necessary to change this logic too? I noticed this line of code was introduced 3 years ago..

Copy link
Contributor Author

Choose a reason for hiding this comment

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

currently it's ok if we keep the origin logic, but i thinks it's more safe if we will make this change

Copy link
Member

@nobodyiam nobodyiam left a comment

Choose a reason for hiding this comment

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

LGTM

@nobodyiam nobodyiam merged commit bfdfd98 into apolloconfig:master Aug 4, 2021
@github-actions github-actions bot locked and limited conversation to collaborators Aug 4, 2021
@nobodyiam nobodyiam added this to the 1.9.0 milestone Aug 7, 2021
@lonre lonre deleted the fix branch April 11, 2022 14:52
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants