Feature improvement, only register one url for port unification.

[chickenlj]

Currently, multiple protocol port-unification feature works the following way:

With the following configuration:

dubbo
  protocol
     name: tri
     port: -1
     ext-protocol: rest

It will create two provider urls in the registry:

• rest://demo-service
• tri://demo-service

But with this pull request, there will only have one url in the registry

tri://demo-service?ext.protocol=rest

Consumers can automatically tell the main protocol tri from the ext protocol, those consumers without protocol set consume tri (the main protocol) while others with protocol set consume the protocol specified (for example rest). One consumer instance will always stick to one specific protocol for each specific service based on the protocol setting on its side, so this pull request also addresses the issue that consumers will randomly use all protocols (tri, rest in this case) for different invocations.

For application-based service discovery, the extra protocol url will still be registered to metadata, so the provide side process is not affected, what I do to this model in this patch is only to decide which protocol to use on the consumer side.

[chickenlj]

For those who want multiple protocol support with multiple address URLs should config like below:

dubbo:
  protocols:
    - id: dubbo
       name: dubbo
    - id: rest
       name: rest

[chickenlj]

With this change, the usage of Dubbo can be as simple as this:

    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();
    }

[sonarqubecloud]

Kudos, SonarCloud Quality Gate passed!   

0 Bugs
0 Vulnerabilities
0 Security Hotspots
9 Code Smells

48.9% Coverage
0.0% Duplication

[codecov-commenter]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 68.07%. Comparing base (33a8063) to head (ebe72f6).

❗ Current head ebe72f6 differs from pull request most recent head 8a9c1ed

Please upload reports for the commit 8a9c1ed to get more accurate results.

Additional details and impacted files

@@                    Coverage Diff                     @@
##             3.3.0-beta.1-release   #12528      +/-   ##
==========================================================
+ Coverage                   66.19%   68.07%   +1.88%     
+ Complexity                     20        6      -14     
==========================================================
  Files                        3485     1710    -1775     
  Lines                      141232    70397   -70835     
  Branches                    20338    10220   -10118     
==========================================================
- Hits                        93485    47922   -45563     
+ Misses                      38460    17805   -20655     
+ Partials                     9287     4670    -4617     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[AlbumenJ]

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

[chickenlj]

DefaultApplicationDeployer.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 #12750

[AlbumenJ]

Depends on MetadataService here might not a good design

[chickenlj]

Consider provide a solution for this issue with #12750

[namelessssssssssss]

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

[chickenlj]

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