You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@dubbo.apache.org by "tswstarplanet (GitHub)" <gi...@apache.org> on 2018/12/10 14:36:07 UTC
[GitHub] [incubator-dubbo] tswstarplanet opened pull request #2934:
[Dubbo 2682]Optimize the logic of find port of one protocol
## What is the purpose of the change
Now the actual logic of get a port of a protocol is executed in the process of every service that use the same protocol. So we can optimize the logic so that it will only parse once actually.
## Brief changelog
XXXXX
## 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) field 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/2934 ]
This message was relayed via gitbox.apache.org for notifications@dubbo.apache.org
[GitHub] [incubator-dubbo] tswstarplanet commented on pull request
#2934: [Dubbo 2682]Optimize the logic of find port of one protocol
Posted by "tswstarplanet (GitHub)" <gi...@apache.org>.
I don't think this problem just from the user config side. I think if the user does not set the port of the protocol, it means that he does care the port and he just want the application to run correctly. So I think the port of protocol can regards as an inner state that the user can ignore as if the application can run with no problems.
[ Full content available at: https://github.com/apache/incubator-dubbo/pull/2934 ]
This message was relayed via gitbox.apache.org for notifications@dubbo.apache.org
[GitHub] [incubator-dubbo] tswstarplanet commented on pull request
#2934: [Dubbo 2682]Optimize the logic of find port of one protocol
Posted by "tswstarplanet (GitHub)" <gi...@apache.org>.
If the user does not set the port, it of course means that he does not care about the port. It is common sense.
[ Full content available at: https://github.com/apache/incubator-dubbo/pull/2934 ]
This message was relayed via gitbox.apache.org for notifications@dubbo.apache.org
[GitHub] [incubator-dubbo] khanimteyaz commented on pull request
#2934: [Dubbo 2682]Optimize the logic of find port of one protocol
Posted by "khanimteyaz (GitHub)" <gi...@apache.org>.
Can be replace by ternary operator
return portToRegistry !=null ? portToRegistry :portToBind;
[ Full content available at: https://github.com/apache/incubator-dubbo/pull/2934 ]
This message was relayed via gitbox.apache.org for notifications@dubbo.apache.org
[GitHub] [incubator-dubbo] carryxyh closed pull request #2934: [Dubbo
2682]Optimize the logic of find port of one protocol
Posted by "carryxyh (GitHub)" <gi...@apache.org>.
[ pull request closed by carryxyh ]
[ Full content available at: https://github.com/apache/incubator-dubbo/pull/2934 ]
This message was relayed via gitbox.apache.org for notifications@dubbo.apache.org
[GitHub] [incubator-dubbo] tswstarplanet commented on pull request
#2934: [Dubbo 2682]Optimize the logic of find port of one protocol
Posted by "tswstarplanet (GitHub)" <gi...@apache.org>.
I think common users of dubbo don’t care what class represents the <dubbo:protocol>, and they don’t have to know how dubbo run. If they don’t config a port clearly, they may or may not know dubbo will pick an available port for them. But I think it’s not important that the common users do know or do not know. They can only care about if dubbo run correctly. The inner running mechanism can be transparent to the common users.
[ Full content available at: https://github.com/apache/incubator-dubbo/pull/2934 ]
This message was relayed via gitbox.apache.org for notifications@dubbo.apache.org
[GitHub] [incubator-dubbo] tswstarplanet commented on pull request
#2934: [Dubbo 2682]Optimize the logic of find port of one protocol
Posted by "tswstarplanet (GitHub)" <gi...@apache.org>.
I think current code is like that we have known the result of one computing problem after we solved it first time. But when we need the result, we still try to solve the problem once by once, but not to get the result directly. But the problem and the result does not change always.
[ Full content available at: https://github.com/apache/incubator-dubbo/pull/2934 ]
This message was relayed via gitbox.apache.org for notifications@dubbo.apache.org
[GitHub] [incubator-dubbo] khanimteyaz commented on pull request
#2934: [Dubbo 2682]Optimize the logic of find port of one protocol
Posted by "khanimteyaz (GitHub)" <gi...@apache.org>.
method name indicate a search operation based on method inputs, should we modify input object here?
[ Full content available at: https://github.com/apache/incubator-dubbo/pull/2934 ]
This message was relayed via gitbox.apache.org for notifications@dubbo.apache.org
[GitHub] [incubator-dubbo] carryxyh commented on pull request #2934:
[Dubbo 2682]Optimize the logic of find port of one protocol
Posted by "carryxyh (GitHub)" <gi...@apache.org>.
I don't think this is a good method name.
When the port is missed in config, it will be found in system or protocol instance or somewhere.
[ Full content available at: https://github.com/apache/incubator-dubbo/pull/2934 ]
This message was relayed via gitbox.apache.org for notifications@dubbo.apache.org
[GitHub] [incubator-dubbo] tswstarplanet commented on pull request
#2934: [Dubbo 2682]Optimize the logic of find port of one protocol
Posted by "tswstarplanet (GitHub)" <gi...@apache.org>.
'Once by once' that I said means that when every <dubbo:service> that use the same protocol export, it will execute the same logic once.
[ Full content available at: https://github.com/apache/incubator-dubbo/pull/2934 ]
This message was relayed via gitbox.apache.org for notifications@dubbo.apache.org
[GitHub] [incubator-dubbo] carryxyh commented on pull request #2934:
[Dubbo 2682]Optimize the logic of find port of one protocol
Posted by "carryxyh (GitHub)" <gi...@apache.org>.
` I think if the user does not set the port of the protocol, it means that he does care the port and he just want the application to run correctly.`
I agree. But this doesn't mean that u can set a default value. U need a cache, cache is just cache, u can keep it another way but not in user config.
[ Full content available at: https://github.com/apache/incubator-dubbo/pull/2934 ]
This message was relayed via gitbox.apache.org for notifications@dubbo.apache.org
[GitHub] [incubator-dubbo] carryxyh commented on pull request #2934:
[Dubbo 2682]Optimize the logic of find port of one protocol
Posted by "carryxyh (GitHub)" <gi...@apache.org>.
I only have one problem. We can use serviceConfig and protocolConfig to publish the service right? If we accept your pr, the user does not set the port of protocolConfig before the release. After the release, protocolConfig has a port. How do we explain this to user that protocolConfig auto set a port?
[ Full content available at: https://github.com/apache/incubator-dubbo/pull/2934 ]
This message was relayed via gitbox.apache.org for notifications@dubbo.apache.org
[GitHub] [incubator-dubbo] tswstarplanet commented on pull request
#2934: [Dubbo 2682]Optimize the logic of find port of one protocol
Posted by "tswstarplanet (GitHub)" <gi...@apache.org>.
I just think that current logic is just that it does duplicate work too many times. We can get available port from system properties, dubbo config properties or the config of xml. But it does not have to do the same thing in every dubbo service export process. It seems redundant
[ Full content available at: https://github.com/apache/incubator-dubbo/pull/2934 ]
This message was relayed via gitbox.apache.org for notifications@dubbo.apache.org
[GitHub] [incubator-dubbo] codecov-io commented on issue #2934:
[Dubbo 2682]Optimize the logic of find port of one protocol
Posted by "codecov-io (GitHub)" <gi...@apache.org>.
# [Codecov](https://codecov.io/gh/apache/incubator-dubbo/pull/2934?src=pr&el=h1) Report
> Merging [#2934](https://codecov.io/gh/apache/incubator-dubbo/pull/2934?src=pr&el=desc) into [master](https://codecov.io/gh/apache/incubator-dubbo/commit/e508ad467f94e853885fdf47a74d312eb5c6001b?src=pr&el=desc) will **increase** coverage by `0.03%`.
> The diff coverage is `81.81%`.
[](https://codecov.io/gh/apache/incubator-dubbo/pull/2934?src=pr&el=tree)
```diff
@@ Coverage Diff @@
## master #2934 +/- ##
==========================================
+ Coverage 63.76% 63.79% +0.03%
==========================================
Files 578 578
Lines 25954 25959 +5
Branches 4543 4543
==========================================
+ Hits 16549 16560 +11
+ Misses 7233 7229 -4
+ Partials 2172 2170 -2
```
| [Impacted Files](https://codecov.io/gh/apache/incubator-dubbo/pull/2934?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [...in/java/org/apache/dubbo/config/ServiceConfig.java](https://codecov.io/gh/apache/incubator-dubbo/pull/2934/diff?src=pr&el=tree#diff-ZHViYm8tY29uZmlnL2R1YmJvLWNvbmZpZy1hcGkvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL2R1YmJvL2NvbmZpZy9TZXJ2aWNlQ29uZmlnLmphdmE=) | `54.91% <81.81%> (+0.29%)` | :arrow_up: |
| [...dubbo/rpc/protocol/dubbo/CallbackServiceCodec.java](https://codecov.io/gh/apache/incubator-dubbo/pull/2934/diff?src=pr&el=tree#diff-ZHViYm8tcnBjL2R1YmJvLXJwYy1kdWJiby9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvZHViYm8vcnBjL3Byb3RvY29sL2R1YmJvL0NhbGxiYWNrU2VydmljZUNvZGVjLmphdmE=) | `77.2% <0%> (-2.21%)` | :arrow_down: |
| [...dubbo/remoting/exchange/support/DefaultFuture.java](https://codecov.io/gh/apache/incubator-dubbo/pull/2934/diff?src=pr&el=tree#diff-ZHViYm8tcmVtb3RpbmcvZHViYm8tcmVtb3RpbmctYXBpL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9kdWJiby9yZW1vdGluZy9leGNoYW5nZS9zdXBwb3J0L0RlZmF1bHRGdXR1cmUuamF2YQ==) | `67.78% <0%> (-2.02%)` | :arrow_down: |
| [...apache/dubbo/rpc/protocol/dubbo/DubboProtocol.java](https://codecov.io/gh/apache/incubator-dubbo/pull/2934/diff?src=pr&el=tree#diff-ZHViYm8tcnBjL2R1YmJvLXJwYy1kdWJiby9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvZHViYm8vcnBjL3Byb3RvY29sL2R1YmJvL0R1YmJvUHJvdG9jb2wuamF2YQ==) | `65.83% <0%> (-0.84%)` | :arrow_down: |
| [.../apache/dubbo/remoting/transport/AbstractPeer.java](https://codecov.io/gh/apache/incubator-dubbo/pull/2934/diff?src=pr&el=tree#diff-ZHViYm8tcmVtb3RpbmcvZHViYm8tcmVtb3RpbmctYXBpL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9kdWJiby9yZW1vdGluZy90cmFuc3BvcnQvQWJzdHJhY3RQZWVyLmphdmE=) | `63.04% <0%> (ø)` | :arrow_up: |
| [...pache/dubbo/registry/support/AbstractRegistry.java](https://codecov.io/gh/apache/incubator-dubbo/pull/2934/diff?src=pr&el=tree#diff-ZHViYm8tcmVnaXN0cnkvZHViYm8tcmVnaXN0cnktYXBpL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9kdWJiby9yZWdpc3RyeS9zdXBwb3J0L0Fic3RyYWN0UmVnaXN0cnkuamF2YQ==) | `82.08% <0%> (+0.74%)` | :arrow_up: |
| [...he/dubbo/registry/multicast/MulticastRegistry.java](https://codecov.io/gh/apache/incubator-dubbo/pull/2934/diff?src=pr&el=tree#diff-ZHViYm8tcmVnaXN0cnkvZHViYm8tcmVnaXN0cnktbXVsdGljYXN0L3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9kdWJiby9yZWdpc3RyeS9tdWx0aWNhc3QvTXVsdGljYXN0UmVnaXN0cnkuamF2YQ==) | `65.51% <0%> (+1.72%)` | :arrow_up: |
| [...he/dubbo/remoting/transport/netty/NettyServer.java](https://codecov.io/gh/apache/incubator-dubbo/pull/2934/diff?src=pr&el=tree#diff-ZHViYm8tcmVtb3RpbmcvZHViYm8tcmVtb3RpbmctbmV0dHkvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL2R1YmJvL3JlbW90aW5nL3RyYW5zcG9ydC9uZXR0eS9OZXR0eVNlcnZlci5qYXZh) | `71.42% <0%> (+3.57%)` | :arrow_up: |
| [...onfig/spring/extension/SpringExtensionFactory.java](https://codecov.io/gh/apache/incubator-dubbo/pull/2934/diff?src=pr&el=tree#diff-ZHViYm8tY29uZmlnL2R1YmJvLWNvbmZpZy1zcHJpbmcvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL2R1YmJvL2NvbmZpZy9zcHJpbmcvZXh0ZW5zaW9uL1NwcmluZ0V4dGVuc2lvbkZhY3RvcnkuamF2YQ==) | `84.61% <0%> (+7.69%)` | :arrow_up: |
| [.../remoting/transport/netty4/NettyClientHandler.java](https://codecov.io/gh/apache/incubator-dubbo/pull/2934/diff?src=pr&el=tree#diff-ZHViYm8tcmVtb3RpbmcvZHViYm8tcmVtb3RpbmctbmV0dHk0L3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9kdWJiby9yZW1vdGluZy90cmFuc3BvcnQvbmV0dHk0L05ldHR5Q2xpZW50SGFuZGxlci5qYXZh) | `86.11% <0%> (+11.11%)` | :arrow_up: |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/incubator-dubbo/pull/2934?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/2934?src=pr&el=footer). Last update [e508ad4...2d18b2b](https://codecov.io/gh/apache/incubator-dubbo/pull/2934?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/2934 ]
This message was relayed via gitbox.apache.org for notifications@dubbo.apache.org
[GitHub] [incubator-dubbo] khanimteyaz commented on pull request
#2934: [Dubbo 2682]Optimize the logic of find port of one protocol
Posted by "khanimteyaz (GitHub)" <gi...@apache.org>.
method name (getPortFromProtocol) indicate a get operation here, should we change the state of input parameter?
[ Full content available at: https://github.com/apache/incubator-dubbo/pull/2934 ]
This message was relayed via gitbox.apache.org for notifications@dubbo.apache.org
[GitHub] [incubator-dubbo] tswstarplanet commented on pull request
#2934: [Dubbo 2682]Optimize the logic of find port of one protocol
Posted by "tswstarplanet (GitHub)" <gi...@apache.org>.
If the port is strongly related to the protocol, can we move the find port logic to the process of generating the protocol config bean ? It can not only reduce the duplicate execution of find port logic, but also can present a more clear logic.
[ Full content available at: https://github.com/apache/incubator-dubbo/pull/2934 ]
This message was relayed via gitbox.apache.org for notifications@dubbo.apache.org
[GitHub] [incubator-dubbo] carryxyh commented on pull request #2934:
[Dubbo 2682]Optimize the logic of find port of one protocol
Posted by "carryxyh (GitHub)" <gi...@apache.org>.
Seems like most time we just have one protocol. So it is not 'once by once' like u said.
ProtocolConfig's port is set by user. U can fix it by other way, but not to change the user config.
[ Full content available at: https://github.com/apache/incubator-dubbo/pull/2934 ]
This message was relayed via gitbox.apache.org for notifications@dubbo.apache.org
[GitHub] [incubator-dubbo] tswstarplanet commented on pull request
#2934: [Dubbo 2682]Optimize the logic of find port of one protocol
Posted by "tswstarplanet (GitHub)" <gi...@apache.org>.
If the port is strongly related to the protocol, can we move the find port logic to the process of generate the protocol config bean ? It can not only reduce the duplicate execution of find port logic, but also can present a more clear logic.
[ Full content available at: https://github.com/apache/incubator-dubbo/pull/2934 ]
This message was relayed via gitbox.apache.org for notifications@dubbo.apache.org
[GitHub] [incubator-dubbo] tswstarplanet commented on pull request
#2934: [Dubbo 2682]Optimize the logic of find port of one protocol
Posted by "tswstarplanet (GitHub)" <gi...@apache.org>.
The change of the state of the input parameter is used to cached the parsed port in protocol config bean, so that the subsequent accesses of port of a protocol can retrieve the port from the protocol config bean directly. Or the find port logic will be executed during the exporting process of every <dubbo:service>
[ Full content available at: https://github.com/apache/incubator-dubbo/pull/2934 ]
This message was relayed via gitbox.apache.org for notifications@dubbo.apache.org