You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@dubbo.apache.org by GitBox <gi...@apache.org> on 2022/07/22 02:28:45 UTC

[GitHub] [dubbo] BurningCN opened a new pull request, #10354: Fix the problem of partial setXXX method injection of spi class

BurningCN opened a new pull request, #10354:
URL: https://github.com/apache/dubbo/pull/10354

   ## What is the purpose of the change
   
   When spiXXX implements ScopeModelAware, ExtensionAccessorAware, the setXXX of ScopeModelAware and ExtensionAccessorAware does not need to be injected(ExtensionDirectorTest#testInjection can be reproduced)
   
   


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: notifications-unsubscribe@dubbo.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@dubbo.apache.org
For additional commands, e-mail: notifications-help@dubbo.apache.org

[GitHub] [dubbo] AlbumenJ commented on a diff in pull request #10354: Fix the problem of partial setXXX method injection of spi class

Posted by GitBox <gi...@apache.org>.
AlbumenJ commented on code in PR #10354:
URL: https://github.com/apache/dubbo/pull/10354#discussion_r929434372


##########
dubbo-common/src/main/java/org/apache/dubbo/rpc/model/ModuleModel.java:
##########
@@ -61,9 +61,6 @@ public ModuleModel(ApplicationModel applicationModel, boolean isInternal) {
         }
 
         initialize();
-        Assert.notNull(serviceRepository, "ModuleServiceRepository can not be null");
-        Assert.notNull(moduleConfigManager, "ModuleConfigManager can not be null");
-        Assert.assertTrue(moduleConfigManager.isInitialized(), "ModuleConfigManager can not be initialized");

Review Comment:
   It would be better that add this check to `ApplicationModel`



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: notifications-unsubscribe@dubbo.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@dubbo.apache.org
For additional commands, e-mail: notifications-help@dubbo.apache.org

[GitHub] [dubbo] BurningCN commented on a diff in pull request #10354: Fix the problem of partial setXXX method injection of spi class

Posted by GitBox <gi...@apache.org>.
BurningCN commented on code in PR #10354:
URL: https://github.com/apache/dubbo/pull/10354#discussion_r928363651


##########
dubbo-common/src/main/java/org/apache/dubbo/rpc/model/ModuleModel.java:
##########
@@ -61,9 +61,6 @@ public ModuleModel(ApplicationModel applicationModel, boolean isInternal) {
         }
 
         initialize();
-        Assert.notNull(serviceRepository, "ModuleServiceRepository can not be null");
-        Assert.notNull(moduleConfigManager, "ModuleConfigManager can not be null");
-        Assert.assertTrue(moduleConfigManager.isInitialized(), "ModuleConfigManager can not be initialized");

Review Comment:
   Of course, it can be reserved, but I just want to unify it with `ApplicationModule`.(`ApplicationModule` does not check its `configManager` and  `serviceRepository`)



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: notifications-unsubscribe@dubbo.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@dubbo.apache.org
For additional commands, e-mail: notifications-help@dubbo.apache.org

[GitHub] [dubbo] BurningCN commented on a diff in pull request #10354: Fix the problem of partial setXXX method injection of spi class

Posted by GitBox <gi...@apache.org>.
BurningCN commented on code in PR #10354:
URL: https://github.com/apache/dubbo/pull/10354#discussion_r927237977


##########
dubbo-common/src/main/java/org/apache/dubbo/rpc/model/ModuleModel.java:
##########
@@ -76,8 +73,6 @@ public ModuleModel(ApplicationModel applicationModel, boolean isInternal) {
     protected void initialize() {
         super.initialize();
         this.serviceRepository = new ModuleServiceRepository(this);
-        this.moduleConfigManager = new ModuleConfigManager(this);
-        this.moduleConfigManager.initialize();

Review Comment:
   Consistent with `applicationModel`, `ModuleConfigManager` is instantiated and initialized through spi.
   和`applicationModel`保持一致，`ModuleConfigManager`通过spi的方式进行实例化以及初始化。



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: notifications-unsubscribe@dubbo.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@dubbo.apache.org
For additional commands, e-mail: notifications-help@dubbo.apache.org

[GitHub] [dubbo] BurningCN commented on a diff in pull request #10354: Fix the problem of partial setXXX method injection of spi class

Posted by GitBox <gi...@apache.org>.
BurningCN commented on code in PR #10354:
URL: https://github.com/apache/dubbo/pull/10354#discussion_r927244270


##########
dubbo-common/src/main/java/org/apache/dubbo/common/extension/ExtensionLoader.java:
##########
@@ -835,11 +845,23 @@ private T injectExtension(T instance) {
                     continue;
                 }
                 /**
-                 * Check {@link DisableInject} to see if we need auto injection for this property
+                 * Check {@link DisableInject} to see if we need auto-injection for this property
                  */
                 if (method.isAnnotationPresent(DisableInject.class)) {
                     continue;
                 }
+
+                // When spiXXX implements ScopeModelAware, ExtensionAccessorAware,
+                // the setXXX of ScopeModelAware and ExtensionAccessorAware does not need to be injected
+                if (method.getDeclaringClass() == ScopeModelAware.class) {
+                    continue;
+                }
+                if (instance instanceof ScopeModelAware || instance instanceof ExtensionAccessorAware) {
+                    if (ignoredInjectMethodsDesc.contains(ReflectUtils.getDesc(method))) {
+                        continue;
+                    }
+                }
+

Review Comment:
   第一个if的作用是：一个`spi`实现类/接口 `implements ScopeModelAware`，但是没有重写`ScopeModelAware`的相关方法（直接继承`ScopeModelAware`的`setXXX`方法），则满足该if条件，不需要考虑注入，直接`continue`。
   第二个if的作用是：如果`spi`实现类重写或实现了 `ScopeModelAware`或者`ExtensionAccessorAware` 的 `setXXX`方法，也不需要考虑注入，直接`continue`。



##########
dubbo-common/src/main/java/org/apache/dubbo/common/extension/ExtensionLoader.java:
##########
@@ -835,11 +845,23 @@ private T injectExtension(T instance) {
                     continue;
                 }
                 /**
-                 * Check {@link DisableInject} to see if we need auto injection for this property
+                 * Check {@link DisableInject} to see if we need auto-injection for this property
                  */
                 if (method.isAnnotationPresent(DisableInject.class)) {
                     continue;
                 }
+
+                // When spiXXX implements ScopeModelAware, ExtensionAccessorAware,
+                // the setXXX of ScopeModelAware and ExtensionAccessorAware does not need to be injected
+                if (method.getDeclaringClass() == ScopeModelAware.class) {
+                    continue;
+                }
+                if (instance instanceof ScopeModelAware || instance instanceof ExtensionAccessorAware) {
+                    if (ignoredInjectMethodsDesc.contains(ReflectUtils.getDesc(method))) {
+                        continue;
+                    }
+                }
+

Review Comment:
   第一个if的作用是：一个`spi`实现类/接口 `implements ScopeModelAware`，但是没有重写`ScopeModelAware`的相关方法（直接继承`ScopeModelAware`的`setXXX`方法），则满足该if条件，不需要考虑注入，直接`continue`。
   
   第二个if的作用是：如果`spi`实现类重写或实现了 `ScopeModelAware`或者`ExtensionAccessorAware` 的 `setXXX`方法，也不需要考虑注入，直接`continue`。



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: notifications-unsubscribe@dubbo.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@dubbo.apache.org
For additional commands, e-mail: notifications-help@dubbo.apache.org

[GitHub] [dubbo] BurningCN commented on a diff in pull request #10354: Fix the problem of partial setXXX method injection of spi class

Posted by GitBox <gi...@apache.org>.
BurningCN commented on code in PR #10354:
URL: https://github.com/apache/dubbo/pull/10354#discussion_r928362734


##########
dubbo-common/src/main/java/org/apache/dubbo/rpc/model/FrameworkModel.java:
##########
@@ -123,7 +123,6 @@ protected void onDestroy() {
         // notify destroy and clean framework resources
         // see org.apache.dubbo.config.deploy.FrameworkModelCleaner
         notifyDestroy();
-        checkApplicationDestroy();

Review Comment:
   checkApplicationDestroy has been called on line 121
   ![image](https://user-images.githubusercontent.com/43363120/180680575-43055339-0169-4ea6-a700-34a2c312ca77.png)
   



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: notifications-unsubscribe@dubbo.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@dubbo.apache.org
For additional commands, e-mail: notifications-help@dubbo.apache.org

[GitHub] [dubbo] AlbumenJ commented on a diff in pull request #10354: Fix the problem of partial setXXX method injection of spi class

Posted by GitBox <gi...@apache.org>.
AlbumenJ commented on code in PR #10354:
URL: https://github.com/apache/dubbo/pull/10354#discussion_r928361822


##########
dubbo-common/src/main/java/org/apache/dubbo/rpc/model/FrameworkModel.java:
##########
@@ -123,7 +123,6 @@ protected void onDestroy() {
         // notify destroy and clean framework resources
         // see org.apache.dubbo.config.deploy.FrameworkModelCleaner
         notifyDestroy();
-        checkApplicationDestroy();

Review Comment:
   why remove this?



##########
dubbo-common/src/main/java/org/apache/dubbo/rpc/model/ModuleModel.java:
##########
@@ -61,9 +61,6 @@ public ModuleModel(ApplicationModel applicationModel, boolean isInternal) {
         }
 
         initialize();
-        Assert.notNull(serviceRepository, "ModuleServiceRepository can not be null");
-        Assert.notNull(moduleConfigManager, "ModuleConfigManager can not be null");
-        Assert.assertTrue(moduleConfigManager.isInitialized(), "ModuleConfigManager can not be initialized");

Review Comment:
   why remove this line?



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: notifications-unsubscribe@dubbo.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@dubbo.apache.org
For additional commands, e-mail: notifications-help@dubbo.apache.org

[GitHub] [dubbo] codecov-commenter commented on pull request #10354: Fix the problem of partial setXXX method injection of spi class

Posted by GitBox <gi...@apache.org>.
codecov-commenter commented on PR #10354:
URL: https://github.com/apache/dubbo/pull/10354#issuecomment-1192132964

   # [Codecov](https://codecov.io/gh/apache/dubbo/pull/10354?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) Report
   > Merging [#10354](https://codecov.io/gh/apache/dubbo/pull/10354?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (70f772a) into [3.0](https://codecov.io/gh/apache/dubbo/commit/4aacd09ee7342c821b01a0b9898cb1892824a1ff?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (4aacd09) will **decrease** coverage by `0.16%`.
   > The diff coverage is `86.66%`.
   
   > :exclamation: Current head 70f772a differs from pull request most recent head 4032934. Consider uploading reports for the commit 4032934 to get more accurate results
   
   ```diff
   @@             Coverage Diff              @@
   ##                3.0   #10354      +/-   ##
   ============================================
   - Coverage     65.73%   65.57%   -0.17%     
   - Complexity      296      318      +22     
   ============================================
     Files          1233     1233              
     Lines         53715    53720       +5     
     Branches       8115     8120       +5     
   ============================================
   - Hits          35312    35228      -84     
   - Misses        14561    14630      +69     
   - Partials       3842     3862      +20     
   ```
   
   
   | [Impacted Files](https://codecov.io/gh/apache/dubbo/pull/10354?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
   |---|---|---|
   | [...va/org/apache/dubbo/common/config/Environment.java](https://codecov.io/gh/apache/dubbo/pull/10354/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-ZHViYm8tY29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9kdWJiby9jb21tb24vY29uZmlnL0Vudmlyb25tZW50LmphdmE=) | `81.45% <ø> (ø)` | |
   | [.../apache/dubbo/common/config/ModuleEnvironment.java](https://codecov.io/gh/apache/dubbo/pull/10354/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-ZHViYm8tY29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9kdWJiby9jb21tb24vY29uZmlnL01vZHVsZUVudmlyb25tZW50LmphdmE=) | `47.29% <ø> (ø)` | |
   | [...ache/dubbo/config/context/ModuleConfigManager.java](https://codecov.io/gh/apache/dubbo/pull/10354/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-ZHViYm8tY29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9kdWJiby9jb25maWcvY29udGV4dC9Nb2R1bGVDb25maWdNYW5hZ2VyLmphdmE=) | `68.42% <ø> (ø)` | |
   | [...ava/org/apache/dubbo/rpc/model/FrameworkModel.java](https://codecov.io/gh/apache/dubbo/pull/10354/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-ZHViYm8tY29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9kdWJiby9ycGMvbW9kZWwvRnJhbWV3b3JrTW9kZWwuamF2YQ==) | `91.39% <ø> (-0.06%)` | :arrow_down: |
   | [...in/java/org/apache/dubbo/rpc/model/ScopeModel.java](https://codecov.io/gh/apache/dubbo/pull/10354/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-ZHViYm8tY29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9kdWJiby9ycGMvbW9kZWwvU2NvcGVNb2RlbC5qYXZh) | `81.44% <ø> (ø)` | |
   | [...apache/dubbo/common/extension/ExtensionLoader.java](https://codecov.io/gh/apache/dubbo/pull/10354/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-ZHViYm8tY29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9kdWJiby9jb21tb24vZXh0ZW5zaW9uL0V4dGVuc2lvbkxvYWRlci5qYXZh) | `79.05% <81.81%> (+0.04%)` | :arrow_up: |
   | [...n/java/org/apache/dubbo/rpc/model/ModuleModel.java](https://codecov.io/gh/apache/dubbo/pull/10354/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-ZHViYm8tY29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9kdWJiby9ycGMvbW9kZWwvTW9kdWxlTW9kZWwuamF2YQ==) | `94.80% <100.00%> (+1.13%)` | :arrow_up: |
   | [...nfig/spring/extension/SpringExtensionInjector.java](https://codecov.io/gh/apache/dubbo/pull/10354/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-ZHViYm8tY29uZmlnL2R1YmJvLWNvbmZpZy1zcHJpbmcvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL2R1YmJvL2NvbmZpZy9zcHJpbmcvZXh0ZW5zaW9uL1NwcmluZ0V4dGVuc2lvbkluamVjdG9yLmphdmE=) | `84.61% <100.00%> (-1.60%)` | :arrow_down: |
   | [...nt/metadata/ServiceInstanceHostPortCustomizer.java](https://codecov.io/gh/apache/dubbo/pull/10354/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-ZHViYm8tcmVnaXN0cnkvZHViYm8tcmVnaXN0cnktYXBpL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9kdWJiby9yZWdpc3RyeS9jbGllbnQvbWV0YWRhdGEvU2VydmljZUluc3RhbmNlSG9zdFBvcnRDdXN0b21pemVyLmphdmE=) | `65.78% <0.00%> (-21.06%)` | :arrow_down: |
   | [.../apache/dubbo/rpc/filter/ProfilerServerFilter.java](https://codecov.io/gh/apache/dubbo/pull/10354/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-ZHViYm8tcnBjL2R1YmJvLXJwYy1hcGkvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL2R1YmJvL3JwYy9maWx0ZXIvUHJvZmlsZXJTZXJ2ZXJGaWx0ZXIuamF2YQ==) | `62.85% <0.00%> (-20.00%)` | :arrow_down: |
   | ... and [42 more](https://codecov.io/gh/apache/dubbo/pull/10354/diff?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | |
   
   Help us with your feedback. Take ten seconds to tell us [how you rate us](https://about.codecov.io/nps?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
   


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: notifications-unsubscribe@dubbo.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@dubbo.apache.org
For additional commands, e-mail: notifications-help@dubbo.apache.org

[GitHub] [dubbo] BurningCN commented on a diff in pull request #10354: Fix the problem of partial setXXX method injection of spi class

Posted by GitBox <gi...@apache.org>.
BurningCN commented on code in PR #10354:
URL: https://github.com/apache/dubbo/pull/10354#discussion_r929840734


##########
dubbo-common/src/main/java/org/apache/dubbo/rpc/model/ModuleModel.java:
##########
@@ -61,9 +61,6 @@ public ModuleModel(ApplicationModel applicationModel, boolean isInternal) {
         }
 
         initialize();
-        Assert.notNull(serviceRepository, "ModuleServiceRepository can not be null");
-        Assert.notNull(moduleConfigManager, "ModuleConfigManager can not be null");
-        Assert.assertTrue(moduleConfigManager.isInitialized(), "ModuleConfigManager can not be initialized");

Review Comment:
   > It would be better that add this check to `ApplicationModel`
   
   done



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: notifications-unsubscribe@dubbo.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@dubbo.apache.org
For additional commands, e-mail: notifications-help@dubbo.apache.org

[GitHub] [dubbo] AlbumenJ merged pull request #10354: Fix the problem of partial setXXX method injection of spi class

Posted by GitBox <gi...@apache.org>.
AlbumenJ merged PR #10354:
URL: https://github.com/apache/dubbo/pull/10354


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: notifications-unsubscribe@dubbo.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@dubbo.apache.org
For additional commands, e-mail: notifications-help@dubbo.apache.org