You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@dubbo.apache.org by "Leishunyu (GitHub)" <gi...@apache.org> on 2018/12/07 06:38:06 UTC

[GitHub] [incubator-dubbo] Leishunyu opened pull request #2909: create AbstractRouter

## What is the purpose of the change
https://github.com/apache/incubator-dubbo/issues/2799
change the jdk compatible

## Brief changelog


## Verifying this change


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` & `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/2909 ]
This message was relayed via gitbox.apache.org for notifications@dubbo.apache.org

[GitHub] [incubator-dubbo] Leishunyu commented on pull request #2909: create AbstractRouter

Posted by "Leishunyu (GitHub)" <gi...@apache.org>.
Ok, modified, thank you


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

[GitHub] [incubator-dubbo] codecov-io commented on issue #2909: create AbstractRouter

Posted by "codecov-io (GitHub)" <gi...@apache.org>.
# [Codecov](https://codecov.io/gh/apache/incubator-dubbo/pull/2909?src=pr&el=h1) Report
> Merging [#2909](https://codecov.io/gh/apache/incubator-dubbo/pull/2909?src=pr&el=desc) into [master](https://codecov.io/gh/apache/incubator-dubbo/commit/ea71adbf2a9526e3091b5c15d59e5e4b8872ca81?src=pr&el=desc) will **increase** coverage by `0.03%`.
> The diff coverage is `5%`.

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

```diff
@@            Coverage Diff             @@
##           master    #2909      +/-   ##
==========================================
+ Coverage   63.74%   63.77%   +0.03%     
==========================================
  Files         577      578       +1     
  Lines       25926    25933       +7     
  Branches     4540     4543       +3     
==========================================
+ Hits        16526    16540      +14     
+ Misses       7230     7223       -7     
  Partials     2170     2170
```


| [Impacted Files](https://codecov.io/gh/apache/incubator-dubbo/pull/2909?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [...apache/dubbo/rpc/cluster/router/tag/TagRouter.java](https://codecov.io/gh/apache/incubator-dubbo/pull/2909/diff?src=pr&el=tree#diff-ZHViYm8tY2x1c3Rlci9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvZHViYm8vcnBjL2NsdXN0ZXIvcm91dGVyL3RhZy9UYWdSb3V0ZXIuamF2YQ==) | `71.87% <0%> (+6.16%)` | :arrow_up: |
| [...main/java/org/apache/dubbo/rpc/cluster/Router.java](https://codecov.io/gh/apache/incubator-dubbo/pull/2909/diff?src=pr&el=tree#diff-ZHViYm8tY2x1c3Rlci9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvZHViYm8vcnBjL2NsdXN0ZXIvUm91dGVyLmphdmE=) | `0% <0%> (ø)` | |
| [...dubbo/rpc/cluster/router/MockInvokersSelector.java](https://codecov.io/gh/apache/incubator-dubbo/pull/2909/diff?src=pr&el=tree#diff-ZHViYm8tY2x1c3Rlci9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvZHViYm8vcnBjL2NsdXN0ZXIvcm91dGVyL01vY2tJbnZva2Vyc1NlbGVjdG9yLmphdmE=) | `91.42% <0%> (-2.69%)` | :arrow_down: |
| [.../dubbo/rpc/cluster/router/script/ScriptRouter.java](https://codecov.io/gh/apache/incubator-dubbo/pull/2909/diff?src=pr&el=tree#diff-ZHViYm8tY2x1c3Rlci9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvZHViYm8vcnBjL2NsdXN0ZXIvcm91dGVyL3NjcmlwdC9TY3JpcHRSb3V0ZXIuamF2YQ==) | `59.18% <0%> (-6.73%)` | :arrow_down: |
| [.../rpc/cluster/router/condition/ConditionRouter.java](https://codecov.io/gh/apache/incubator-dubbo/pull/2909/diff?src=pr&el=tree#diff-ZHViYm8tY2x1c3Rlci9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvZHViYm8vcnBjL2NsdXN0ZXIvcm91dGVyL2NvbmRpdGlvbi9Db25kaXRpb25Sb3V0ZXIuamF2YQ==) | `73.28% <50%> (+1.64%)` | :arrow_up: |
| [...ache/dubbo/remoting/transport/AbstractChannel.java](https://codecov.io/gh/apache/incubator-dubbo/pull/2909/diff?src=pr&el=tree#diff-ZHViYm8tcmVtb3RpbmcvZHViYm8tcmVtb3RpbmctYXBpL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9kdWJiby9yZW1vdGluZy90cmFuc3BvcnQvQWJzdHJhY3RDaGFubmVsLmphdmE=) | `75% <0%> (-12.5%)` | :arrow_down: |
| [...ache/dubbo/remoting/p2p/support/AbstractGroup.java](https://codecov.io/gh/apache/incubator-dubbo/pull/2909/diff?src=pr&el=tree#diff-ZHViYm8tcmVtb3RpbmcvZHViYm8tcmVtb3RpbmctcDJwL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9kdWJiby9yZW1vdGluZy9wMnAvc3VwcG9ydC9BYnN0cmFjdEdyb3VwLmphdmE=) | `45.45% <0%> (-11.37%)` | :arrow_down: |
| [...ng/transport/dispatcher/all/AllChannelHandler.java](https://codecov.io/gh/apache/incubator-dubbo/pull/2909/diff?src=pr&el=tree#diff-ZHViYm8tcmVtb3RpbmcvZHViYm8tcmVtb3RpbmctYXBpL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9kdWJiby9yZW1vdGluZy90cmFuc3BvcnQvZGlzcGF0Y2hlci9hbGwvQWxsQ2hhbm5lbEhhbmRsZXIuamF2YQ==) | `51.42% <0%> (-5.72%)` | :arrow_down: |
| [...ng/exchange/support/header/HeartbeatTimerTask.java](https://codecov.io/gh/apache/incubator-dubbo/pull/2909/diff?src=pr&el=tree#diff-ZHViYm8tcmVtb3RpbmcvZHViYm8tcmVtb3RpbmctYXBpL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9kdWJiby9yZW1vdGluZy9leGNoYW5nZS9zdXBwb3J0L2hlYWRlci9IZWFydGJlYXRUaW1lclRhc2suamF2YQ==) | `73.68% <0%> (-5.27%)` | :arrow_down: |
| [...rg/apache/dubbo/common/timer/HashedWheelTimer.java](https://codecov.io/gh/apache/incubator-dubbo/pull/2909/diff?src=pr&el=tree#diff-ZHViYm8tY29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9kdWJiby9jb21tb24vdGltZXIvSGFzaGVkV2hlZWxUaW1lci5qYXZh) | `59.93% <0%> (-0.7%)` | :arrow_down: |
| ... and [11 more](https://codecov.io/gh/apache/incubator-dubbo/pull/2909/diff?src=pr&el=tree-more) | |

------

[Continue to review full report at Codecov](https://codecov.io/gh/apache/incubator-dubbo/pull/2909?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/2909?src=pr&el=footer). Last update [ea71adb...6aedf05](https://codecov.io/gh/apache/incubator-dubbo/pull/2909?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/2909 ]
This message was relayed via gitbox.apache.org for notifications@dubbo.apache.org

[GitHub] [incubator-dubbo] carryxyh commented on issue #2909: create AbstractRouter

Posted by "carryxyh (GitHub)" <gi...@apache.org>.
@ralf0131 
Would u pls have a look?

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

[GitHub] [incubator-dubbo] Leishunyu commented on issue #2909: create AbstractRouter

Posted by "Leishunyu (GitHub)" <gi...@apache.org>.
> 如果它通过CI，我将合并它。

3Q!

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

[GitHub] [incubator-dubbo] carryxyh commented on pull request #2909: create AbstractRouter

Posted by "carryxyh (GitHub)" <gi...@apache.org>.
This does not match the old logic.
The old logic is: If the priorities are equal, compare the rules.

If you call super first and the priority is equal, continue to compare the url. If the url returns not 0, it will return directly.

Old logic doesn't compare to url and url would not effect result.

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

[GitHub] [incubator-dubbo] beiwei30 commented on issue #2909: create AbstractRouter

Posted by "beiwei30 (GitHub)" <gi...@apache.org>.
I will merge it if it passes CI.

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

[GitHub] [incubator-dubbo] carryxyh commented on pull request #2909: create AbstractRouter

Posted by "carryxyh (GitHub)" <gi...@apache.org>.
Simply,
Sometimes the url compare(super method) return 1. But the rule compare return -1.
If we call super first, it will return 1.
But in the old logic, it will return -1.

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

[GitHub] [incubator-dubbo] beiwei30 closed pull request #2909: create AbstractRouter

Posted by "beiwei30 (GitHub)" <gi...@apache.org>.
[ pull request closed by beiwei30 ]

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

[GitHub] [incubator-dubbo] codecov-io commented on issue #2909: create AbstractRouter

Posted by "codecov-io (GitHub)" <gi...@apache.org>.
# [Codecov](https://codecov.io/gh/apache/incubator-dubbo/pull/2909?src=pr&el=h1) Report
> Merging [#2909](https://codecov.io/gh/apache/incubator-dubbo/pull/2909?src=pr&el=desc) into [master](https://codecov.io/gh/apache/incubator-dubbo/commit/ea71adbf2a9526e3091b5c15d59e5e4b8872ca81?src=pr&el=desc) will **decrease** coverage by `0.03%`.
> The diff coverage is `5%`.

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

```diff
@@            Coverage Diff            @@
##           master   #2909      +/-   ##
=========================================
- Coverage   63.74%   63.7%   -0.04%     
=========================================
  Files         577     578       +1     
  Lines       25926   25928       +2     
  Branches     4540    4539       -1     
=========================================
- Hits        16526   16517       -9     
- Misses       7230    7244      +14     
+ Partials     2170    2167       -3
```


| [Impacted Files](https://codecov.io/gh/apache/incubator-dubbo/pull/2909?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [...apache/dubbo/rpc/cluster/router/tag/TagRouter.java](https://codecov.io/gh/apache/incubator-dubbo/pull/2909/diff?src=pr&el=tree#diff-ZHViYm8tY2x1c3Rlci9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvZHViYm8vcnBjL2NsdXN0ZXIvcm91dGVyL3RhZy9UYWdSb3V0ZXIuamF2YQ==) | `71.87% <0%> (+6.16%)` | :arrow_up: |
| [...main/java/org/apache/dubbo/rpc/cluster/Router.java](https://codecov.io/gh/apache/incubator-dubbo/pull/2909/diff?src=pr&el=tree#diff-ZHViYm8tY2x1c3Rlci9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvZHViYm8vcnBjL2NsdXN0ZXIvUm91dGVyLmphdmE=) | `0% <0%> (ø)` | |
| [...dubbo/rpc/cluster/router/MockInvokersSelector.java](https://codecov.io/gh/apache/incubator-dubbo/pull/2909/diff?src=pr&el=tree#diff-ZHViYm8tY2x1c3Rlci9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvZHViYm8vcnBjL2NsdXN0ZXIvcm91dGVyL01vY2tJbnZva2Vyc1NlbGVjdG9yLmphdmE=) | `91.42% <0%> (-2.69%)` | :arrow_down: |
| [.../dubbo/rpc/cluster/router/script/ScriptRouter.java](https://codecov.io/gh/apache/incubator-dubbo/pull/2909/diff?src=pr&el=tree#diff-ZHViYm8tY2x1c3Rlci9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvZHViYm8vcnBjL2NsdXN0ZXIvcm91dGVyL3NjcmlwdC9TY3JpcHRSb3V0ZXIuamF2YQ==) | `59.18% <0%> (-6.73%)` | :arrow_down: |
| [.../rpc/cluster/router/condition/ConditionRouter.java](https://codecov.io/gh/apache/incubator-dubbo/pull/2909/diff?src=pr&el=tree#diff-ZHViYm8tY2x1c3Rlci9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvZHViYm8vcnBjL2NsdXN0ZXIvcm91dGVyL2NvbmRpdGlvbi9Db25kaXRpb25Sb3V0ZXIuamF2YQ==) | `73.28% <50%> (+1.64%)` | :arrow_up: |
| [...onfig/spring/extension/SpringExtensionFactory.java](https://codecov.io/gh/apache/incubator-dubbo/pull/2909/diff?src=pr&el=tree#diff-ZHViYm8tY29uZmlnL2R1YmJvLWNvbmZpZy1zcHJpbmcvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL2R1YmJvL2NvbmZpZy9zcHJpbmcvZXh0ZW5zaW9uL1NwcmluZ0V4dGVuc2lvbkZhY3RvcnkuamF2YQ==) | `69.23% <0%> (-15.39%)` | :arrow_down: |
| [...ache/dubbo/remoting/p2p/support/AbstractGroup.java](https://codecov.io/gh/apache/incubator-dubbo/pull/2909/diff?src=pr&el=tree#diff-ZHViYm8tcmVtb3RpbmcvZHViYm8tcmVtb3RpbmctcDJwL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9kdWJiby9yZW1vdGluZy9wMnAvc3VwcG9ydC9BYnN0cmFjdEdyb3VwLmphdmE=) | `45.45% <0%> (-11.37%)` | :arrow_down: |
| [.../remoting/transport/netty4/NettyClientHandler.java](https://codecov.io/gh/apache/incubator-dubbo/pull/2909/diff?src=pr&el=tree#diff-ZHViYm8tcmVtb3RpbmcvZHViYm8tcmVtb3RpbmctbmV0dHk0L3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9kdWJiby9yZW1vdGluZy90cmFuc3BvcnQvbmV0dHk0L05ldHR5Q2xpZW50SGFuZGxlci5qYXZh) | `75% <0%> (-11.12%)` | :arrow_down: |
| [...ubbo/rpc/protocol/dubbo/ChannelWrappedInvoker.java](https://codecov.io/gh/apache/incubator-dubbo/pull/2909/diff?src=pr&el=tree#diff-ZHViYm8tcnBjL2R1YmJvLXJwYy1kdWJiby9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvZHViYm8vcnBjL3Byb3RvY29sL2R1YmJvL0NoYW5uZWxXcmFwcGVkSW52b2tlci5qYXZh) | `41.66% <0%> (-8.34%)` | :arrow_down: |
| [...ng/transport/dispatcher/all/AllChannelHandler.java](https://codecov.io/gh/apache/incubator-dubbo/pull/2909/diff?src=pr&el=tree#diff-ZHViYm8tcmVtb3RpbmcvZHViYm8tcmVtb3RpbmctYXBpL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9kdWJiby9yZW1vdGluZy90cmFuc3BvcnQvZGlzcGF0Y2hlci9hbGwvQWxsQ2hhbm5lbEhhbmRsZXIuamF2YQ==) | `51.42% <0%> (-5.72%)` | :arrow_down: |
| ... and [8 more](https://codecov.io/gh/apache/incubator-dubbo/pull/2909/diff?src=pr&el=tree-more) | |

------

[Continue to review full report at Codecov](https://codecov.io/gh/apache/incubator-dubbo/pull/2909?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/2909?src=pr&el=footer). Last update [ea71adb...10a106d](https://codecov.io/gh/apache/incubator-dubbo/pull/2909?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/2909 ]
This message was relayed via gitbox.apache.org for notifications@dubbo.apache.org

[GitHub] [incubator-dubbo] beiwei30 commented on pull request #2909: create AbstractRouter

Posted by "beiwei30 (GitHub)" <gi...@apache.org>.
instead of introduce AbstractRouter, why don't we do this on the interface?

```java
    int getPriority();

    @Override
    default int compareTo(Router o) {
        if (o == null) {
            throw new IllegalArgumentException();
        }

        if (this.getPriority() == o.getPriority()) {
            if (o.getUrl() == null) {
                return -1;
            }
            return getUrl().toFullString().compareTo(o.getUrl().toFullString());
        } else {
            return this.getPriority() > o.getPriority() ? 1 : -1;
        }
    }
}
```

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

[GitHub] [incubator-dubbo] carryxyh commented on pull request #2909: create AbstractRouter

Posted by "carryxyh (GitHub)" <gi...@apache.org>.
Simply,
Sometimes the url compare(super method) return 1. But the rule compare return -1.
If we can super first, it will return 1.
But in the old logic, it will return -1.

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

[GitHub] [incubator-dubbo] beiwei30 commented on pull request #2909: create AbstractRouter

Posted by "beiwei30 (GitHub)" <gi...@apache.org>.
shouldn't we call super first, and do extra logic when super return 0 instead?

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

[GitHub] [incubator-dubbo] beiwei30 commented on pull request #2909: create AbstractRouter

Posted by "beiwei30 (GitHub)" <gi...@apache.org>.
I think `int` is better, let's rely on auto-boxing.

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

[GitHub] [incubator-dubbo] Leishunyu commented on pull request #2909: create AbstractRouter

Posted by "Leishunyu (GitHub)" <gi...@apache.org>.
Your suggestion has been adopted, the default method of jdk8 is really better here, thank you

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