[DUBBO-2988] Unrecognized the other provider #3013

beiwei30 · 2018-12-19T06:22:04Z

What is the purpose of the change

Brief changelog

start to honor both 'group' and 'version'
git rid of relying on Dubbo protocol's exporter, instead, use injvm invoker and application model.

dubbo-config/dubbo-config-api/src/main/java/org/apache/dubbo/config/ReferenceConfig.java
dubbo-demo/dubbo-demo-provider/src/main/java/org/apache/dubbo/demo/provider/ServiceA.java
dubbo-demo/dubbo-demo-provider/src/main/java/org/apache/dubbo/demo/provider/ServiceB.java
dubbo-plugin/dubbo-qos/src/main/java/org/apache/dubbo/qos/command/impl/Ls.java
dubbo-registry/dubbo-registry-api/src/main/java/org/apache/dubbo/registry/support/ProviderConsumerRegTable.java
dubbo-rpc/dubbo-rpc-api/src/main/java/org/apache/dubbo/rpc/model/ConsumerModel.java
dubbo-rpc/dubbo-rpc-dubbo/pom.xml
dubbo-rpc/dubbo-rpc-dubbo/src/main/java/org/apache/dubbo/rpc/protocol/dubbo/telnet/InvokeTelnetHandler.java
dubbo-rpc/dubbo-rpc-dubbo/src/main/java/org/apache/dubbo/rpc/protocol/dubbo/telnet/ListTelnetHandler.java
dubbo-rpc/dubbo-rpc-dubbo/src/test/java/org/apache/dubbo/rpc/protocol/dubbo/ImplicitCallBackTest.java

Verifying this change

XXXXX

Follow this checklist to help us incorporate your contribution quickly and easily:

Make sure there is a GITHUB_issue field for the change (usually before you start working on it). Trivial changes like typos do not require a GITHUB issue. Your pull request should address just this issue, without pulling in other changes - one PR resolves one issue.
Format the pull request title like [Dubbo-XXX] Fix UnknownException when host config not exist #XXX. Each commit in the pull request should have a meaningful subject line and body.
Write a pull request description that is detailed enough to understand what the pull request does, how, and why.
Write necessary unit-test to verify your logic correction, more mock a little better when cross module dependency exist. If the new feature or significant change is committed, please remember to add integration-test in test module.
Run mvn clean install -DskipTests=false & mvn clean test-compile failsafe:integration-test to make sure unit-test and integration-test pass.
If this contribution is large, please follow the Software Donation Guide.

dubbo-config/dubbo-config-api/src/main/java/org/apache/dubbo/config/ReferenceConfig.java

dubbo-config/dubbo-config-api/src/test/java/org/apache/dubbo/config/url/UrlTestBase.java

dubbo-plugin/dubbo-qos/src/main/java/org/apache/dubbo/qos/command/impl/Ls.java

...o-registry-api/src/main/java/org/apache/dubbo/registry/support/ProviderConsumerRegTable.java

codecov-io · 2018-12-19T13:30:27Z

Codecov Report

Merging #3013 into master will increase coverage by 0.01%.
The diff coverage is 70%.

@@            Coverage Diff             @@
##           master    #3013      +/-   ##
==========================================
+ Coverage    64.3%   64.32%   +0.01%     
==========================================
  Files         584      584              
  Lines       26056    26084      +28     
  Branches     4562     4559       -3     
==========================================
+ Hits        16755    16778      +23     
- Misses       7118     7129      +11     
+ Partials     2183     2177       -6

