Skip to content
New issue

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

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

Already on GitHub? Sign in to your account

add optional parameter to support hessian protocol method overload and request protocol version #1331

Merged
merged 10 commits into from
Feb 24, 2018

Conversation

maxiaoguang64
Copy link
Contributor

@maxiaoguang64 maxiaoguang64 commented Feb 6, 2018

add hessian.overload.method to support method overload default is false
add hessian2.request to support request protocol version default is version 1

原来不支持重载的方法调用
@CLAassistant
Copy link

CLAassistant commented Feb 6, 2018

CLA assistant check
Thank you for your submission, we really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
You have signed the CLA already but the status is still pending? Let us recheck it.

1 similar comment
@CLAassistant
Copy link

CLA assistant check
Thank you for your submission, we really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
You have signed the CLA already but the status is still pending? Let us recheck it.

增加重载方法的单元测试
增加重载方法的单元测试
增加重载方法的单元测试
@codecov-io
Copy link

codecov-io commented Feb 6, 2018

Codecov Report

Merging #1331 into master will increase coverage by 0.11%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1331      +/-   ##
==========================================
+ Coverage   32.31%   32.43%   +0.11%     
==========================================
  Files         691      691              
  Lines       34535    34535              
  Branches     6811     6811              
==========================================
+ Hits        11161    11201      +40     
+ Misses      21449    21401      -48     
- Partials     1925     1933       +8
Impacted Files Coverage Δ
.../main/java/com/alibaba/dubbo/common/Constants.java 88.88% <ø> (ø) ⬆️
...ba/dubbo/rpc/protocol/hessian/HessianProtocol.java 60% <100%> (+2.62%) ⬆️
...ba/dubbo/remoting/transport/netty/NettyClient.java 72.58% <0%> (-11.3%) ⬇️
...mon/serialize/support/dubbo/GenericDataOutput.java 67.01% <0%> (-8.43%) ⬇️
...om/alibaba/dubbo/rpc/filter/ActiveLimitFilter.java 83.33% <0%> (-5.56%) ⬇️
...mmon/serialize/support/dubbo/GenericDataInput.java 59.07% <0%> (-4.64%) ⬇️
.../com/alibaba/dubbo/monitor/dubbo/DubboMonitor.java 73.14% <0%> (-2.78%) ⬇️
...baba/dubbo/remoting/transport/mina/MinaClient.java 57.97% <0%> (-1.45%) ⬇️
...cluster/loadbalance/ConsistentHashLoadBalance.java 0% <0%> (ø) ⬆️
...src/main/java/com/alibaba/dubbo/rpc/RpcResult.java 35.29% <0%> (ø) ⬆️
... and 7 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 98c548a...b9ac7d6. Read the comment docs.

@kimmking
Copy link
Member

kimmking commented Feb 6, 2018

good job

I suggest this option can be a optional parameter for hessian protocol, then developers can turn it on/off.

