You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@dubbo.apache.org by "chickenlj (via GitHub)" <gi...@apache.org> on 2023/06/14 07:39:34 UTC

[GitHub] [dubbo] chickenlj opened a new pull request, #12528: Feature improvement, only register one url for port unification.

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

   Currently, [multiple protocol port-unification](https://cn.dubbo.apache.org/zh-cn/overview/tasks/protocols/multi-protocols/) feature works the following way:
   
   With the following configuration:
   
   ```yaml
   dubbo
     protocol
        name: tri
        port: -1
        ext-protocol: rest
   ```
   
   It will crate two provider urls in registry:
   
   * rest://demo-service
   * tri://demo-service
   
   But with this pull request, there will only have one url in registry
   
   `tri://demo-service?ext.protocol=rest`
   
   Consumers can automatically tell the main protocol `tri` from the `ext` protocol,  those without protocol set consume `tri`  while others with protocol set consume `rest`.


-- 
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 #12528: Feature improvement, only register one url for port unification.

Posted by "codecov-commenter (via GitHub)" <gi...@apache.org>.
codecov-commenter commented on PR #12528:
URL: https://github.com/apache/dubbo/pull/12528#issuecomment-1672530983

   ## [Codecov](https://app.codecov.io/gh/apache/dubbo/pull/12528?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
   > Merging [#12528](https://app.codecov.io/gh/apache/dubbo/pull/12528?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) (ebe72f6) into [3.3](https://app.codecov.io/gh/apache/dubbo/commit/7c971ca1bdd1779586faa84b978742a7df881897?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) (7c971ca) will **decrease** coverage by `0.01%`.
   > Report is 3 commits behind head on 3.3.
   > The diff coverage is `n/a`.
   
   ```diff
   @@             Coverage Diff              @@
   ##                3.3   #12528      +/-   ##
   ============================================
   - Coverage     68.08%   68.07%   -0.01%     
     Complexity        6        6              
   ============================================
     Files          1703     1710       +7     
     Lines         70196    70397     +201     
     Branches      10199    10220      +21     
   ============================================
   + Hits          47792    47922     +130     
   - Misses        17752    17805      +53     
   - Partials       4652     4670      +18     
   ```
   
   
   [see 81 files with indirect coverage changes](https://app.codecov.io/gh/apache/dubbo/pull/12528/indirect-changes?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)
   
   :mega: We’re building smart automated test selection to slash your CI/CD build times. [Learn more](https://about.codecov.io/iterative-testing/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)
   


-- 
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] chickenlj commented on a diff in pull request #12528: Feature improvement, only register one url for port unification.

Posted by "chickenlj (via GitHub)" <gi...@apache.org>.
chickenlj commented on code in PR #12528:
URL: https://github.com/apache/dubbo/pull/12528#discussion_r1295541188


##########
dubbo-config/dubbo-config-api/src/main/java/org/apache/dubbo/config/ServiceConfig.java:
##########
@@ -368,7 +370,17 @@ protected void exported() {
                 mapServiceName(url, serviceNameMapping, scheduledExecutor);
             }
         });
+
         onExported();
+
+        if (hasRegistrySpecified()) {
+            getScopeModel().getDeployer().getApplicationDeployer().exportMetadataService();

Review Comment:
   Yes, I think so, this might need some extra efforts to merge this change with Lifecycle mechanism.



-- 
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 #12528: Feature improvement, only register one url for port unification.

Posted by "AlbumenJ (via GitHub)" <gi...@apache.org>.
AlbumenJ commented on code in PR #12528:
URL: https://github.com/apache/dubbo/pull/12528#discussion_r1291321195


##########
dubbo-config/dubbo-config-api/src/main/java/org/apache/dubbo/config/ServiceConfig.java:
##########
@@ -368,7 +370,17 @@ protected void exported() {
                 mapServiceName(url, serviceNameMapping, scheduledExecutor);
             }
         });
+
         onExported();
+
+        if (hasRegistrySpecified()) {
+            getScopeModel().getDeployer().getApplicationDeployer().exportMetadataService();

Review Comment:
   Depends on MetadataService here might not a good design



-- 
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] chickenlj commented on a diff in pull request #12528: Feature improvement, only register one url for port unification.

Posted by "chickenlj (via GitHub)" <gi...@apache.org>.
chickenlj commented on code in PR #12528:
URL: https://github.com/apache/dubbo/pull/12528#discussion_r1292929195