Impacted Files	Coverage Δ
...ain/java/org/apache/dubbo/qos/command/impl/Ls.java	`96% <0%> (+11%)`	⬆️
.../java/org/apache/dubbo/config/ReferenceConfig.java	`56.01% <100%> (ø)`	⬆️
...a/org/apache/dubbo/rpc/model/ApplicationModel.java	`94.11% <100%> (+1.26%)`	⬆️
...o/rpc/protocol/dubbo/telnet/ListTelnetHandler.java	`66.21% <62.5%> (-13.33%)`	⬇️
...java/org/apache/dubbo/rpc/model/ConsumerModel.java	`62.5% <66.66%> (-1.79%)`	⬇️
...rpc/protocol/dubbo/telnet/InvokeTelnetHandler.java	`73.5% <76.31%> (+3.32%)`	⬆️
...bbo/registry/support/ProviderConsumerRegTable.java	`84.44% <87.5%> (+9.44%)`	⬆️
.../apache/dubbo/qos/protocol/QosProtocolWrapper.java	`64.1% <0%> (-17.95%)`	⬇️
...e/dubbo/remoting/transport/netty/NettyChannel.java	`57.64% <0%> (-4.71%)`	⬇️
.../apache/dubbo/remoting/transport/AbstractPeer.java	`58.69% <0%> (-4.35%)`	⬇️
... and 12 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 3e6ae66...1e98e87. Read the comment docs.

dubbo-config/dubbo-config-api/src/test/java/org/apache/dubbo/config/ReferenceConfigTest.java

dubbo-rpc/dubbo-rpc-api/src/main/java/org/apache/dubbo/rpc/model/ApplicationModel.java

khanimteyaz · 2018-12-19T15:50:58Z

dubbo-rpc/dubbo-rpc-api/src/main/java/org/apache/dubbo/rpc/model/ConsumerModel.java

        this.serviceName = serviceName;
