You are viewing a plain text version of this content. The canonical link for it is here.

Posted to notifications@dubbo.apache.org by "LiZhenNet (GitHub)" <gi...@apache.org> on 2019/01/11 14:58:14 UTC

[GitHub] [incubator-dubbo] LiZhenNet opened pull request #3210: Refactor telnet invoke command

## What is the purpose of the change

Refactor telnet invoke command ,make it friendly support overridden method .
fix https://github.com/apache/incubator-dubbo/issues/3177
fix https://github.com/apache/incubator-dubbo/issues/3105

## Brief changelog

针对使用 Json 对象调用方法时：
1.当没有相同参数个数的重载方法时，无需指定 “class”。
2. 当有相同参数个数的重载方法时，通过添加 “class” 来明确参数类型。
3. 当有相同参数个数的重载方法时，没有使用 “class” 来明确参数类型，提示用户。

When invoke with Json string:
1. When Invoke method does not have overridden method, it can invoke easily ，like what document recommend.
2. When Invoke method has overridden method, Use "class" key to specify the type inside of command line parameter "-p" .
3. When Invoke method does not have overridden method, prompt the user when parameter does not has "class " key.

## Verifying this change

all unit test pass

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

- [x] Make sure there is a [GITHUB_issue](https://github.com/apache/incubator-dubbo/issues) 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](https://github.com/apache/incubator-dubbo/tree/master/dubbo-test).
- [ ] 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](https://github.com/apache/incubator-dubbo/wiki/Software-donation-guide).

[ Full content available at: https://github.com/apache/incubator-dubbo/pull/3210 ]
This message was relayed via gitbox.apache.org for notifications@dubbo.apache.org

[GitHub] [incubator-dubbo] khanimteyaz commented on pull request #3210: Refactor telnet invoke command

Posted by "khanimteyaz (GitHub)" <gi...@apache.org>.

I think can be rewritten as 

```
selectedProvider=ApplicationModel.allProviderModels()
                .stream()
                .filter(provider->isServiceMatch(service, provider))
                .findFirst().orElse(null);
```

In this case we may need to make **service** as final.  What do you say?

[ Full content available at: https://github.com/apache/incubator-dubbo/pull/3210 ]
This message was relayed via gitbox.apache.org for notifications@dubbo.apache.org

[GitHub] [incubator-dubbo] khanimteyaz commented on pull request #3210: Refactor telnet invoke command

Posted by "khanimteyaz (GitHub)" <gi...@apache.org>.

any specific reason for removing UT ? Are those now become invalid or it is something else ?

[ Full content available at: https://github.com/apache/incubator-dubbo/pull/3210 ]
This message was relayed via gitbox.apache.org for notifications@dubbo.apache.org

[GitHub] [incubator-dubbo] LiZhenNet commented on pull request #3210: Refactor telnet invoke command

Posted by "LiZhenNet (GitHub)" <gi...@apache.org>.

It is invalid

[ Full content available at: https://github.com/apache/incubator-dubbo/pull/3210 ]
This message was relayed via gitbox.apache.org for notifications@dubbo.apache.org

[GitHub] [incubator-dubbo] khanimteyaz commented on pull request #3210: Refactor telnet invoke command

Posted by "khanimteyaz (GitHub)" <gi...@apache.org>.

Looking at the api implementation it seems to be this can become a util method which others can use in future. Would you mind to extract as an util method. Before that would you check whether we can use any of the ClassHelper or ReflectionUtils any method which does the same thing? 
Nice effort of refactoring :)

[ Full content available at: https://github.com/apache/incubator-dubbo/pull/3210 ]
This message was relayed via gitbox.apache.org for notifications@dubbo.apache.org

[GitHub] [incubator-dubbo] khanimteyaz commented on pull request #3210: Refactor telnet invoke command

Posted by "khanimteyaz (GitHub)" <gi...@apache.org>.

Looking at isServiceMatch, it looks it has a or condition where it will return **true** if **service** is null or blank. So is it a expected? @chickenlj @beiwei30 could you have a look at this. Because this is an existing behavior?

[ Full content available at: https://github.com/apache/incubator-dubbo/pull/3210 ]
This message was relayed via gitbox.apache.org for notifications@dubbo.apache.org

[GitHub] [incubator-dubbo] LiZhenNet commented on pull request #3210: Refactor telnet invoke command

Posted by "LiZhenNet (GitHub)" <gi...@apache.org>.

I am not sure if I can use stream api 。

[ Full content available at: https://github.com/apache/incubator-dubbo/pull/3210 ]
This message was relayed via gitbox.apache.org for notifications@dubbo.apache.org

[GitHub] [incubator-dubbo] beiwei30 commented on pull request #3210: Refactor telnet invoke command

Posted by "beiwei30 (GitHub)" <gi...@apache.org>.

use best guess to find out a method candidate to invoke, since service is optional (`[service]`)

[ Full content available at: https://github.com/apache/incubator-dubbo/pull/3210 ]
This message was relayed via gitbox.apache.org for notifications@dubbo.apache.org

[GitHub] [incubator-dubbo] kexianjun commented on issue #3210: Refactor telnet invoke command

Posted by "kexianjun (GitHub)" <gi...@apache.org>.

考虑如下场景：
```
interface interfaceName{
        String methodA(String msg);
        String methodA(MyEnum myEnum);
}
```
```
enum MyEnum{
        LAG;
}
```
使用如下telnet调用：
> invoke interfaceName.methodA("LAG")

应该调用哪个方法，如果没有重载方法的时候，这个调用对String类型的参数以及枚举类型的参数都是可以正常工作的.所以我觉得完全移除-p不太合理的，因为目前-p对于json以及非json，重载方法非重载方法都是可以正常工作的.而且完全移除-p以后像上面的场景就没有地方指定参数类型了.使用-p的时候其实是不用关心是不是重载方法的,只要指定具体的参数类型就可以了.class属性确实可以加上来和2.5.x的telnet调用一样，也符合原来用户的习惯，-p属于新加的可能很多用户不知道，是不是class属性和-p可以共存呢？


[ Full content available at: https://github.com/apache/incubator-dubbo/pull/3210 ]
This message was relayed via gitbox.apache.org for notifications@dubbo.apache.org

[GitHub] [incubator-dubbo] khanimteyaz commented on pull request #3210: Refactor telnet invoke command

Posted by "khanimteyaz (GitHub)" <gi...@apache.org>.

I think 
    `ProviderModel providerModel = new ProviderModel(DemoService.class.getName(), new DemoServiceImpl(), DemoService.class);` would better approach, it will avoid the the problem if class package changes in future. What would you say?

[ Full content available at: https://github.com/apache/incubator-dubbo/pull/3210 ]
This message was relayed via gitbox.apache.org for notifications@dubbo.apache.org

[GitHub] [incubator-dubbo] khanimteyaz commented on pull request #3210: Refactor telnet invoke command

Posted by "khanimteyaz (GitHub)" <gi...@apache.org>.

can be rewritten as 
`return methods.stream.filter(method->method.getName().equals(lookupMethodName) && method.getParameterTypes().length == args.size()).collect(Collectors.toList());`

[ Full content available at: https://github.com/apache/incubator-dubbo/pull/3210 ]
This message was relayed via gitbox.apache.org for notifications@dubbo.apache.org

[GitHub] [incubator-dubbo] khanimteyaz commented on pull request #3210: Refactor telnet invoke command

Posted by "khanimteyaz (GitHub)" <gi...@apache.org>.

should we remove UT in refactore? Are those now become invalid or it is something else ?

[ Full content available at: https://github.com/apache/incubator-dubbo/pull/3210 ]
This message was relayed via gitbox.apache.org for notifications@dubbo.apache.org

[GitHub] [incubator-dubbo] khanimteyaz commented on pull request #3210: Refactor telnet invoke command

Posted by "khanimteyaz (GitHub)" <gi...@apache.org>.

can be rewritten as 
`given(mockChannel.getAttribute("telnet.service")).willReturn(DemoService.class.getName());`

[ Full content available at: https://github.com/apache/incubator-dubbo/pull/3210 ]
This message was relayed via gitbox.apache.org for notifications@dubbo.apache.org

[GitHub] [incubator-dubbo] beiwei30 closed pull request #3210: Refactor telnet invoke command

Posted by "beiwei30 (GitHub)" <gi...@apache.org>.

[ pull request closed by beiwei30 ]

[ Full content available at: https://github.com/apache/incubator-dubbo/pull/3210 ]
This message was relayed via gitbox.apache.org for notifications@dubbo.apache.org

[GitHub] [incubator-dubbo] codecov-io commented on issue #3210: Refactor telnet invoke command

Posted by "codecov-io (GitHub)" <gi...@apache.org>.

# [Codecov](https://codecov.io/gh/apache/incubator-dubbo/pull/3210?src=pr&el=h1) Report
> Merging [#3210](https://codecov.io/gh/apache/incubator-dubbo/pull/3210?src=pr&el=desc) into [master](https://codecov.io/gh/apache/incubator-dubbo/commit/5298afdf401185876edd49b0ad5755f8f618e4cb?src=pr&el=desc) will **decrease** coverage by `0.06%`.
> The diff coverage is `77.08%`.

[![Impacted file tree graph](https://codecov.io/gh/apache/incubator-dubbo/pull/3210/graphs/tree.svg?width=650&token=VnEIkiFQT0&height=150&src=pr)](https://codecov.io/gh/apache/incubator-dubbo/pull/3210?src=pr&el=tree)

```diff
@@             Coverage Diff              @@
##             master    #3210      +/-   ##
============================================
- Coverage     63.75%   63.69%   -0.07%     
  Complexity       75       75              
============================================
  Files           652      653       +1     
  Lines         28200    28218      +18     
  Branches       4781     4784       +3     
============================================
- Hits          17980    17973       -7     
- Misses         7963     7983      +20     
- Partials       2257     2262       +5
```


| [Impacted Files](https://codecov.io/gh/apache/incubator-dubbo/pull/3210?src=pr&el=tree) | Coverage Δ | Complexity Δ | |
|---|---|---|---|
| [...rpc/protocol/dubbo/telnet/InvokeTelnetHandler.java](https://codecov.io/gh/apache/incubator-dubbo/pull/3210/diff?src=pr&el=tree#diff-ZHViYm8tcnBjL2R1YmJvLXJwYy1kdWJiby9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvZHViYm8vcnBjL3Byb3RvY29sL2R1YmJvL3RlbG5ldC9JbnZva2VUZWxuZXRIYW5kbGVyLmphdmE=) | `73.07% <74.39%> (-0.74%)` | `0 <0> (ø)` | |
| [...rpc/protocol/dubbo/telnet/SelectTelnetHandler.java](https://codecov.io/gh/apache/incubator-dubbo/pull/3210/diff?src=pr&el=tree#diff-ZHViYm8tcnBjL2R1YmJvLXJwYy1kdWJiby9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvZHViYm8vcnBjL3Byb3RvY29sL2R1YmJvL3RlbG5ldC9TZWxlY3RUZWxuZXRIYW5kbGVyLmphdmE=) | `92.85% <92.85%> (ø)` | `0 <0> (?)` | |
| [.../apache/dubbo/remoting/transport/AbstractPeer.java](https://codecov.io/gh/apache/incubator-dubbo/pull/3210/diff?src=pr&el=tree#diff-ZHViYm8tcmVtb3RpbmcvZHViYm8tcmVtb3RpbmctYXBpL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9kdWJiby9yZW1vdGluZy90cmFuc3BvcnQvQWJzdHJhY3RQZWVyLmphdmE=) | `58.69% <0%> (-13.05%)` | `0% <0%> (ø)` | |
| [.../remoting/transport/netty4/NettyServerHandler.java](https://codecov.io/gh/apache/incubator-dubbo/pull/3210/diff?src=pr&el=tree#diff-ZHViYm8tcmVtb3RpbmcvZHViYm8tcmVtb3RpbmctbmV0dHk0L3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9kdWJiby9yZW1vdGluZy90cmFuc3BvcnQvbmV0dHk0L05ldHR5U2VydmVySGFuZGxlci5qYXZh) | `73.52% <0%> (-11.77%)` | `0% <0%> (ø)` | |
| [...ng/transport/dispatcher/all/AllChannelHandler.java](https://codecov.io/gh/apache/incubator-dubbo/pull/3210/diff?src=pr&el=tree#diff-ZHViYm8tcmVtb3RpbmcvZHViYm8tcmVtb3RpbmctYXBpL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9kdWJiby9yZW1vdGluZy90cmFuc3BvcnQvZGlzcGF0Y2hlci9hbGwvQWxsQ2hhbm5lbEhhbmRsZXIuamF2YQ==) | `51.42% <0%> (-5.72%)` | `0% <0%> (ø)` | |
| [.../org/apache/dubbo/remoting/ExecutionException.java](https://codecov.io/gh/apache/incubator-dubbo/pull/3210/diff?src=pr&el=tree#diff-ZHViYm8tcmVtb3RpbmcvZHViYm8tcmVtb3RpbmctYXBpL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9kdWJiby9yZW1vdGluZy9FeGVjdXRpb25FeGNlcHRpb24uamF2YQ==) | `15.78% <0%> (-5.27%)` | `0% <0%> (ø)` | |
| [...dubbo/remoting/exchange/support/DefaultFuture.java](https://codecov.io/gh/apache/incubator-dubbo/pull/3210/diff?src=pr&el=tree#diff-ZHViYm8tcmVtb3RpbmcvZHViYm8tcmVtb3RpbmctYXBpL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9kdWJiby9yZW1vdGluZy9leGNoYW5nZS9zdXBwb3J0L0RlZmF1bHRGdXR1cmUuamF2YQ==) | `71.14% <0%> (-4.03%)` | `0% <0%> (ø)` | |
| [...he/dubbo/remoting/transport/netty/NettyServer.java](https://codecov.io/gh/apache/incubator-dubbo/pull/3210/diff?src=pr&el=tree#diff-ZHViYm8tcmVtb3RpbmcvZHViYm8tcmVtb3RpbmctbmV0dHkvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL2R1YmJvL3JlbW90aW5nL3RyYW5zcG9ydC9uZXR0eS9OZXR0eVNlcnZlci5qYXZh) | `67.85% <0%> (-3.58%)` | `0% <0%> (ø)` | |
| [...dubbo/rpc/protocol/dubbo/CallbackServiceCodec.java](https://codecov.io/gh/apache/incubator-dubbo/pull/3210/diff?src=pr&el=tree#diff-ZHViYm8tcnBjL2R1YmJvLXJwYy1kdWJiby9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvZHViYm8vcnBjL3Byb3RvY29sL2R1YmJvL0NhbGxiYWNrU2VydmljZUNvZGVjLmphdmE=) | `76.47% <0%> (-2.95%)` | `0% <0%> (ø)` | |
| [...exchange/support/header/HeaderExchangeHandler.java](https://codecov.io/gh/apache/incubator-dubbo/pull/3210/diff?src=pr&el=tree#diff-ZHViYm8tcmVtb3RpbmcvZHViYm8tcmVtb3RpbmctYXBpL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9kdWJiby9yZW1vdGluZy9leGNoYW5nZS9zdXBwb3J0L2hlYWRlci9IZWFkZXJFeGNoYW5nZUhhbmRsZXIuamF2YQ==) | `55.81% <0%> (-2.33%)` | `0% <0%> (ø)` | |
| ... and [12 more](https://codecov.io/gh/apache/incubator-dubbo/pull/3210/diff?src=pr&el=tree-more) | |

------

[Continue to review full report at Codecov](https://codecov.io/gh/apache/incubator-dubbo/pull/3210?src=pr&el=continue).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/apache/incubator-dubbo/pull/3210?src=pr&el=footer). Last update [5298afd...3ff0fda](https://codecov.io/gh/apache/incubator-dubbo/pull/3210?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).


[ Full content available at: https://github.com/apache/incubator-dubbo/pull/3210 ]
This message was relayed via gitbox.apache.org for notifications@dubbo.apache.org