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

Posted to notifications@dubbo.apache.org by "lexburner (GitHub)" <gi...@apache.org> on 2019/01/19 12:13:40 UTC

[GitHub] [incubator-dubbo] lexburner opened pull request #3276: Improve/heartbeat

## What is the purpose of the change

remove the heartbeat on the server side

## Brief changelog

- Change the direction of the heartbeat request to a one-way request: client -> server.
- Added a timer to the server side to close the connection when the connection is idle. 



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

[GitHub] [incubator-dubbo] lexburner commented on issue #3276: Improve/heartbeat

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

> > @kexianjun In my opinion, idle check timer is responsible to a `Connection`, not a `Service`.
> 
> 目前的处理方式，一个HashedWheelTimer最多只处理两个任务(server端是一个，clien端是两个)，每个HashedWheelTimer都会启动一个线程检测，如果一个服务作为消费方调用服务比较多的情况下，就需要起很多线程做心跳检查，比如说我们的一个业务系统，调用其他不同系统的服务多达上十个，再加上线上集群部署，单是心跳检测线程就要起二三十个了。我觉得检测的任务和连接是对应的，定时器是可以共享的。

Got it. I will make this change.

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

[GitHub] [incubator-dubbo] lexburner commented on pull request #3276: Improve/heartbeat

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

These variable names are named by other people. In my opinion, stay the same is also OK.

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

[GitHub] [incubator-dubbo] khanimteyaz commented on pull request #3276: Improve/heartbeat

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

In **HeaderExchangeHandler.java** disconnect method we are setting KEY_READ_TIMESTAMP and KEY_WRITE_TIMESTAMP with the value of System.currentTimeMillis() , will these impact this detection here? Should we also consider whether a given channel has already been closed or not? What do you say?

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

[GitHub] [incubator-dubbo] khanimteyaz commented on pull request #3276: Improve/heartbeat

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

**h** can be rename to **urlHeartBeatKey** what do you say?

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

[GitHub] [incubator-dubbo] kexianjun commented on issue #3276: Improve/heartbeat

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

> @kexianjun In my opinion, idle check timer is responsible to a `Connection`, not a `Service`.

目前的处理方式，一个HashedWheelTimer最多只处理两个任务(server端是一个，clien端是两个)，每个HashedWheelTimer都会启动一个线程检测，如果一个服务作为消费方调用服务比较多的情况下，就需要起很多线程做心跳检查，比如说我们的一个业务系统，调用其他不同系统的服务多达上十个，再加上线上集群部署，单是心跳检测线程就要起二三十个了。我觉得检测的任务和连接是对应的，定时器是可以共享的。

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

[GitHub] [incubator-dubbo] khanimteyaz commented on pull request #3276: Improve/heartbeat

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

can be renamed to **urlTimeOutKey**. What do you say?

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

[GitHub] [incubator-dubbo] khanimteyaz commented on pull request #3276: Improve/heartbeat

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

I could be wrong here, but just wanted to understand and would love to gets wrong myself 😄 .  In the scenario where channel has received data but have not perform any write or vice versa should we consider as timeout  (as we have OR condition based check).
 

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

[GitHub] [incubator-dubbo] khanimteyaz commented on pull request #3276: Improve/heartbeat

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

Can be replace with 
`Assert.notNull(client, "Client can't be null");`

as already dubbo already have a util for this.

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

[GitHub] [incubator-dubbo] khanimteyaz commented on pull request #3276: Improve/heartbeat

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

can be renamed to **urlTimeoutKey**. What do you say?

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

[GitHub] [incubator-dubbo] codecov-io commented on issue #3276: Improve/heartbeat

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

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

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

```diff
@@             Coverage Diff              @@
##             master    #3276      +/-   ##
============================================
- Coverage     63.98%   63.92%   -0.07%     
  Complexity       75       75              
============================================
  Files           653      654       +1     
  Lines         28218    28228      +10     
  Branches       4778     4779       +1     
============================================
- Hits          18055    18044      -11     
- Misses         7978     7993      +15     
- Partials       2185     2191       +6
```