##########
dubbo-config/dubbo-config-api/src/main/java/org/apache/dubbo/config/deploy/DefaultApplicationDeployer.java:
##########
@@ -747,23 +747,27 @@ private void startModules() {
     }
 
     @Override
-    public void prepareApplicationInstance() {
+    public void prepareApplicationInstance(ModuleModel moduleModel) {

Review Comment:
   DefaultApplicationDeployer.java
   ```java
       public void notifyModuleChanged(ModuleModel moduleModel, DeployState state) {
           checkState(moduleModel, state);
       }
   
       public void checkState(ModuleModel moduleModel, DeployState moduleState) {
           synchronized (stateLock) {
               if (!moduleModel.isInternal() && moduleState == DeployState.STARTED) {
                   prepareApplicationInstance(moduleModel);
               }
   }
   ```
   
   It's currently designed to work this way: 
   1. when the state of a ModuleModel changes, it will call `notifyModuleChanged` and passes ModuleModel instance in.
   2. `checkState` will then check if this ModelModel isInternal, if true, `prepareApplicationInstance(moduleModel);` will be called.
   
   ApplicationDeployer depends on ModuleModel mainly querying for statuses such as 'STARTED', 'isInternal', and 'hasRegistryInteraction'. All these statuses can be wrapped into an Event or something, but I think this can be further optimized with this issue https://github.com/apache/dubbo/pull/12750



-- 
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] chickenlj commented on a diff in pull request #12528: Feature improvement, only register one url for port unification.

Posted by "chickenlj (via GitHub)" <gi...@apache.org>.
chickenlj commented on code in PR #12528:
URL: https://github.com/apache/dubbo/pull/12528#discussion_r1293069468


##########
dubbo-config/dubbo-config-api/src/main/java/org/apache/dubbo/config/ServiceConfig.java:
##########
@@ -368,7 +370,17 @@ protected void exported() {
                 mapServiceName(url, serviceNameMapping, scheduledExecutor);
             }
         });
+
         onExported();
+
+        if (hasRegistrySpecified()) {
+            getScopeModel().getDeployer().getApplicationDeployer().exportMetadataService();

Review Comment:
   Consider provide a solution for this issue with https://github.com/apache/dubbo/pull/12750



-- 
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] chickenlj merged pull request #12528: Feature improvement, only register one url for port unification.

Posted by "chickenlj (via GitHub)" <gi...@apache.org>.
chickenlj merged PR #12528:
URL: https://github.com/apache/dubbo/pull/12528


-- 
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] chickenlj commented on a diff in pull request #12528: Feature improvement, only register one url for port unification.

Posted by "chickenlj (via GitHub)" <gi...@apache.org>.
chickenlj commented on code in PR #12528:
URL: https://github.com/apache/dubbo/pull/12528#discussion_r1292929195


##########
dubbo-config/dubbo-config-api/src/main/java/org/apache/dubbo/config/deploy/DefaultApplicationDeployer.java:
##########
@@ -747,23 +747,27 @@ private void startModules() {
     }
 
     @Override
-    public void prepareApplicationInstance() {
+    public void prepareApplicationInstance(ModuleModel moduleModel) {

Review Comment:
   DefaultApplicationDeployer.java
   ```java
       public void notifyModuleChanged(ModuleModel moduleModel, DeployState state) {
           checkState(moduleModel, state);
       }
   
       public void checkState(ModuleModel moduleModel, DeployState moduleState) {
           synchronized (stateLock) {
               if (!moduleModel.isInternal() && moduleState == DeployState.STARTED) {
                   prepareApplicationInstance(moduleModel);
               }
   }
   ```
   
   It's currently designed to work this way: 
   1. when the state of a ModuleModel changes, it will call `notifyModuleChanged`
   2. `checkState` will then check if this ModelModel isInternal, if true, `prepareApplicationInstance(moduleModel);` will be called.
   
   ApplicationDeployer depends on ModuleModel mainly querying for statuses such as 'STARTED', 'isInternal', and 'hasRegistryInteraction'. All these statuses can be wrapped into an Event or something, but I think this can be further optimized with this issue 



-- 
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] chickenlj commented on pull request #12528: Feature improvement, only register one url for port unification.

Posted by "chickenlj (via GitHub)" <gi...@apache.org>.
chickenlj commented on PR #12528:
URL: https://github.com/apache/dubbo/pull/12528#issuecomment-1624553440

   For those who want multiple protocol support with multiple address URLs  should config like below:
   ```yaml
   dubbo:
     protocols:
       - id: dubbo
          name: dubbo
       - id: rest
          name: rest
   ```


-- 
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] chickenlj commented on a diff in pull request #12528: Feature improvement, only register one url for port unification.