+        this.serviceInterfaceClass = serviceInterfaceClass;
        this.proxyObject = proxyObject;

        if (proxyObject != null) {


Is methods and attributes are optional and can be consider my this class only if proxyObject not null? If this is the case then can we have two constructor one with only service and another one with all the parameters?

I am afraid I don't fully understand, could you pls. rephrase your concern?

It is my bad I failed to explain it.
The constructor ConsumerModel takes 5 parameter where methods and attributes are are getting used if and only if **proxyObject ** is not null. So if **proxyObject ** is null then rest of the two methods and attributes are not getting used. So to keep it simple I was saying we can have two constructor

public ConsumerModel(String serviceName, Class<?> serviceInterfaceClass)

public ConsumerModel(String serviceName, Class<?> serviceInterfaceClass, Object proxyObject, Method[] methods, Map<String, Object> attributes)

So using constructor definition it will be easy to understand by the user that object created using constructor 1 does not deal with proxy object, methods and attributes, object populated with these 3 parameter will be null or empty. Where if someone populate object using constructor 2 will be dealing with rest of the parameters and explicitly throw exception of proxyObject is null.

Do let me know if I am able to explain the scenario or whether it make sense or not to consider this.

In fact, if (proxyObject != null) is a defensive programming. In Dubbo, proxyObject should not be null and will not be null.

I guess we should change it to assert or throw IllegelArgumentException, would you mind take care of this in other change?

This change contains too much already, in my opinion.

...-rpc-dubbo/src/main/java/org/apache/dubbo/rpc/protocol/dubbo/telnet/InvokeTelnetHandler.java

...bo-rpc-dubbo/src/main/java/org/apache/dubbo/rpc/protocol/dubbo/telnet/ListTelnetHandler.java

khanimteyaz

There are few changes we can do from readability point of view, which we can do ltr once this PR is merged and for the same I have created an issue #3032.
This changes looks good to me.

beiwei30 · 2018-12-21T14:00:54Z

since there's no further comments, I will merge this pull request. @khanimteyaz you can continue your work based on it.

chickenlj · 2018-12-23T05:45:34Z

...-rpc-dubbo/src/main/java/org/apache/dubbo/rpc/protocol/dubbo/telnet/InvokeTelnetHandler.java

+                    RpcResult result = new RpcResult();
+                    try {
+                        Object o = invokeMethod.invoke(selectedProvider.getServiceInstance(), array);
+                        result.setValue(o);


I doubt that invoking the underlying service instance directly bypassing Dubbo's proxy will make this invoke command meaningless. For a reading service, the result may be qualified as a reflection of that it will behave in production; for a writing service, the side effects may be not predictable

@chickenlj would be possible to explain this in a bit more details (you might have already explained it, but for the shake of mine understanding it would great if you can again)

@khanimteyaz
Hi, I tried to explain it instead of him.

In dubbo, the Provider side exposes a Proxy object instead of the actual ServiceImpl object (such as UserServiceImpl).

If we try to call UserServiceImpl directly, then we actually bypass the proxy class of UserServiceImpl (this proxy class is produced by dubbo).

Telnet is to test the effect of a method. If we bypass the entire proxy and only test UserServiceImpl, then it is meaningless. The effect of this operation is equivalent to Unit Test.

@carryxyh . Got it. Thanks for expplaing it to me. What you saying is make sense and I think we allow to use the actual call path instead of by pass.

@carryxyh I have created an issue 3059 for the same which we can use as a reffrence to work on.

chickenlj · 2018-12-23T05:50:42Z

...bo-rpc-dubbo/src/main/java/org/apache/dubbo/rpc/protocol/dubbo/telnet/ListTelnetHandler.java

-            for (Exporter<?> exporter : DubboProtocol.getDubboProtocol().getExporters()) {
-                if (service.equals(exporter.getInvoker().getInterface().getSimpleName())
-                        || service.equals(exporter.getInvoker().getInterface().getName())
-                        || service.equals(exporter.getInvoker().getUrl().getPath())) {


service.equals(exporter.getInvoker().getUrl().getPath())
This condition will lost in the new implementation while the Model concept used only takes group/service:version into account. I am a little confused about the relation between path and group/service:version at this moment, but I think we should be careful before we can think of them clearly enough.

Yes agree that for changes we should be thought full. In this case what approach we could take to make it better. If you could guide, we would make change to enhance it?

chickenlj · 2018-12-23T05:53:39Z

@beiwei30 @khanimteyaz
Please let me know your thoughts on my review comments above.

#2988 [Dubbo - telnet] Unrecognized the other provider

6c5b3ef

beiwei30 mentioned this pull request Dec 19, 2018

Telnet use Exporter's key to find the Exporter #3007

Closed

6 tasks

remove useless files added by accident.

7545ef3

jasonjoo2010 reviewed Dec 19, 2018

View reviewed changes

dubbo-config/dubbo-config-api/src/main/java/org/apache/dubbo/config/ReferenceConfig.java Outdated Show resolved Hide resolved

beiwei30 added 2 commits December 19, 2018 17:06

fix cycle dependency

8dd623f

fix more cycle dependency

ffbbb59

khanimteyaz reviewed Dec 19, 2018

View reviewed changes

dubbo-config/dubbo-config-api/src/test/java/org/apache/dubbo/config/url/UrlTestBase.java Show resolved Hide resolved

khanimteyaz reviewed Dec 19, 2018

View reviewed changes

dubbo-plugin/dubbo-qos/src/main/java/org/apache/dubbo/qos/command/impl/Ls.java Show resolved Hide resolved

khanimteyaz reviewed Dec 19, 2018

View reviewed changes

...o-registry-api/src/main/java/org/apache/dubbo/registry/support/ProviderConsumerRegTable.java Outdated Show resolved Hide resolved

khanimteyaz reviewed Dec 19, 2018

View reviewed changes

...o-registry-api/src/main/java/org/apache/dubbo/registry/support/ProviderConsumerRegTable.java Show resolved Hide resolved

beiwei30 added 7 commits December 19, 2018 19:50

revert unnecessary change in demo

de28447

make unit test pass

fdd2fba

revert LF to the original CRLF

c9b63f3

change to use lambda

fabf273

fix cycle dependency

b126ff6

remove useless imports

e9c8b43

make unit test stable

14de967

khanimteyaz reviewed Dec 19, 2018

View reviewed changes

dubbo-config/dubbo-config-api/src/test/java/org/apache/dubbo/config/ReferenceConfigTest.java Show resolved Hide resolved

khanimteyaz reviewed Dec 19, 2018

View reviewed changes

dubbo-config/dubbo-config-api/src/test/java/org/apache/dubbo/config/ReferenceConfigTest.java Show resolved Hide resolved

khanimteyaz reviewed Dec 19, 2018

View reviewed changes

dubbo-rpc/dubbo-rpc-api/src/main/java/org/apache/dubbo/rpc/model/ApplicationModel.java Show resolved Hide resolved

khanimteyaz reviewed Dec 19, 2018

View reviewed changes

...-rpc-dubbo/src/main/java/org/apache/dubbo/rpc/protocol/dubbo/telnet/InvokeTelnetHandler.java Outdated Show resolved Hide resolved

khanimteyaz reviewed Dec 19, 2018

View reviewed changes

...-rpc-dubbo/src/main/java/org/apache/dubbo/rpc/protocol/dubbo/telnet/InvokeTelnetHandler.java Outdated Show resolved Hide resolved

make refactor change according to code review comments

8ffbc9b

khanimteyaz reviewed Dec 20, 2018

View reviewed changes

...-rpc-dubbo/src/main/java/org/apache/dubbo/rpc/protocol/dubbo/telnet/InvokeTelnetHandler.java Outdated Show resolved Hide resolved

khanimteyaz reviewed Dec 20, 2018

View reviewed changes

...bo-rpc-dubbo/src/main/java/org/apache/dubbo/rpc/protocol/dubbo/telnet/ListTelnetHandler.java Show resolved Hide resolved

khanimteyaz reviewed Dec 20, 2018

View reviewed changes

...bo-rpc-dubbo/src/main/java/org/apache/dubbo/rpc/protocol/dubbo/telnet/ListTelnetHandler.java Outdated Show resolved Hide resolved

khanimteyaz reviewed Dec 20, 2018

View reviewed changes

...bo-rpc-dubbo/src/main/java/org/apache/dubbo/rpc/protocol/dubbo/telnet/ListTelnetHandler.java Show resolved Hide resolved

khanimteyaz reviewed Dec 20, 2018

View reviewed changes

...bo-rpc-dubbo/src/main/java/org/apache/dubbo/rpc/protocol/dubbo/telnet/ListTelnetHandler.java Outdated Show resolved Hide resolved

khanimteyaz reviewed Dec 20, 2018

View reviewed changes

...bo-rpc-dubbo/src/main/java/org/apache/dubbo/rpc/protocol/dubbo/telnet/ListTelnetHandler.java Show resolved Hide resolved

refactor as review comments suggest.

34959fb

khanimteyaz approved these changes Dec 20, 2018

View reviewed changes

Merge branch 'master' into telnet-group

1e98e87

beiwei30 merged commit 678cdb4 into apache:master Dec 21, 2018

beiwei30 deleted the telnet-group branch December 21, 2018 14:01

chickenlj reviewed Dec 23, 2018

View reviewed changes

khanimteyaz mentioned this pull request Dec 25, 2018

Use the proxy object for InvokeTelnetHandler.java #3059

Closed

2 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[DUBBO-2988] Unrecognized the other provider #3013

[DUBBO-2988] Unrecognized the other provider #3013

beiwei30 commented Dec 19, 2018

codecov-io commented Dec 19, 2018 •

edited

Loading

khanimteyaz Dec 19, 2018

beiwei30 Dec 20, 2018

khanimteyaz Dec 20, 2018 •

edited

Loading

beiwei30 Dec 20, 2018

khanimteyaz Dec 20, 2018

khanimteyaz left a comment

beiwei30 commented Dec 21, 2018

chickenlj Dec 23, 2018

khanimteyaz Dec 24, 2018

carryxyh Dec 25, 2018

khanimteyaz Dec 25, 2018

khanimteyaz Dec 25, 2018

chickenlj Dec 23, 2018

khanimteyaz Dec 24, 2018

chickenlj commented Dec 23, 2018

[DUBBO-2988] Unrecognized the other provider #3013

[DUBBO-2988] Unrecognized the other provider #3013

Conversation

beiwei30 commented Dec 19, 2018

What is the purpose of the change

Brief changelog

Verifying this change

codecov-io commented Dec 19, 2018 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

khanimteyaz Dec 20, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

khanimteyaz left a comment

Choose a reason for hiding this comment

beiwei30 commented Dec 21, 2018

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

Choose a reason for hiding this comment

chickenlj commented Dec 23, 2018

codecov-io commented Dec 19, 2018 •

edited

Loading

khanimteyaz Dec 20, 2018 •

edited

Loading