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

feat: Connect spring cloud properties refresh #4303

Closed
7 changes: 7 additions & 0 deletions apollo-client/pom.xml
Original file line number Diff line number Diff line change
Expand Up @@ -93,5 +93,12 @@
<scope>test</scope>
</dependency>
<!-- end of test -->
<!-- spring cloud -->
<dependency>
<groupId>org.springframework.cloud</groupId>
<artifactId>spring-cloud-context</artifactId>
<scope>provided</scope>
</dependency>
Copy link
Member

@nobodyiam nobodyiam Apr 8, 2022

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.

Suggested change
<dependency>
<groupId>org.springframework.cloud</groupId>
<artifactId>spring-cloud-context</artifactId>
<scope>provided</scope>
</dependency>
<dependency>
<groupId>org.springframework.cloud</groupId>
<artifactId>spring-cloud-context</artifactId>
<optional>true</optional>
</dependency>

Copy link
Contributor Author

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.

fixed

<!-- end of spring cloud -->
</dependencies>
</project>
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,10 @@
import com.ctrip.framework.apollo.spring.config.ConfigPropertySourcesProcessor;
import com.ctrip.framework.apollo.spring.config.PropertySourcesConstants;
import com.ctrip.framework.apollo.spring.config.PropertySourcesProcessor;
import com.ctrip.framework.apollo.spring.config.SpringCloudConfigPropertySourceProcessor;
import org.springframework.boot.autoconfigure.condition.ConditionalOnClass;
import org.springframework.boot.autoconfigure.condition.ConditionalOnMissingBean;
import org.springframework.boot.autoconfigure.condition.ConditionalOnMissingClass;
import org.springframework.boot.autoconfigure.condition.ConditionalOnProperty;
import org.springframework.context.annotation.Bean;
import org.springframework.context.annotation.Configuration;
Expand All @@ -30,7 +33,14 @@
public class ApolloAutoConfiguration {

@Bean
@ConditionalOnMissingClass("org.springframework.cloud.context.scope.refresh.RefreshScope")
public ConfigPropertySourcesProcessor configPropertySourcesProcessor() {
return new ConfigPropertySourcesProcessor();
}

@Bean
@ConditionalOnClass(name = {"org.springframework.cloud.context.scope.refresh.RefreshScope"})
public SpringCloudConfigPropertySourceProcessor springCloudConfigPropertySourceProcessor() {
return new SpringCloudConfigPropertySourceProcessor();
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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));
Copy link
Member

Choose a reason for hiding this comment

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

why do we create a new config change listener for every configPropertySource?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

why do we create a new config change listener for every configPropertySource?

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
Expand Down
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;
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 possible to retrieve the RefreshScope bean here and cache it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Is it possible to retrieve the RefreshScope bean here and cache it?

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();
Copy link
Member

Choose a reason for hiding this comment

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

refreshAll is kind of a dangerous operation, it could cause unexpected behaviors, e.g. eureka service up and down. So I think we need to allow users to customize the change listener behaviors.

There was a config apollo.autoUpdateInjectedSpringProperties to control whether AutoUpdateConfigChangeListener is enabled or not. I think we need to also have two configs to control whether refreshScope.refreshAll and applicationContext.publishEvent are enabled respectively.

With that said, I would suggest we refactor the change listener logic instead of adding a new SpringCloudConfigPropertySourceProcessor

  1. Define a Service Provider Interface, e.g. SpringAutoUpdateConfigChangeListener
  2. Implement different implementations: AutoUpdateConfigChangeListener(the current one), SpringCloudRefreshScopeConfigChangeListener, SpringCloudEnviromentChangeEventConfigChangeListener
  3. Use ServiceBootstrap#loadAllOrdered method to load all the implementations in PropertySourcesProcessor#initializeAutoUpdatePropertiesFeature
  4. Write a CompositeConfigChagneListener to trigger each of the change listener
  5. In the onChange method, each listener will first check whether itself is enabled, then will do the actual work if enabled.

image

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.

Copy link
Contributor Author

@gy09535 gy09535 Apr 8, 2022

Choose a reason for hiding this comment

The 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.

Copy link
Member

Choose a reason for hiding this comment

The 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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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.

Got it.

}
applicationContext.publishEvent(new EnvironmentChangeEvent(changeEvent.changedKeys()));
}
}