You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@dubbo.apache.org by "beiwei30 (GitHub)" <gi...@apache.org> on 2018/10/23 09:39:35 UTC
[GitHub] [incubator-dubbo] beiwei30 opened pull request #2679:
issue2016: Consumer throws RpcException after RegistryDirectory notify in
high QPS
## What is the purpose of the change
issue2016: Consumer throws RpcException after RegistryDirectory notify in high QPS
## Brief changelog
dubbo-rpc/dubbo-rpc-api/src/main/java/org/apache/dubbo/rpc/protocol/AbstractInvoker.java
## Verifying this change
XXXXX
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) filed 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/2679 ]
This message was relayed via gitbox.apache.org for notifications@dubbo.apache.org
[GitHub] [incubator-dubbo] chickenlj commented on issue #2679:
issue2016: Consumer throws RpcException after RegistryDirectory notify in
high QPS
Posted by "chickenlj (GitHub)" <gi...@apache.org>.
It's a concurrent problem of Registry notification thread and RPC invoking thread.
I think your patch can reduce the chances of meeting this problem to some extent but makes no essential differences. If the `destroy` status of an Invoker was set, most likely, the remote connection and some other resources may have also been destroyed, which may still cause the invoke to fail. One side effect if the invoke fails later is that the user may finally get a raw connection exception which may further confuse them.
https://github.com/apache/incubator-dubbo/blob/6938d487a388cc8cd0a2bc8740fe9ee8d378767e/dubbo-rpc/dubbo-rpc-dubbo/src/main/java/org/apache/dubbo/rpc/protocol/dubbo/DubboInvoker.java#L132
How about add a retry policy in AbstractClusterInvoker.invoke() for this scenario, and we can decide whether retry or not by recognize the RpcException with a special code? I am still thinking of some other better solutions.
[ Full content available at: https://github.com/apache/incubator-dubbo/pull/2679 ]
This message was relayed via gitbox.apache.org for notifications@dubbo.apache.org
[GitHub] [incubator-dubbo] codecov-io commented on issue #2679:
issue2016: Consumer throws RpcException after RegistryDirectory notify in
high QPS
Posted by "codecov-io (GitHub)" <gi...@apache.org>.
# [Codecov](https://codecov.io/gh/apache/incubator-dubbo/pull/2679?src=pr&el=h1) Report
> Merging [#2679](https://codecov.io/gh/apache/incubator-dubbo/pull/2679?src=pr&el=desc) into [master](https://codecov.io/gh/apache/incubator-dubbo/commit/6938d487a388cc8cd0a2bc8740fe9ee8d378767e?src=pr&el=desc) will **decrease** coverage by `1.27%`.
> The diff coverage is `0%`.
[](https://codecov.io/gh/apache/incubator-dubbo/pull/2679?src=pr&el=tree)
```diff
@@ Coverage Diff @@
## master #2679 +/- ##
==========================================
- Coverage 63.18% 61.91% -1.28%
==========================================
Files 574 596 +22
Lines 25786 27781 +1995
Branches 4537 4825 +288
==========================================
+ Hits 16294 17201 +907
- Misses 7341 8317 +976
- Partials 2151 2263 +112
```
| [Impacted Files](https://codecov.io/gh/apache/incubator-dubbo/pull/2679?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [...org/apache/dubbo/rpc/protocol/AbstractInvoker.java](https://codecov.io/gh/apache/incubator-dubbo/pull/2679/diff?src=pr&el=tree#diff-ZHViYm8tcnBjL2R1YmJvLXJwYy1hcGkvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL2R1YmJvL3JwYy9wcm90b2NvbC9BYnN0cmFjdEludm9rZXIuamF2YQ==) | `62.9% <0%> (ø)` | :arrow_up: |
| [...ache/dubbo/remoting/transport/AbstractChannel.java](https://codecov.io/gh/apache/incubator-dubbo/pull/2679/diff?src=pr&el=tree#diff-ZHViYm8tcmVtb3RpbmcvZHViYm8tcmVtb3RpbmctYXBpL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9kdWJiby9yZW1vdGluZy90cmFuc3BvcnQvQWJzdHJhY3RDaGFubmVsLmphdmE=) | `37.5% <0%> (-50%)` | :arrow_down: |
| [...che/dubbo/remoting/transport/mina/MinaChannel.java](https://codecov.io/gh/apache/incubator-dubbo/pull/2679/diff?src=pr&el=tree#diff-ZHViYm8tcmVtb3RpbmcvZHViYm8tcmVtb3RpbmctbWluYS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvZHViYm8vcmVtb3RpbmcvdHJhbnNwb3J0L21pbmEvTWluYUNoYW5uZWwuamF2YQ==) | `39.47% <0%> (-10.53%)` | :arrow_down: |
| [...onfig/spring/extension/SpringExtensionFactory.java](https://codecov.io/gh/apache/incubator-dubbo/pull/2679/diff?src=pr&el=tree#diff-ZHViYm8tY29uZmlnL2R1YmJvLWNvbmZpZy1zcHJpbmcvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL2R1YmJvL2NvbmZpZy9zcHJpbmcvZXh0ZW5zaW9uL1NwcmluZ0V4dGVuc2lvbkZhY3RvcnkuamF2YQ==) | `75.86% <0%> (-10.35%)` | :arrow_down: |
| [...he/dubbo/remoting/transport/netty/NettyClient.java](https://codecov.io/gh/apache/incubator-dubbo/pull/2679/diff?src=pr&el=tree#diff-ZHViYm8tcmVtb3RpbmcvZHViYm8tcmVtb3RpbmctbmV0dHkvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL2R1YmJvL3JlbW90aW5nL3RyYW5zcG9ydC9uZXR0eS9OZXR0eUNsaWVudC5qYXZh) | `72.88% <0%> (-10.17%)` | :arrow_down: |
| [...e/dubbo/remoting/transport/netty/NettyChannel.java](https://codecov.io/gh/apache/incubator-dubbo/pull/2679/diff?src=pr&el=tree#diff-ZHViYm8tcmVtb3RpbmcvZHViYm8tcmVtb3RpbmctbmV0dHkvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL2R1YmJvL3JlbW90aW5nL3RyYW5zcG9ydC9uZXR0eS9OZXR0eUNoYW5uZWwuamF2YQ==) | `54.11% <0%> (-8.24%)` | :arrow_down: |
| [...org/apache/dubbo/rpc/filter/ActiveLimitFilter.java](https://codecov.io/gh/apache/incubator-dubbo/pull/2679/diff?src=pr&el=tree#diff-ZHViYm8tcnBjL2R1YmJvLXJwYy1hcGkvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL2R1YmJvL3JwYy9maWx0ZXIvQWN0aXZlTGltaXRGaWx0ZXIuamF2YQ==) | `77.77% <0%> (-5.56%)` | :arrow_down: |
| [...ubbo/rpc/protocol/dubbo/ChannelWrappedInvoker.java](https://codecov.io/gh/apache/incubator-dubbo/pull/2679/diff?src=pr&el=tree#diff-ZHViYm8tcnBjL2R1YmJvLXJwYy1kdWJiby9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvZHViYm8vcnBjL3Byb3RvY29sL2R1YmJvL0NoYW5uZWxXcmFwcGVkSW52b2tlci5qYXZh) | `37.5% <0%> (-4.17%)` | :arrow_down: |
| [...rpc/protocol/dubbo/telnet/InvokeTelnetHandler.java](https://codecov.io/gh/apache/incubator-dubbo/pull/2679/diff?src=pr&el=tree#diff-ZHViYm8tcnBjL2R1YmJvLXJwYy1kdWJiby9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvZHViYm8vcnBjL3Byb3RvY29sL2R1YmJvL3RlbG5ldC9JbnZva2VUZWxuZXRIYW5kbGVyLmphdmE=) | `54.21% <0%> (-3.62%)` | :arrow_down: |
| [...he/dubbo/remoting/transport/netty/NettyServer.java](https://codecov.io/gh/apache/incubator-dubbo/pull/2679/diff?src=pr&el=tree#diff-ZHViYm8tcmVtb3RpbmcvZHViYm8tcmVtb3RpbmctbmV0dHkvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL2R1YmJvL3JlbW90aW5nL3RyYW5zcG9ydC9uZXR0eS9OZXR0eVNlcnZlci5qYXZh) | `67.85% <0%> (-3.58%)` | :arrow_down: |
| ... and [27 more](https://codecov.io/gh/apache/incubator-dubbo/pull/2679/diff?src=pr&el=tree-more) | |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/incubator-dubbo/pull/2679?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/2679?src=pr&el=footer). Last update [6938d48...95fc6d2](https://codecov.io/gh/apache/incubator-dubbo/pull/2679?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/2679 ]
This message was relayed via gitbox.apache.org for notifications@dubbo.apache.org
[GitHub] [incubator-dubbo] chickenlj closed pull request #2679:
issue2016: Consumer throws RpcException after RegistryDirectory notify in
high QPS
Posted by "chickenlj (GitHub)" <gi...@apache.org>.
[ pull request closed by chickenlj ]
[ Full content available at: https://github.com/apache/incubator-dubbo/pull/2679 ]
This message was relayed via gitbox.apache.org for notifications@dubbo.apache.org
[GitHub] [incubator-dubbo] chickenlj commented on issue #2679:
issue2016: Consumer throws RpcException after RegistryDirectory notify in
high QPS
Posted by "chickenlj (GitHub)" <gi...@apache.org>.
```java
for (ExchangeClient client : clients) {
try {
client.close(ConfigUtils.getServerShutdownTimeout());
} catch (Throwable t) {
logger.warn(t.getMessage(), t);
}
}
```
I have double checked that `client.close();` will not actually close the client and any resources. Because the Client is an instance of ReferenceCountExchangeClient and it will only close when there's no Invoker other than this one is using it, while `rerefer` must happens before `destroyUnusedInvokers`, so there will at least two invokers refering the client instance when close method is called.
```java
@Override
public void close(int timeout) {
if (refenceCount.decrementAndGet() <= 0) {
if (timeout == 0) {
client.close();
} else {
client.close(timeout);
}
client = replaceWithLazyClient();
}
}
```
[ Full content available at: https://github.com/apache/incubator-dubbo/pull/2679 ]
This message was relayed via gitbox.apache.org for notifications@dubbo.apache.org
[GitHub] [incubator-dubbo] chickenlj commented on issue #2679:
issue2016: Consumer throws RpcException after RegistryDirectory notify in
high QPS
Posted by "chickenlj (GitHub)" <gi...@apache.org>.
```java
for (ExchangeClient client : clients) {
try {
client.close(ConfigUtils.getServerShutdownTimeout());
} catch (Throwable t) {
logger.warn(t.getMessage(), t);
}
}
```
I have double checked that `client.close();` will not actually close the client and any resources. Because the Client is a ReferenceCountExchangeClient and it will only close when there's no Invoker other than this one is using it, while `rerefer` must happens before `destroyUnusedInvokers`, so there will at least two invokers refering the client instance when close method is called.
```java
@Override
public void close(int timeout) {
if (refenceCount.decrementAndGet() <= 0) {
if (timeout == 0) {
client.close();
} else {
client.close(timeout);
}
client = replaceWithLazyClient();
}
}
```
[ Full content available at: https://github.com/apache/incubator-dubbo/pull/2679 ]
This message was relayed via gitbox.apache.org for notifications@dubbo.apache.org