| [Impacted Files](https://codecov.io/gh/apache/incubator-dubbo/pull/3276?src=pr&el=tree) | Coverage Δ | Complexity Δ | |
|---|---|---|---|
| [.../exchange/support/header/HeaderExchangeServer.java](https://codecov.io/gh/apache/incubator-dubbo/pull/3276/diff?src=pr&el=tree#diff-ZHViYm8tcmVtb3RpbmcvZHViYm8tcmVtb3RpbmctYXBpL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9kdWJiby9yZW1vdGluZy9leGNoYW5nZS9zdXBwb3J0L2hlYWRlci9IZWFkZXJFeGNoYW5nZVNlcnZlci5qYXZh) | `59.64% <52.94%> (-1.04%)` | `0 <0> (ø)` | |
| [...ng/exchange/support/header/ReconnectTimerTask.java](https://codecov.io/gh/apache/incubator-dubbo/pull/3276/diff?src=pr&el=tree#diff-ZHViYm8tcmVtb3RpbmcvZHViYm8tcmVtb3RpbmctYXBpL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9kdWJiby9yZW1vdGluZy9leGNoYW5nZS9zdXBwb3J0L2hlYWRlci9SZWNvbm5lY3RUaW1lclRhc2suamF2YQ==) | `75% <55.55%> (-8.34%)` | `0 <0> (ø)` | |
| [...moting/exchange/support/header/CloseTimerTask.java](https://codecov.io/gh/apache/incubator-dubbo/pull/3276/diff?src=pr&el=tree#diff-ZHViYm8tcmVtb3RpbmcvZHViYm8tcmVtb3RpbmctYXBpL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9kdWJiby9yZW1vdGluZy9leGNoYW5nZS9zdXBwb3J0L2hlYWRlci9DbG9zZVRpbWVyVGFzay5qYXZh) | `73.33% <73.33%> (ø)` | `0 <0> (?)` | |
| [.../exchange/support/header/HeaderExchangeClient.java](https://codecov.io/gh/apache/incubator-dubbo/pull/3276/diff?src=pr&el=tree#diff-ZHViYm8tcmVtb3RpbmcvZHViYm8tcmVtb3RpbmctYXBpL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9kdWJiby9yZW1vdGluZy9leGNoYW5nZS9zdXBwb3J0L2hlYWRlci9IZWFkZXJFeGNoYW5nZUNsaWVudC5qYXZh) | `82.35% <84.61%> (+1.47%)` | `0 <0> (ø)` | :arrow_down: |
| [...ache/dubbo/remoting/p2p/support/AbstractGroup.java](https://codecov.io/gh/apache/incubator-dubbo/pull/3276/diff?src=pr&el=tree#diff-ZHViYm8tcmVtb3RpbmcvZHViYm8tcmVtb3RpbmctcDJwL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9kdWJiby9yZW1vdGluZy9wMnAvc3VwcG9ydC9BYnN0cmFjdEdyb3VwLmphdmE=) | `45.45% <0%> (-11.37%)` | `0% <0%> (ø)` | |
| [...onfig/spring/extension/SpringExtensionFactory.java](https://codecov.io/gh/apache/incubator-dubbo/pull/3276/diff?src=pr&el=tree#diff-ZHViYm8tY29uZmlnL2R1YmJvLWNvbmZpZy1zcHJpbmcvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL2R1YmJvL2NvbmZpZy9zcHJpbmcvZXh0ZW5zaW9uL1NwcmluZ0V4dGVuc2lvbkZhY3RvcnkuamF2YQ==) | `78.04% <0%> (-7.32%)` | `0% <0%> (ø)` | |
| [.../apache/dubbo/remoting/transport/AbstractPeer.java](https://codecov.io/gh/apache/incubator-dubbo/pull/3276/diff?src=pr&el=tree#diff-ZHViYm8tcmVtb3RpbmcvZHViYm8tcmVtb3RpbmctYXBpL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9kdWJiby9yZW1vdGluZy90cmFuc3BvcnQvQWJzdHJhY3RQZWVyLmphdmE=) | `60.86% <0%> (-6.53%)` | `0% <0%> (ø)` | |
| [...he/dubbo/remoting/transport/netty/NettyServer.java](https://codecov.io/gh/apache/incubator-dubbo/pull/3276/diff?src=pr&el=tree#diff-ZHViYm8tcmVtb3RpbmcvZHViYm8tcmVtb3RpbmctbmV0dHkvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL2R1YmJvL3JlbW90aW5nL3RyYW5zcG9ydC9uZXR0eS9OZXR0eVNlcnZlci5qYXZh) | `69.64% <0%> (-3.58%)` | `0% <0%> (ø)` | |
| [...pache/dubbo/registry/support/AbstractRegistry.java](https://codecov.io/gh/apache/incubator-dubbo/pull/3276/diff?src=pr&el=tree#diff-ZHViYm8tcmVnaXN0cnkvZHViYm8tcmVnaXN0cnktYXBpL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9kdWJiby9yZWdpc3RyeS9zdXBwb3J0L0Fic3RyYWN0UmVnaXN0cnkuamF2YQ==) | `79.61% <0%> (-1.93%)` | `0% <0%> (ø)` | |
| [...he/dubbo/registry/multicast/MulticastRegistry.java](https://codecov.io/gh/apache/incubator-dubbo/pull/3276/diff?src=pr&el=tree#diff-ZHViYm8tcmVnaXN0cnkvZHViYm8tcmVnaXN0cnktbXVsdGljYXN0L3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9kdWJiby9yZWdpc3RyeS9tdWx0aWNhc3QvTXVsdGljYXN0UmVnaXN0cnkuamF2YQ==) | `64.93% <0%> (-1.74%)` | `0% <0%> (ø)` | |
| ... and [9 more](https://codecov.io/gh/apache/incubator-dubbo/pull/3276/diff?src=pr&el=tree-more) | |

------

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

[GitHub] [incubator-dubbo] khanimteyaz commented on pull request #3276: Improve/heartbeat

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

can be replace with 

Can be replace with
Assert.notNull(server, "Server can't be null");

as already dubbo already have a util for this.

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

[GitHub] [incubator-dubbo] codecov-io commented on issue #3276: Improve/heartbeat

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

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

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

```diff
@@             Coverage Diff              @@
##             master    #3276      +/-   ##
============================================
- Coverage     64.02%   63.88%   -0.15%     
  Complexity       75       75              
============================================
  Files           653      654       +1     
  Lines         28218    28213       -5     
  Branches       4778     4777       -1     
============================================
- Hits          18067    18024      -43     
- Misses         7967     8000      +33     
- Partials       2184     2189       +5
```


| [Impacted Files](https://codecov.io/gh/apache/incubator-dubbo/pull/3276?src=pr&el=tree) | Coverage Δ | Complexity Δ | |
|---|---|---|---|
| [.../exchange/support/header/HeaderExchangeServer.java](https://codecov.io/gh/apache/incubator-dubbo/pull/3276/diff?src=pr&el=tree#diff-ZHViYm8tcmVtb3RpbmcvZHViYm8tcmVtb3RpbmctYXBpL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9kdWJiby9yZW1vdGluZy9leGNoYW5nZS9zdXBwb3J0L2hlYWRlci9IZWFkZXJFeGNoYW5nZVNlcnZlci5qYXZh) | `60.37% <53.84%> (-0.31%)` | `0 <0> (ø)` | |
| [...ng/exchange/support/header/ReconnectTimerTask.java](https://codecov.io/gh/apache/incubator-dubbo/pull/3276/diff?src=pr&el=tree#diff-ZHViYm8tcmVtb3RpbmcvZHViYm8tcmVtb3RpbmctYXBpL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9kdWJiby9yZW1vdGluZy9leGNoYW5nZS9zdXBwb3J0L2hlYWRlci9SZWNvbm5lY3RUaW1lclRhc2suamF2YQ==) | `75% <55.55%> (-8.34%)` | `0 <0> (ø)` | |
| [...moting/exchange/support/header/CloseTimerTask.java](https://codecov.io/gh/apache/incubator-dubbo/pull/3276/diff?src=pr&el=tree#diff-ZHViYm8tcmVtb3RpbmcvZHViYm8tcmVtb3RpbmctYXBpL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9kdWJiby9yZW1vdGluZy9leGNoYW5nZS9zdXBwb3J0L2hlYWRlci9DbG9zZVRpbWVyVGFzay5qYXZh) | `73.33% <73.33%> (ø)` | `0 <0> (?)` | |
| [.../exchange/support/header/HeaderExchangeClient.java](https://codecov.io/gh/apache/incubator-dubbo/pull/3276/diff?src=pr&el=tree#diff-ZHViYm8tcmVtb3RpbmcvZHViYm8tcmVtb3RpbmctYXBpL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9kdWJiby9yZW1vdGluZy9leGNoYW5nZS9zdXBwb3J0L2hlYWRlci9IZWFkZXJFeGNoYW5nZUNsaWVudC5qYXZh) | `83.6% <80%> (+2.72%)` | `0 <0> (ø)` | :arrow_down: |
| [...ache/dubbo/remoting/p2p/support/AbstractGroup.java](https://codecov.io/gh/apache/incubator-dubbo/pull/3276/diff?src=pr&el=tree#diff-ZHViYm8tcmVtb3RpbmcvZHViYm8tcmVtb3RpbmctcDJwL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9kdWJiby9yZW1vdGluZy9wMnAvc3VwcG9ydC9BYnN0cmFjdEdyb3VwLmphdmE=) | `45.45% <0%> (-11.37%)` | `0% <0%> (ø)` | |
| [...he/dubbo/remoting/transport/netty/NettyServer.java](https://codecov.io/gh/apache/incubator-dubbo/pull/3276/diff?src=pr&el=tree#diff-ZHViYm8tcmVtb3RpbmcvZHViYm8tcmVtb3RpbmctbmV0dHkvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL2R1YmJvL3JlbW90aW5nL3RyYW5zcG9ydC9uZXR0eS9OZXR0eVNlcnZlci5qYXZh) | `64.28% <0%> (-8.93%)` | `0% <0%> (ø)` | |
| [.../apache/dubbo/remoting/transport/AbstractPeer.java](https://codecov.io/gh/apache/incubator-dubbo/pull/3276/diff?src=pr&el=tree#diff-ZHViYm8tcmVtb3RpbmcvZHViYm8tcmVtb3RpbmctYXBpL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9kdWJiby9yZW1vdGluZy90cmFuc3BvcnQvQWJzdHJhY3RQZWVyLmphdmE=) | `58.69% <0%> (-8.7%)` | `0% <0%> (ø)` | |
| [...dubbo/remoting/exchange/support/DefaultFuture.java](https://codecov.io/gh/apache/incubator-dubbo/pull/3276/diff?src=pr&el=tree#diff-ZHViYm8tcmVtb3RpbmcvZHViYm8tcmVtb3RpbmctYXBpL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9kdWJiby9yZW1vdGluZy9leGNoYW5nZS9zdXBwb3J0L0RlZmF1bHRGdXR1cmUuamF2YQ==) | `70.94% <0%> (-4.73%)` | `0% <0%> (ø)` | |
| [...e/dubbo/remoting/transport/netty/NettyChannel.java](https://codecov.io/gh/apache/incubator-dubbo/pull/3276/diff?src=pr&el=tree#diff-ZHViYm8tcmVtb3RpbmcvZHViYm8tcmVtb3RpbmctbmV0dHkvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL2R1YmJvL3JlbW90aW5nL3RyYW5zcG9ydC9uZXR0eS9OZXR0eUNoYW5uZWwuamF2YQ==) | `57.64% <0%> (-4.71%)` | `0% <0%> (ø)` | |
| [...rg/apache/dubbo/common/timer/HashedWheelTimer.java](https://codecov.io/gh/apache/incubator-dubbo/pull/3276/diff?src=pr&el=tree#diff-ZHViYm8tY29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9kdWJiby9jb21tb24vdGltZXIvSGFzaGVkV2hlZWxUaW1lci5qYXZh) | `58.18% <0%> (-4.19%)` | `0% <0%> (ø)` | |
| ... and [16 more](https://codecov.io/gh/apache/incubator-dubbo/pull/3276/diff?src=pr&el=tree-more) | |

------

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

[GitHub] [incubator-dubbo] lexburner commented on pull request #3276: Improve/heartbeat

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

The magic number is indeed troublesome, but it isn't related to this pull request. Let us change them in other pr.

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

[GitHub] [incubator-dubbo] khanimteyaz commented on pull request #3276: Improve/heartbeat

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

`private final int NO_HEARTBEAT = 0;`
can we replace **0** in case if **dubbo** does not start with **1.0** with NO_HEARTBEAT for better readability.

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

[GitHub] [incubator-dubbo] lexburner commented on pull request #3276: Improve/heartbeat

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

invocation can be divided into two kinds: normal, heartbeat. Both of them should update the  KEY_READ_TIMESTAMP and KEY_WRITE_TIMESTAMP. This is a deliberate design, which means IDLE CHECK.

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

[GitHub] [incubator-dubbo] beiwei30 commented on pull request #3276: Improve/heartbeat

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

why can we remove `stopHeartbeatTimer()`?

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

[GitHub] [incubator-dubbo] beiwei30 closed pull request #3276: Improve/heartbeat

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

[ pull request closed by beiwei30 ]

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

[GitHub] [incubator-dubbo] lexburner commented on issue #3276: Improve/heartbeat

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

@kexianjun In my opinion, idle check timer is responsible to a `Connection`, not a `Service`. 

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

[GitHub] [incubator-dubbo] carryxyh commented on issue #3276: Improve/heartbeat

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

@kexianjun 
同意。之前这个地方我做的时候忽略了这个地方，谢谢提醒。

@lexburner 
这个pr里可以直接把这个地方共享掉，需要注意的是hashedWheelTimer的容量需要增大（Constants.TICKS_PER_WHEEL这个值）。我们需要尽量避免hash冲突来减少一些运算。

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

[GitHub] [incubator-dubbo] lexburner commented on pull request #3276: Improve/heartbeat

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

The timer to send heartbeat was an instance of every `Connection` before this change, I have changed the scope of `Timer` to `static`, so we could not close the heartbeat timer by non-static method. 

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

[GitHub] [incubator-dubbo] carryxyh commented on issue #3276: Improve/heartbeat

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

Since this pr has been merged, I open a new pr to optimize this point.

Since Timer is now shared, we won't turn off the timer when a few clients or servers are down. At this point we need to ensure that our tasks don't grow indefinitely.

I added a mapping between client(server) and task. When a client(server) is closed, we can cancel the corresponding task. The canceled task will not be re-inputted into the timer to ensure that our number of tasks is controllable.

would u pls have a look?

pr:
https://github.com/apache/incubator-dubbo/pull/3299

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

[GitHub] [incubator-dubbo] lexburner commented on pull request #3276: Improve/heartbeat

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

> > @kexianjun In my opinion, idle check timer is responsible to a `Connection`, not a `Service`.
> 
> 目前的处理方式，一个HashedWheelTimer最多只处理两个任务(server端是一个，clien端是两个)，每个HashedWheelTimer都会启动一个线程检测，如果一个服务作为消费方调用服务比较多的情况下，就需要起很多线程做心跳检查，比如说我们的一个业务系统，调用其他不同系统的服务多达上十个，再加上线上集群部署，单是心跳检测线程就要起二三十个了。我觉得检测的任务和连接是对应的，定时器是可以共享的。

Got it. I will make this change.

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

[GitHub] [incubator-dubbo] beiwei30 commented on pull request #3276: Improve/heartbeat

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

i see.

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

[GitHub] [incubator-dubbo] carryxyh commented on pull request #3276: Improve/heartbeat

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

I think we should establish a mapping relationship between client and task.

Since we are using the global Timer at the moment, when any client is closed, we should cancel the task corresponding to this client. In this way, we can guarantee that our tasks will not be too much.

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