@@ -83,6 +83,7 @@ public void run() {
@SuppressWarnings("unchecked")
protected <T> T doRefer(Class<T> serviceType, URL url) throws RpcException {
HessianProxyFactory hessianProxyFactory = new HessianProxyFactory();
hessianProxyFactory.setOverloadEnabled(true);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I suggest this option can be a optional parameter for hessian protocol, then developers can turn it on/off.

Copy link
Contributor Author

@maxiaoguang64 maxiaoguang64 Feb 6, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok, please review again

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what happend in jdk8 travis-ci, please help me

@ralf0131
Copy link
Contributor

ralf0131 commented Feb 6, 2018

Hi, Could you please update the description in your pull request, and your commit message into English? Dubbo is a global community, please be aware that non-English commit message may not be understood by other people.

hessian协议中方法重载和request协议可配置
hessian协议中方法重载和request协议可配置
hessian协议中方法重载和request协议可配置
@maxiaoguang64 maxiaoguang64 changed the title hessian协议支持重载的方法调用 add optional parameter to support hessian protocal method overload and request protocal version Feb 6, 2018
hessian协议中方法重载和request协议可配置
@chickenlj chickenlj added the status/waiting-for-feedback Need reporters to triage label Feb 7, 2018
@@ -83,6 +83,10 @@ public void run() {
@SuppressWarnings("unchecked")
protected <T> T doRefer(Class<T> serviceType, URL url) throws RpcException {
HessianProxyFactory hessianProxyFactory = new HessianProxyFactory();
boolean isHessian2Request = url.getParameter(Constants.HESSIAN2_REQUEST_KEY, Constants.DEFAULT_HESSIAN2_REQUEST);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When will the Constants.HESSIAN2_REQUEST_KEY key add to url?
If i configure dubbo.service.hessian2.request=true, will that take effect?

Copy link
Contributor Author

@maxiaoguang64 maxiaoguang64 Feb 12, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sorry, i am not sure but will try it

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I will add hessian2.request and hessian.overload.method to dubbo.xsd.
currently you configure dubbo.service.hessian2.request and dubbo.service.hessian.overload.method will effective

@maxiaoguang64 maxiaoguang64 changed the title add optional parameter to support hessian protocal method overload and request protocal version add optional parameter to support hessian protocol method overload and request protocol version Feb 12, 2018
添加hessian协议规则
<xsd:annotation>
<xsd:documentation><![CDATA[ hessian protocol support overload method. ]]></xsd:documentation>
</xsd:annotation>
</xsd:attribute>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do you put different prefixes hessian2.request and hessian.overload.method.
Actually, i think we don't have to add xml support for every parameter, especially for parameters bind to a specific protocol, we can use properties or <dubbo:parameter /> to do that. Your previous changes is ready to work, would you please revert changes adding xml, sorry for misleading you.

取消dubbo:provider 和 dubbo:consumer中关于hessian协议的配置
@maxiaoguang64
Copy link
Contributor Author

@chickenlj
ok, i fix it.
now, we can use <dubbo:parameter key="hessian2.request" value="true" /> and <dubbo:parameter key="hessian.overload.method" value="true" />

@chickenlj chickenlj merged commit ace2f4f into apache:master Feb 24, 2018
@chickenlj chickenlj removed the status/waiting-for-feedback Need reporters to triage label Feb 24, 2018
zonghaishang pushed a commit to zonghaishang/dubbo that referenced this pull request Feb 26, 2018
* @reference support annotate on annotation type

* Fixes apache#1303 TimeUnit conversion error

* Fixes apache#1289, use bind_port as mapping key

* Fixes apache#1313, remove destroy check in Registry.

* checkout .travis.yml from origin/master

* Fix hessian2 serialized short, byte is converted to int bug (apache#1232)

* Fix hessian2 serialized short, byte is converted to int bug

* Fix hessian2 serialized short, byte is converted to int bug

* adapt jdk1.5+

* fixed travis-ci failed because of test cases. (apache#1370)

* Merge pull request apache#1377, remove redundant arguments for StatItem.isAllowable()

* Merge pull request apache#1378, replace StringBuider with simple string concatenation in log.

* Merge pull request apache#1375, remove unnecessary boxing.

Fixes apache#1245

* Merge pull request apache#1331, add optional parameter to support hessian protocol method overload and request protocol version.

* Merge pull request apache#1376, do not instantiate load balance if there is no invokers

Fixes apache#1297

* Merge pull request apache#1384, fix build string bug.

* Merge pull request apache#1040, refactor: replace some deprecated methods related with jedis.

* Merge pull request apache#1242, remove redundant null check.

fixes apache#1231
zonghaishang pushed a commit to zonghaishang/dubbo that referenced this pull request Feb 26, 2018
* @reference support annotate on annotation type

* Fixes apache#1303 TimeUnit conversion error

* Fixes apache#1289, use bind_port as mapping key

* Fixes apache#1313, remove destroy check in Registry.

* checkout .travis.yml from origin/master

* Fix hessian2 serialized short, byte is converted to int bug (apache#1232)

* Fix hessian2 serialized short, byte is converted to int bug

* Fix hessian2 serialized short, byte is converted to int bug

* adapt jdk1.5+

* fixed travis-ci failed because of test cases. (apache#1370)

* Merge pull request apache#1377, remove redundant arguments for StatItem.isAllowable()

* Merge pull request apache#1378, replace StringBuider with simple string concatenation in log.

* Merge pull request apache#1375, remove unnecessary boxing.

Fixes apache#1245

* Merge pull request apache#1331, add optional parameter to support hessian protocol method overload and request protocol version.

* Merge pull request apache#1376, do not instantiate load balance if there is no invokers

Fixes apache#1297

* Merge pull request apache#1384, fix build string bug.

* Merge pull request apache#1040, refactor: replace some deprecated methods related with jedis.

* Merge pull request apache#1242, remove redundant null check.

fixes apache#1231

* Change Mailing list address

* Fixed apache#1398, revert bugs introduced from apache#1375

* Fix time unit problem in UT

* Fix time unit problem related with FutureAdapter in UT
zonghaishang pushed a commit to zonghaishang/dubbo that referenced this pull request Mar 2, 2018
* @reference support annotate on annotation type

* Fixes apache#1303 TimeUnit conversion error

* Fixes apache#1289, use bind_port as mapping key

* Fixes apache#1313, remove destroy check in Registry.

* checkout .travis.yml from origin/master

* Fix hessian2 serialized short, byte is converted to int bug (apache#1232)

* Fix hessian2 serialized short, byte is converted to int bug

* Fix hessian2 serialized short, byte is converted to int bug

* adapt jdk1.5+

* fixed travis-ci failed because of test cases. (apache#1370)

* Merge pull request apache#1377, remove redundant arguments for StatItem.isAllowable()

* Merge pull request apache#1378, replace StringBuider with simple string concatenation in log.

* Merge pull request apache#1375, remove unnecessary boxing.

Fixes apache#1245

* Merge pull request apache#1331, add optional parameter to support hessian protocol method overload and request protocol version.

* Merge pull request apache#1376, do not instantiate load balance if there is no invokers

Fixes apache#1297

* Merge pull request apache#1384, fix build string bug.

* Merge pull request apache#1040, refactor: replace some deprecated methods related with jedis.

* Merge pull request apache#1242, remove redundant null check.

fixes apache#1231

* Change Mailing list address

* Fixed apache#1398, revert bugs introduced from apache#1375

* Fix time unit problem in UT

* Fix time unit problem related with FutureAdapter in UT

* Fix time unit problem related with FutureAdapter in UT

* Merge pull request apache#1391, fix typo of method name in qos module.

* fix hessian lite test case fail bug (apache#1394)

* fix hessian lite test case fail bug

* update test

* remove ignore

* Fix time unit problem related with FutureAdapter in UT

* revert file

* fix number type is lost in yaml config file (apache#1401)

* apache#1399 fi

* update test

* update readme to add some details (apache#1403)

* update readme to add some details

update readme to add some details

* delete duplicated words

delete duplicated words

* update README format

*     apache#1411: Locale deserialize 'zh-hant_CN'
xpylq pushed a commit to xpylq/dubbo that referenced this pull request Mar 5, 2018
* remotes/upstream/master: (226 commits)
  clean up imports for CacheTest
  [Dubbo-apache#1362] cache provider always lru cache (apache#1396)
  Remove author info and add apache license
  Fix "promoteTransitiveDependencies=false" of maven-shade-plugin
  apache#1411: Locale deserialize 'zh-hant_CN'
  update README format
  update readme to add some details (apache#1403)
  fix number type is lost in yaml config file (apache#1401)
  fix hessian lite test case fail bug (apache#1394)
  Merge pull request apache#1391, fix typo of method name in qos module.
  Fix time unit problem related with FutureAdapter in UT
  Fix time unit problem related with FutureAdapter in UT
  Fix time unit problem in UT
  Fixed apache#1398, revert bugs introduced from apache#1375
  Change Mailing list address
  Merge pull request apache#1242, remove redundant null check.
  Merge pull request apache#1040, refactor: replace some deprecated methods related with jedis.
  Merge pull request apache#1384, fix build string bug.
  Merge pull request apache#1376, do not instantiate load balance if there is no invokers
  Merge pull request apache#1331, add optional parameter to support hessian protocol method overload and request protocol version.
  ...
rolandhe pushed a commit to rolandhe/dubbo that referenced this pull request Sep 9, 2019
…sian protocol method overload and request protocol version.
This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants