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-2029]: upgrade netty4 to the latest release and make it the default option for transporter #2049

Merged
merged 2 commits into from
Jul 9, 2018

Conversation

beiwei30
Copy link
Member

@beiwei30 beiwei30 commented Jul 9, 2018

…lt option for transporter

What is the purpose of the change

issue#2029: upgrade netty4 to the latest release and make it the default option for transporter

Brief changelog

dependencies-bom/pom.xml
dubbo-common/src/main/java/org/apache/dubbo/common/Constants.java
dubbo-config/dubbo-config-api/pom.xml
dubbo-config/dubbo-config-spring/pom.xml
dubbo-demo/dubbo-demo-consumer/pom.xml
dubbo-demo/dubbo-demo-provider/pom.xml
dubbo-monitor/dubbo-monitor-default/pom.xml
dubbo-registry/dubbo-registry-default/pom.xml
dubbo-remoting/dubbo-remoting-api/src/main/java/org/apache/dubbo/remoting/Transporter.java
dubbo-remoting/dubbo-remoting-netty/src/test/java/org/apache/dubbo/remoting/exchange/support/header/HeartbeatHandlerTest.java
dubbo-remoting/dubbo-remoting-netty/src/test/java/org/apache/dubbo/remoting/transport/netty/ClientReconnectTest.java
dubbo-remoting/dubbo-remoting-netty/src/test/java/org/apache/dubbo/remoting/transport/netty/NettyClientTest.java
dubbo-remoting/dubbo-remoting-netty4/src/main/java/org/apache/dubbo/remoting/transport/netty4/NettyClientHandler.java
dubbo-remoting/dubbo-remoting-p2p/pom.xml
dubbo-rpc/dubbo-rpc-dubbo/pom.xml
dubbo-rpc/dubbo-rpc-thrift/pom.xml

Verifying this change

XXXXX

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.

@codecov-io
Copy link

codecov-io commented Jul 9, 2018

Codecov Report

Merging #2049 into master will increase coverage by 0.83%.
The diff coverage is 12.5%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master    #2049      +/-   ##
============================================
+ Coverage     52.19%   53.02%   +0.83%     
- Complexity     4963     4986      +23     
============================================
  Files           568      568              
  Lines         25453    25460       +7     
  Branches       4486     4487       +1     
============================================
+ Hits          13285    13501     +216     
+ Misses        10144     9913     -231     
- Partials       2024     2046      +22
Impacted Files Coverage Δ Complexity Δ
...bbo/remoting/transport/netty/NettyTransporter.java 100% <ø> (ø) 3 <0> (ø) ⬇️
...bo/remoting/transport/netty4/NettyTransporter.java 100% <ø> (ø) 3 <0> (ø) ⬇️
.../remoting/transport/netty4/NettyClientHandler.java 58.33% <12.5%> (-14.09%) 5 <0> (ø)
...ache/dubbo/remoting/p2p/support/AbstractGroup.java 45.45% <0%> (-11.37%) 7% <0%> (-1%)
...he/dubbo/remoting/transport/netty/NettyClient.java 72.88% <0%> (-8.48%) 12% <0%> (-1%)
...rpc/protocol/dubbo/telnet/InvokeTelnetHandler.java 51.89% <0%> (-3.8%) 11% <0%> (-2%)
...dubbo/rpc/protocol/dubbo/CallbackServiceCodec.java 77.2% <0%> (-1.48%) 29% <0%> (ø)
.../dubbo/remoting/transport/netty4/NettyChannel.java 65% <0%> (-1.25%) 22% <0%> (-1%)
.../apache/dubbo/rpc/protocol/injvm/InjvmInvoker.java 69.23% <0%> (+15.38%) 3% <0%> (+1%) ⬆️
.../apache/dubbo/rpc/protocol/thrift/ThriftUtils.java 91.8% <0%> (+16.39%) 8% <0%> (+2%) ⬆️
... and 3 more

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 b36a6ee...7f89783. Read the comment docs.

@beiwei30 beiwei30 requested review from qinliujie and chickenlj and removed request for qinliujie July 9, 2018 05:21
@chickenlj
Copy link
Contributor

chickenlj commented Jul 9, 2018

Agree to change the default transporter to netty4.

Only one thing to note: this change is not transparent to the user, they should guarantee [netty 4](https://github.com/netty/netty) in the classpath, which is not always satisfied since their projects may rely on netty3 by default. But I think it's ok, a release note for incompatible upgrade is enough.

@chickenlj
Copy link
Contributor

chickenlj commented Jul 9, 2018

To continue use netty, how about this way:

based on your PR, add the following items,
For netty4/Transporter

netty=org.apache.dubbo.remoting.transport.netty4.NettyTransporter
netty4=org.apache.dubbo.remoting.transport.netty4.NettyTransporter

And

For netty3/Transporter

netty3=org.apache.dubbo.remoting.transport.netty.NettyTransporter

@chickenlj
Copy link
Contributor

Keep netty no changed

@SPI("netty")
 public interface Transporter {

@zonghaishang
Copy link
Member

Perhaps the server should be removed and passed to the client.

@chickenlj
Copy link
Contributor

@zonghaishang Agree to remove server from the client, and this can be achieved within this issue #2030

@chickenlj chickenlj merged commit d2ae941 into apache:master Jul 9, 2018
@beiwei30 beiwei30 deleted the issue2036 branch July 10, 2018 02:08
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