-
-
Notifications
You must be signed in to change notification settings - Fork 10.2k
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
feat: Connect spring cloud properties refresh #4303
Changes from 2 commits
d10f29b
bdd3a6e
2ecea07
c4c7355
e57c5f1
127b41a
ef94892
c9f9f3c
35092b7
0d5a357
08cda42
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -16,6 +16,7 @@ | |
*/ | ||
package com.ctrip.framework.apollo.spring.config; | ||
|
||
import com.ctrip.framework.apollo.ConfigChangeListener; | ||
import com.ctrip.framework.apollo.build.ApolloInjector; | ||
import com.ctrip.framework.apollo.spring.property.AutoUpdateConfigChangeListener; | ||
import com.ctrip.framework.apollo.spring.util.SpringInjector; | ||
|
@@ -139,15 +140,17 @@ private void initializeAutoUpdatePropertiesFeature(ConfigurableListableBeanFacto | |
return; | ||
} | ||
|
||
AutoUpdateConfigChangeListener autoUpdateConfigChangeListener = new AutoUpdateConfigChangeListener( | ||
environment, beanFactory); | ||
|
||
List<ConfigPropertySource> configPropertySources = configPropertySourceFactory.getAllConfigPropertySources(); | ||
for (ConfigPropertySource configPropertySource : configPropertySources) { | ||
configPropertySource.addChangeListener(autoUpdateConfigChangeListener); | ||
configPropertySource.addChangeListener(configChangeListener(this.environment, beanFactory)); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. why do we create a new config change listener for every configPropertySource? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
fixed |
||
} | ||
} | ||
|
||
protected ConfigChangeListener configChangeListener( | ||
Environment environment, ConfigurableListableBeanFactory beanFactory) { | ||
return new AutoUpdateConfigChangeListener(environment, beanFactory); | ||
} | ||
|
||
@Override | ||
public void setEnvironment(Environment environment) { | ||
//it is safe enough to cast as all known environment is derived from ConfigurableEnvironment | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,43 @@ | ||
/* | ||
* Copyright 2022 Apollo Authors | ||
* | ||
* Licensed under the Apache License, Version 2.0 (the "License"); | ||
* you may not use this file except in compliance with the License. | ||
* You may obtain a copy of the License at | ||
* | ||
* http://www.apache.org/licenses/LICENSE-2.0 | ||
* | ||
* Unless required by applicable law or agreed to in writing, software | ||
* distributed under the License is distributed on an "AS IS" BASIS, | ||
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. | ||
* See the License for the specific language governing permissions and | ||
* limitations under the License. | ||
* | ||
*/ | ||
package com.ctrip.framework.apollo.spring.config; | ||
|
||
import com.ctrip.framework.apollo.ConfigChangeListener; | ||
import com.ctrip.framework.apollo.spring.property.SpringCloudAutoUpdateConfigChangeListener; | ||
import org.springframework.beans.BeansException; | ||
import org.springframework.beans.factory.config.ConfigurableListableBeanFactory; | ||
import org.springframework.context.ApplicationContext; | ||
import org.springframework.context.ApplicationContextAware; | ||
import org.springframework.core.env.Environment; | ||
|
||
public class SpringCloudConfigPropertySourceProcessor extends ConfigPropertySourcesProcessor | ||
implements ApplicationContextAware { | ||
|
||
private ApplicationContext applicationContext; | ||
|
||
@Override | ||
protected ConfigChangeListener configChangeListener( | ||
Environment environment, ConfigurableListableBeanFactory beanFactory) { | ||
return new SpringCloudAutoUpdateConfigChangeListener( | ||
environment, beanFactory, applicationContext); | ||
} | ||
|
||
@Override | ||
public void setApplicationContext(ApplicationContext applicationContext) throws BeansException { | ||
this.applicationContext = applicationContext; | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,50 @@ | ||
/* | ||
* Copyright 2022 Apollo Authors | ||
* | ||
* Licensed under the Apache License, Version 2.0 (the "License"); | ||
* you may not use this file except in compliance with the License. | ||
* You may obtain a copy of the License at | ||
* | ||
* http://www.apache.org/licenses/LICENSE-2.0 | ||
* | ||
* Unless required by applicable law or agreed to in writing, software | ||
* distributed under the License is distributed on an "AS IS" BASIS, | ||
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. | ||
* See the License for the specific language governing permissions and | ||
* limitations under the License. | ||
* | ||
*/ | ||
package com.ctrip.framework.apollo.spring.property; | ||
|
||
import com.ctrip.framework.apollo.model.ConfigChangeEvent; | ||
import org.springframework.beans.factory.config.ConfigurableListableBeanFactory; | ||
import org.springframework.cloud.context.environment.EnvironmentChangeEvent; | ||
import org.springframework.cloud.context.scope.refresh.RefreshScope; | ||
import org.springframework.context.ApplicationContext; | ||
import org.springframework.core.env.Environment; | ||
|
||
public class SpringCloudAutoUpdateConfigChangeListener extends AutoUpdateConfigChangeListener { | ||
final ApplicationContext applicationContext; | ||
|
||
public SpringCloudAutoUpdateConfigChangeListener( | ||
Environment environment, | ||
ConfigurableListableBeanFactory beanFactory, | ||
ApplicationContext applicationContext) { | ||
super(environment, beanFactory); | ||
this.applicationContext = applicationContext; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is it possible to retrieve the RefreshScope bean here and cache it? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
fixed |
||
} | ||
|
||
@Override | ||
public void onChange(ConfigChangeEvent changeEvent) { | ||
super.onChange(changeEvent); | ||
adapterSpringCloud(changeEvent); | ||
} | ||
|
||
private void adapterSpringCloud(ConfigChangeEvent changeEvent) { | ||
RefreshScope refreshScope = applicationContext.getBean(RefreshScope.class); | ||
if (refreshScope != null) { | ||
refreshScope.refreshAll(); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
There was a config With that said, I would suggest we refactor the change listener logic instead of adding a new SpringCloudConfigPropertySourceProcessor
The benefit of this architecture is it's easy to add more customized config change listeners in the future. If you like this idea, I think I could contribute to the refactoring part. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah, Sounds good, l prefer it. Maybe we can base this code and create another PR to do this. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I submitted #4305 to enable the spi mechanism, please help to take a look. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Got it. |
||
} | ||
applicationContext.publishEvent(new EnvironmentChangeEvent(changeEvent.changedKeys())); | ||
} | ||
} |
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.
I think
optional
is more appropriate and it's better to move it up with the spring-boot dependencies.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.
fixed