Module config no support unexport #10380

owen200008 · 2022-07-28T05:36:30Z

unexport add remove config

codecov-commenter · 2022-07-28T06:21:54Z

Codecov Report

Merging #10380 (283fb84) into 3.0 (e3cc859) will decrease coverage by 0.07%.
The diff coverage is 86.95%.

@@             Coverage Diff              @@
##                3.0   #10380      +/-   ##
============================================
- Coverage     65.72%   65.65%   -0.08%     
+ Complexity      319      318       -1     
============================================
  Files          1233     1234       +1     
  Lines         53721    53859     +138     
  Branches       8119     8154      +35     
============================================
+ Hits          35309    35359      +50     
- Misses        14564    14642      +78     
- Partials       3848     3858      +10

Impacted Files	Coverage Δ
...ache/dubbo/config/context/ModuleConfigManager.java	`69.76% <78.57%> (+1.34%)`	⬆️
...he/dubbo/config/context/AbstractConfigManager.java	`76.69% <100.00%> (+2.64%)`	⬆️
...va/org/apache/dubbo/config/spring/ServiceBean.java	`70.73% <100.00%> (+3.16%)`	⬆️
.../apache/dubbo/rpc/filter/ProfilerServerFilter.java	`62.85% <0.00%> (-20.00%)`	⬇️
...bo/rpc/protocol/dubbo/DecodeableRpcInvocation.java	`70.19% <0.00%> (-6.74%)`	⬇️
...e/dubbo/rpc/protocol/tri/transport/WriteQueue.java	`73.80% <0.00%> (-4.77%)`	⬇️
...dubbo/remoting/exchange/support/DefaultFuture.java	`87.93% <0.00%> (-4.32%)`	⬇️
.../apache/dubbo/rpc/protocol/injvm/InjvmInvoker.java	`63.56% <0.00%> (-3.88%)`	⬇️
...he/dubbo/remoting/transport/netty/NettyServer.java	`70.17% <0.00%> (-3.51%)`	⬇️
.../registry/zookeeper/ZookeeperServiceDiscovery.java	`68.35% <0.00%> (-2.54%)`	⬇️
... and 24 more

Help us with your feedback. Take ten seconds to tell us how you rate us.

BurningCN · 2022-07-28T06:55:08Z

dubbo-config/dubbo-config-spring/src/main/java/org/apache/dubbo/config/spring/ServiceBean.java

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


ServiceBean/Config has removed from configManger in here

it only destroy call

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

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

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

config里面是否还要保留着这个ref相关的信息

因为保存了相关的信息那么同样接口的ref就无法再创建了，是否合适

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

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

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

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

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

config里面是否还要保留着这个ref相关的信息

因为保存了相关的信息那么同样接口的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方法中也没问题。

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

AlbumenJ · 2022-08-01T03:31:49Z

dubbo-config/dubbo-config-spring/src/main/java/org/apache/dubbo/config/spring/ServiceBean.java

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


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

AlbumenJ · 2022-08-01T05:03:11Z

还有一个问题是 ServiceBean 的 unexport 方法是谁调用的，理论上用户不会直接使用 ServiceBean 的，一般都是使用的 ReferenceConfig。如果 ServiceBean 的 unexport 是来自框架的调用，那么应该由外面的销毁管理器来统一清理。
这里有一个依赖关系是 ModuleModel => ConfigManager => ReferenceConfig，理论上是不应该由后级直接向前级直接发起调用的（一些特殊场景除外，但是要尽量减少这种场景），否则会很容易出现循环依赖。

owen200008 · 2022-08-01T06:11:59Z

还有一个问题是 ServiceBean 的 unexport 方法是谁调用的，理论上用户不会直接使用 ServiceBean 的，一般都是使用的 ReferenceConfig。如果 ServiceBean 的 unexport 是来自框架的调用，那么应该由外面的销毁管理器来统一清理。这里有一个依赖关系是 ModuleModel => ConfigManager => ReferenceConfig，理论上是不应该由后级直接向前级直接发起调用的（一些特殊场景除外，但是要尽量减少这种场景），否则会很容易出现循环依赖。

我这边要做的就是service和ref的一个热加载的功能，service mesh里面控制面肯定也是需要这块的

我原始是基于ServiceBean做export和unexport来实现的，我这边考虑改成用ServiceConfig去热加载服务，这样可以避开目前这个问题，我这边的实现这里应该是我用法问题

针对ServiceBean的代码增加我看了#8789 #8867 没有完全理解

