Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

[Dubbo- netty4 nettyclienthandler should trigger heartbeat handler] fix heartbeat when channel active #1997

Merged
merged 2 commits into from
Jun 29, 2018

Conversation

zonghaishang
Copy link
Member

What is the purpose of the change

set heartbeat flag when client channel active (netty4), issue: #1996

Brief changelog

  1. org.apache.dubbo.remoting.transport.netty4.NettyClientHandler#channelActive
  2. org.apache.dubbo.remoting.transport.netty4.NettyClientHandler#channelInactive
    trigger handler event.

Verifying this change

Enable netty4 transporter, client connect to remote server, org.apache.dubbo.remoting.exchange.support.header.HeaderExchangeHandler#connected should be triggered.

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

  • Make sure there is a GITHUB_issue 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.
  • 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.

@zonghaishang
Copy link
Member Author

@qinliujie Please help to review this pr, thanks.

@codecov-io
Copy link

codecov-io commented Jun 27, 2018

Codecov Report

Merging #1997 into master will increase coverage by 0.03%.
The diff coverage is 100%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master    #1997      +/-   ##
============================================
+ Coverage     50.94%   50.98%   +0.03%     
- Complexity     4773     4775       +2     
============================================
  Files           562      562              
  Lines         25218    25221       +3     
  Branches       4449     4449              
============================================
+ Hits          12848    12859      +11     
+ Misses        10439    10432       -7     
+ Partials       1931     1930       -1
Impacted Files Coverage Δ Complexity Δ
.../remoting/transport/netty4/NettyServerHandler.java 71.42% <ø> (-0.8%) 6 <0> (ø)
.../remoting/transport/netty4/NettyClientHandler.java 63.63% <100%> (+5.01%) 5 <2> (ø) ⬇️
...onfig/spring/extension/SpringExtensionFactory.java 73.07% <0%> (-11.54%) 8% <0%> (ø)
...bo/remoting/transport/netty/NettyCodecAdapter.java 53.12% <0%> (-1.57%) 3% <0%> (ø)
.../dubbo/remoting/transport/netty4/NettyChannel.java 65% <0%> (-1.25%) 22% <0%> (-1%)
...a/org/apache/dubbo/monitor/dubbo/DubboMonitor.java 89.71% <0%> (+1.86%) 15% <0%> (ø) ⬇️
...rpc/protocol/dubbo/telnet/InvokeTelnetHandler.java 55.69% <0%> (+3.79%) 13% <0%> (+3%) ⬆️
...ubbo/rpc/protocol/dubbo/ChannelWrappedInvoker.java 50% <0%> (+4.16%) 3% <0%> (ø) ⬇️
...apache/dubbo/rpc/protocol/dubbo/FutureAdapter.java 48.38% <0%> (+19.35%) 3% <0%> (ø) ⬇️

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 534fd24...26af99e. Read the comment docs.

@qinliujie
Copy link
Contributor

@zonghaishang it is ok,but I have a question,can we remove com.alibaba.dubbo.remoting.transport.netty4.NettyClientHandler#disconnect method?By querying the netty api document, I find that it is useless. The disconnect method is invoked only when the user intend to disconnect network and is not called when the network connection is broken.So I think channelInactive method is enough

@zonghaishang
Copy link
Member Author

@qinliujie

I confirmed your question and found the basis from the netty project:

channelOpen, channelBound, and channelConnected have been merged to channelActive. channelDisconnected, channelUnbound, and channelClosed have been merged to channelInactive. Likewise, Channel.isBound() and isConnected() have been merged to isActive().

More detail can refer here: https://github.com/netty/netty/wiki/New-and-noteworthy-in-4.0
Em... we can safely remove the disconnect method

@zonghaishang zonghaishang requested a review from qinliujie June 27, 2018 16:47
@qinliujie
Copy link
Contributor

@zonghaishang well done!! @chickenlj please review again~

@zonghaishang zonghaishang requested a review from chickenlj June 29, 2018 03:31
@chickenlj chickenlj merged commit c271039 into apache:master Jun 29, 2018
July-X pushed a commit to July-X/dubbo that referenced this pull request Jul 12, 2018
* commit 'b055991b317f4e58d256721875a00c52fe415510': (271 commits)
  Merge pull request apache#1957, enhancements for the new async way of Dubbo.
  rename log file from alibaba to custom-access (apache#2057)
  Merge pull request apache#2049, upgrade netty4 to the latest release and make it the default option for transporter.
  Format style.
  Restore the badges in README.
  Polish README.
  Refactor README.
  Merge pull request apache#2047, deprecate dubbo-rpc-thrift.
  Merge pull request apache#2005, change maven parent from sonatype to apache.
  rename access log in unit test from 'alibaba' to 'alibaba.log' so that it cannot be committed by accident. (apache#2051)
  add test cases for injvm rpc protocol (apache#2041)
  add test cases for rpc thrift protocol (apache#2042)
  Merge pull request apache#1966, introduces dubbo metrics API module.
  [Dubbo- unit test class not found] fix class not found "hi" (apache#2034)
  add README for compatible module (apache#2019)
  Polish README.
  update README (apache#2025)
  [Dubbo-1695] Enhance the test coverage part-16 : dubbo-rpc/dubbo-rpc-api  module (apache#2004)
  Merge pull request apache#1997, clienthandler in netty4 should trigger heartbeat handler.
  Update issue template description.
  ...
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants