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.

Sign up for GitHub

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

Jump to bottom

Module config no support unexport #10380

Closed
wants to merge 4 commits into from
Closed

Module config no support unexport #10380

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
a8d7c8d
unexport and delete config
owen200008 Jul 26, 2022
43492bc
fix no id null bug
owen200008 Jul 27, 2022
d221795
fix code bug
owen200008 Jul 27, 2022
283fb84
service unexport support
owen200008 Jul 28, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 11 additions & 1 deletion dubbo-common/src/main/java/org/apache/dubbo/config/context/AbstractConfigManager.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -223,6 +223,13 @@ private <C extends AbstractConfig> C addIfAbsent(C config, Map<String, C> config
return config;
}

protected <C extends AbstractConfig> boolean removeIfAbsent(C config, Map<String, C> configsMap) {
if(config.getId() != null) {
return configsMap.remove(config.getId(), config);
}
return configsMap.values().removeIf(c -> config == c);
}

protected boolean isUniqueConfig(AbstractConfig config) {
if (uniqueConfigTypes.contains(config.getClass())) {
return true;
Expand Down Expand Up @@ -651,7 +658,10 @@ public boolean removeConfig(AbstractConfig config) {

Map<String, AbstractConfig> configs = configsCache.get(getTagName(config.getClass()));
if (CollectionUtils.isNotEmptyMap(configs)) {
return configs.values().removeIf(c -> config == c);
// lock by config type
synchronized (configs) {
return removeIfAbsent(config, configs);
}
}
return false;
}
Expand Down
26 changes: 26 additions & 0 deletions dubbo-common/src/main/java/org/apache/dubbo/config/context/ModuleConfigManager.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -199,6 +199,17 @@ protected <C extends AbstractConfig> Optional<C> findDuplicatedConfig(Map<String
return Optional.empty();
}

@Override
protected <C extends AbstractConfig> boolean removeIfAbsent(C config, Map<String, C> configsMap) {
if(super.removeIfAbsent(config, configsMap)) {
if (config instanceof ReferenceConfigBase || config instanceof ServiceConfigBase) {
removeInterfaceConfig((AbstractInterfaceConfig) config);
}
return true;
}
return false;
}

/**
* check duplicated ReferenceConfig/ServiceConfig
*
Expand Down Expand Up @@ -247,6 +258,21 @@ private AbstractInterfaceConfig findDuplicatedInterfaceConfig(AbstractInterfaceC
return prevConfig;
}

private void removeInterfaceConfig(AbstractInterfaceConfig config) {
String uniqueServiceName;
Map<String, AbstractInterfaceConfig> configCache;
if (config instanceof ReferenceConfigBase) {
return;
} else if (config instanceof ServiceConfigBase) {
ServiceConfigBase serviceConfig = (ServiceConfigBase) config;
uniqueServiceName = serviceConfig.getUniqueServiceName();
configCache = serviceConfigCache;
} else {
throw new IllegalArgumentException("Illegal type of parameter 'config' : " + config.getClass().getName());
}
configCache.remove(uniqueServiceName, config);
}

@Override
public void loadConfigs() {
// load dubbo.providers.xxx
Expand Down
7 changes: 7 additions & 0 deletions ...-config/dubbo-config-spring/src/main/java/org/apache/dubbo/config/spring/ServiceBean.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -104,6 +104,13 @@ public void afterPropertiesSet() throws Exception {
moduleModel.getDeployer().setPending();
}

@Override
public void unexport() {
ModuleModel moduleModel = DubboBeanUtils.getModuleModel(applicationContext);
moduleModel.getConfigManager().removeConfig(this);
Copy link
Member

Choose a reason for hiding this comment

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

ServiceBean/Config has removed from configManger in here
image

Copy link
Contributor Author

Choose a reason for hiding this comment

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

it only destroy call

Copy link
Contributor Author

Choose a reason for hiding this comment

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

你可以试下 unexport 然后再export 同样的service

Copy link
Member

Choose a reason for hiding this comment

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

正常就应该是在ModuleModel销毁的时候对configManger做清空操作的。
ServiceConfig的export和unexport内部都不应该对configManager进行添加或者移除操作。

Copy link
Contributor Author

Choose a reason for hiding this comment

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

应该这个词有点太随便了
当一个ref unexport的时候

  1. config里面是否还要保留着这个ref相关的信息
  2. 因为保存了相关的信息那么同样接口的ref就无法再创建了，是否合适

当然这个都是要跟设计有关，但是从来没说过一个service和ref是单例的，只有在destroy才需要销毁，至少2.7.x的service和ref的生命周期是跟unexport相关的

Copy link
Contributor Author

Choose a reason for hiding this comment

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

写这里原因是，加入是bean做的，所以删除也由它管理

Copy link
Member

@BurningCN BurningCN Jul 28, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

可以看如下demo，不是通过ServiceBean的方式，直接是ServiceConfig。
我上面表达的意思是Spring配置方式不是ServiceBean/Config唯一加入configManager的方式。如果按照如下这配置方式，按照目前pr的修改逻辑，在unexport就不会从configManager移除了。

image

Copy link
Contributor Author

Choose a reason for hiding this comment

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

加入和删除一起迁移到父类我觉得是OK的
目前还是保持统一在bean中加入和删除更合适

Copy link
Member

@BurningCN BurningCN Jul 28, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

应该这个词有点太随便了 当一个ref unexport的时候

  1. config里面是否还要保留着这个ref相关的信息
  2. 因为保存了相关的信息那么同样接口的ref就无法再创建了，是否合适

当然这个都是要跟设计有关，但是从来没说过一个service和ref是单例的，只有在destroy才需要销毁，至少2.7.x的service和ref的生命周期是跟unexport相关的

我又想了下，针对你上面提到的。在ModuleDeployer销毁的时候，在每个ServiceConfig#unexport发生之后就立马做了configManager的remove操作（所以你说的12情况都不存在），只是这个时机没有放在unexport里面。而且目前unexport操作目前基本都是在ModuleDeployer销毁时调用的，除非说使用者自己去执行了unexport，才出现你说的情况，但一般使用者不会去这么做。

目前的设计是执行export之前，ServiceConfig已经加入到了configManager，unexport之后才将ServiceConfig从configManager移除，即export和unexport内部都没有关注对configManager的添加或移除操作。

不过按照目前的修改，将ServiceConfig的从configManager的移除逻辑放在unexport方法中也没问题。

Copy link
Member

Choose a reason for hiding this comment

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

这里需要考虑下循环依赖的问题，ModuleModel.destroy -> foreach -> ServiceBean.unexport -> ModuleModel.remove

super.unexport();
}

/**
* Get the name of {@link ServiceBean}
*
Expand Down