就是为什么不是加到自己成员的ModuleModel
ModuleModel => ConfigManager => ServiceConfig 理论上是不应该由后级直接向前级直接发起调用的，这个我非常同意，对用户使用的是ServiceConfig和ReferenceConfig，因此难免会有操作上涉及到管理类的变更，更极端一些我可能不希望包含这些类，我只想包含定义变更的接口让对应管理层来实现（类似delegate也可以），这样用于解决内部循环依赖问题
用户使用层不用关心内部实现层次，但是实现层内部本身解决调用链问题

AlbumenJ · 2022-08-01T06:48:56Z

我这边要做的就是service和ref的一个热加载的功能，service mesh里面控制面肯定也是需要这块的

这里在 3.0 的最新版本中，最好是和 ModuleModel 一起来做的，一次热发布绑定到 ModuleModel 的生命周期。目前 Spring Context 也支持绑定到 ModuleModel 上，在 Spring Context 关闭后也会自动关闭 ModuleModel，然后连带清理环境。当然单个的 Config 也是支持独立热发布的。

ModuleModel => ConfigManager => ServiceConfig

这里想到一个方式，是否可以 ServiceConfig 向 ConfigManager 发送初始化或者销毁的通知，由 ConfigManager 来决策是否销毁这个引用。

owen200008 · 2022-08-01T08:55:43Z

我这边要做的就是service和ref的一个热加载的功能，service mesh里面控制面肯定也是需要这块的

这里在 3.0 的最新版本中，最好是和 ModuleModel 一起来做的，一次热发布绑定到 ModuleModel 的生命周期。目前 Spring Context 也支持绑定到 ModuleModel 上，在 Spring Context 关闭后也会自动关闭 ModuleModel，然后连带清理环境。当然单个的 Config 也是支持独立热发布的。

ModuleModel => ConfigManager => ServiceConfig

这里想到一个方式，是否可以 ServiceConfig 向 ConfigManager 发送初始化或者销毁的通知，由 ConfigManager 来决策是否销毁这个引用。

这样是可以的
目前我对注册进ConfigManager的用途没有深入研究过，因为目前在ModuleModel里面的deployer有exportservice这个保存的配置信息已经足够了，后期是打算全部放到ConfigManager，这边都删除掉吗？

AlbumenJ · 2022-08-04T02:52:05Z

多实例化改造之后ModuleModel持有了 ConfigManager 和 ServiceRepository。和配置相关的放在 ConfigManager，和发布后服务相关的放在 ServiceRepositorory。Deployer 直接存储 exportedService 应该是有问题，最后都要收口到 ConfigManager 和 ServiceRepository。

owen200008 added 4 commits July 26, 2022 16:08

unexport and delete config

a8d7c8d

fix no id null bug

43492bc

fix code bug

d221795

service unexport support

283fb84

BurningCN reviewed Jul 28, 2022

View reviewed changes

AlbumenJ requested changes Aug 1, 2022

View reviewed changes

owen200008 closed this Sep 20, 2022

owen200008 deleted the module_config_no_support_unexport branch September 20, 2022 08:33

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Module config no support unexport #10380

Module config no support unexport #10380

owen200008 commented Jul 28, 2022

codecov-commenter commented Jul 28, 2022 •

edited

Loading

BurningCN Jul 28, 2022

owen200008 Jul 28, 2022

owen200008 Jul 28, 2022

BurningCN Jul 28, 2022

owen200008 Jul 28, 2022

owen200008 Jul 28, 2022

BurningCN Jul 28, 2022 •

edited

Loading

owen200008 Jul 28, 2022

BurningCN Jul 28, 2022 •

edited

Loading

AlbumenJ Aug 1, 2022

AlbumenJ Aug 1, 2022

AlbumenJ commented Aug 1, 2022

owen200008 commented Aug 1, 2022 •

edited

Loading

AlbumenJ commented Aug 1, 2022

owen200008 commented Aug 1, 2022

AlbumenJ commented Aug 4, 2022

Module config no support unexport #10380

Module config no support unexport #10380

Conversation

owen200008 commented Jul 28, 2022

codecov-commenter commented Jul 28, 2022 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BurningCN Jul 28, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BurningCN Jul 28, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AlbumenJ commented Aug 1, 2022

owen200008 commented Aug 1, 2022 • edited Loading

AlbumenJ commented Aug 1, 2022

owen200008 commented Aug 1, 2022

AlbumenJ commented Aug 4, 2022

codecov-commenter commented Jul 28, 2022 •

edited

Loading

BurningCN Jul 28, 2022 •

edited

Loading

BurningCN Jul 28, 2022 •

edited

Loading

owen200008 commented Aug 1, 2022 •

edited

Loading