Posted by "chickenlj (via GitHub)" <gi...@apache.org>.
chickenlj commented on code in PR #12528:
URL: https://github.com/apache/dubbo/pull/12528#discussion_r1292929195


##########
dubbo-config/dubbo-config-api/src/main/java/org/apache/dubbo/config/deploy/DefaultApplicationDeployer.java:
##########
@@ -747,23 +747,27 @@ private void startModules() {
     }
 
     @Override
-    public void prepareApplicationInstance() {
+    public void prepareApplicationInstance(ModuleModel moduleModel) {

Review Comment:
   DefaultApplicationDeployer.java
   ```java
       public void notifyModuleChanged(ModuleModel moduleModel, DeployState state) {
           checkState(moduleModel, state);
       }
   
       public void checkState(ModuleModel moduleModel, DeployState moduleState) {
           synchronized (stateLock) {
               if (!moduleModel.isInternal() && moduleState == DeployState.STARTED) {
                   prepareApplicationInstance(moduleModel);
               }
   }
   ```
   
   It's currently designed to work this way: 
   1. when the state of a ModuleModel changes, it will call `notifyModuleChanged` and passes ModuleModel instance in.
   2. `checkState` will then check if this ModelModel isInternal, if true, `prepareApplicationInstance(moduleModel);` will be called.
   
   ApplicationDeployer depends on ModuleModel mainly querying for statuses such as 'STARTED', 'isInternal', and 'hasRegistryInteraction'. All these statuses can be wrapped into an Event or something, but I think this can be further optimized with this issue 



-- 
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] sonarcloud[bot] commented on pull request #12528: Feature improvement, only register one url for port unification.

Posted by "sonarcloud[bot] (via GitHub)" <gi...@apache.org>.
sonarcloud[bot] commented on PR #12528:
URL: https://github.com/apache/dubbo/pull/12528#issuecomment-1672527848

   Kudos, SonarCloud Quality Gate passed!&nbsp; &nbsp; [![Quality Gate passed](https://sonarsource.github.io/sonarcloud-github-static-resources/v2/checks/QualityGateBadge/passed-16px.png 'Quality Gate passed')](https://sonarcloud.io/dashboard?id=apache_dubbo&pullRequest=12528)
   
   [![Bug](https://sonarsource.github.io/sonarcloud-github-static-resources/v2/common/bug-16px.png 'Bug')](https://sonarcloud.io/project/issues?id=apache_dubbo&pullRequest=12528&resolved=false&types=BUG) [![A](https://sonarsource.github.io/sonarcloud-github-static-resources/v2/checks/RatingBadge/A-16px.png 'A')](https://sonarcloud.io/project/issues?id=apache_dubbo&pullRequest=12528&resolved=false&types=BUG) [0 Bugs](https://sonarcloud.io/project/issues?id=apache_dubbo&pullRequest=12528&resolved=false&types=BUG)  
   [![Vulnerability](https://sonarsource.github.io/sonarcloud-github-static-resources/v2/common/vulnerability-16px.png 'Vulnerability')](https://sonarcloud.io/project/issues?id=apache_dubbo&pullRequest=12528&resolved=false&types=VULNERABILITY) [![A](https://sonarsource.github.io/sonarcloud-github-static-resources/v2/checks/RatingBadge/A-16px.png 'A')](https://sonarcloud.io/project/issues?id=apache_dubbo&pullRequest=12528&resolved=false&types=VULNERABILITY) [0 Vulnerabilities](https://sonarcloud.io/project/issues?id=apache_dubbo&pullRequest=12528&resolved=false&types=VULNERABILITY)  
   [![Security Hotspot](https://sonarsource.github.io/sonarcloud-github-static-resources/v2/common/security_hotspot-16px.png 'Security Hotspot')](https://sonarcloud.io/project/security_hotspots?id=apache_dubbo&pullRequest=12528&resolved=false&types=SECURITY_HOTSPOT) [![A](https://sonarsource.github.io/sonarcloud-github-static-resources/v2/checks/RatingBadge/A-16px.png 'A')](https://sonarcloud.io/project/security_hotspots?id=apache_dubbo&pullRequest=12528&resolved=false&types=SECURITY_HOTSPOT) [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_dubbo&pullRequest=12528&resolved=false&types=SECURITY_HOTSPOT)  
   [![Code Smell](https://sonarsource.github.io/sonarcloud-github-static-resources/v2/common/code_smell-16px.png 'Code Smell')](https://sonarcloud.io/project/issues?id=apache_dubbo&pullRequest=12528&resolved=false&types=CODE_SMELL) [![A](https://sonarsource.github.io/sonarcloud-github-static-resources/v2/checks/RatingBadge/A-16px.png 'A')](https://sonarcloud.io/project/issues?id=apache_dubbo&pullRequest=12528&resolved=false&types=CODE_SMELL) [9 Code Smells](https://sonarcloud.io/project/issues?id=apache_dubbo&pullRequest=12528&resolved=false&types=CODE_SMELL)
   
   [![48.9%](https://sonarsource.github.io/sonarcloud-github-static-resources/v2/checks/CoverageChart/40-16px.png '48.9%')](https://sonarcloud.io/component_measures?id=apache_dubbo&pullRequest=12528&metric=new_coverage&view=list) [48.9% Coverage](https://sonarcloud.io/component_measures?id=apache_dubbo&pullRequest=12528&metric=new_coverage&view=list)  
   [![0.0%](https://sonarsource.github.io/sonarcloud-github-static-resources/v2/checks/Duplications/3-16px.png '0.0%')](https://sonarcloud.io/component_measures?id=apache_dubbo&pullRequest=12528&metric=new_duplicated_lines_density&view=list) [0.0% Duplication](https://sonarcloud.io/component_measures?id=apache_dubbo&pullRequest=12528&metric=new_duplicated_lines_density&view=list)
   
   


-- 
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] chickenlj commented on pull request #12528: Feature improvement, only register one url for port unification.

Posted by "chickenlj (via GitHub)" <gi...@apache.org>.
chickenlj commented on PR #12528:
URL: https://github.com/apache/dubbo/pull/12528#issuecomment-1672498531

   With this change, the usage of Dubbo can be as simple as this:
   ```java
       private static void main(String[] args) {
           ServiceConfig<DemoService> service = new ServiceConfig<>();
           service.setInterface(DemoService.class);
           service.setRef(new DemoServiceImpl());
   
           DubboBootstrap.getInstance().service(service).start().await();
       }
   
   ```


-- 
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 #12528: Feature improvement, only register one url for port unification.

Posted by "AlbumenJ (via GitHub)" <gi...@apache.org>.
AlbumenJ commented on code in PR #12528:
URL: https://github.com/apache/dubbo/pull/12528#discussion_r1291320040


##########
dubbo-config/dubbo-config-api/src/main/java/org/apache/dubbo/config/deploy/DefaultApplicationDeployer.java:
##########
@@ -747,23 +747,27 @@ private void startModules() {
     }
 
     @Override
-    public void prepareApplicationInstance() {
+    public void prepareApplicationInstance(ModuleModel moduleModel) {

Review Comment:
   ApplicationDeployer depends on ModuleModel might not a good design.
   BTW, if there are more than one ModuleModels, will this work not expected?



-- 
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] namelessssssssssss commented on a diff in pull request #12528: Feature improvement, only register one url for port unification.

Posted by "namelessssssssssss (via GitHub)" <gi...@apache.org>.
namelessssssssssss commented on code in PR #12528:
URL: https://github.com/apache/dubbo/pull/12528#discussion_r1294427252


##########
dubbo-config/dubbo-config-api/src/main/java/org/apache/dubbo/config/ServiceConfig.java:
##########
@@ -368,7 +370,17 @@ protected void exported() {
                 mapServiceName(url, serviceNameMapping, scheduledExecutor);
             }
         });
+
         onExported();
+
+        if (hasRegistrySpecified()) {
+            getScopeModel().getDeployer().getApplicationDeployer().exportMetadataService();

Review Comment:
   It seems that the full service export process can move from ServiceConfig to a new ModuleLifecycle implement? This might solve this problem



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