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

Ftr: Use invoker with same ip as client first. #1023

Merged
merged 22 commits into from Feb 25, 2021

Conversation

LaurenceLiZhixin
Copy link
Contributor

What this PR does:
Add feature :
After client gets invoker lists from registry, if there is provider with the same ip as client, client would use the same ip first, which is not using wide network and faster.
If it not exists the same ip provider, it would handle as normal way.

Which issue(s) this PR fixes:

Fixes #

Special notes for your reviewer:
I add a router named selfDiscovery, which contains the router logic above.

Does this PR introduce a user-facing change?:


@codecov-io
Copy link

codecov-io commented Jan 27, 2021

Codecov Report

Merging #1023 (152d4dd) into develop (d2a40aa) will decrease coverage by 0.10%.
The diff coverage is 75.00%.

Impacted file tree graph

@@             Coverage Diff             @@
##           develop    #1023      +/-   ##
===========================================
- Coverage    59.49%   59.38%   -0.11%     
===========================================
  Files          261      263       +2     
  Lines        12950    12988      +38     
===========================================
+ Hits          7704     7713       +9     
- Misses        4273     4301      +28     
- Partials       973      974       +1     
Impacted Files Coverage Δ
cluster/router/self/factory.go 66.66% <66.66%> (ø)
cluster/router/self/self_priority_route.go 76.00% <76.00%> (ø)
registry/kubernetes/registry.go 51.11% <0.00%> (-8.89%) ⬇️
remoting/kubernetes/registry_controller.go 46.12% <0.00%> (-3.68%) ⬇️
...rotocol/protocolwrapper/protocol_filter_wrapper.go 48.14% <0.00%> (-1.86%) ⬇️
remoting/getty/getty_client.go 43.02% <0.00%> (-1.29%) ⬇️
remoting/kubernetes/watch.go 76.92% <0.00%> (ø)
remoting/kubernetes/listener.go 50.52% <0.00%> (ø)
... and 1 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 d2a40aa...152d4dd. Read the comment docs.

@LaurenceLiZhixin LaurenceLiZhixin changed the title Ftr/self discovery: Use invoker with same ip as client first. Ftr: Use invoker with same ip as client first. Jan 27, 2021
* limitations under the License.
*/

package self_disc
Copy link
Member

Choose a reason for hiding this comment

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

dont use _ in package name. I think can change to self

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed

Copy link
Contributor

Choose a reason for hiding this comment

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

I think consistency for directory name and package name will not lead to misunderstanding.

And directory name should not be ‘aB’

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 think consistency for directory name and package name will not lead to misunderstanding.

And directory name should not be ‘aB’

fixed

)

const (
selfDesc = "self-desc"
Copy link
Member

Choose a reason for hiding this comment

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

Can you use common.SelfDiscoveryRouterName ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

common.SelfDiscoveryRouterName is to find out registered router.
selfDesc is to find the bitmap of target router in cache.
In order to be consistent with the code design of other routers, I think your suggesting way can be confusing.

@@ -176,6 +176,8 @@ github.com/dubbogo/go-zookeeper v1.0.2/go.mod h1:fn6n2CAEer3novYgk9ULLwAjuV8/g4D
github.com/dubbogo/gost v1.9.0/go.mod h1:pPTjVyoJan3aPxBPNUX0ADkXjPibLo+/Ib0/fADXSG8=
github.com/dubbogo/gost v1.10.1 h1:39kF9Cd5JOiMpmwG6dX1/aLWNFqFv9gHp8HrhzMmjLY=
github.com/dubbogo/gost v1.10.1/go.mod h1:+mQGS51XQEUWZP2JeGZTxJwipjRKtJO7Tr+FOg+72rI=
github.com/dubbogo/gost v1.11.0 h1:9KtyWQz1gMlAfwzen5iyhMdoe08SPBBUVhco4rdgJ9I=
github.com/dubbogo/gost v1.11.0/go.mod h1:w8Yw29eDWtRVo3tx9nPpHkNZnOi4SRx1fZf7eVlAAU4=
Copy link
Contributor

Choose a reason for hiding this comment

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

using go mod tidy to delete the useless gost version.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed

* limitations under the License.
*/

package self
Copy link
Contributor

Choose a reason for hiding this comment

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

pls change the pkg name

selectedInvokers := utils.JoinIfNotEqual(addrPool[selfPriority], invokers)
// If all invokers are considered not match, downgrade to all invoker
if selectedInvokers.IsEmpty() {
logger.Warnf(" Now all invokers are not match, so downgraded to all! Service: [%s]", url.ServiceKey())
Copy link
Contributor

Choose a reason for hiding this comment

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

maybe it should be logger.Debugf


// Priority
func (r *SelfPriorityRouter) Priority() int64 {
return 0
Copy link
Contributor

Choose a reason for hiding this comment

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

the priority should be latest

@AlexStocks
Copy link
Contributor

pls change the target branch to 1.5

@AlexStocks AlexStocks merged commit 15ca488 into apache:develop Feb 25, 2021
@cityiron cityiron added this to the v1.5.7 milestone Feb 25, 2021
AlexStocks added a commit that referenced this pull request Apr 14, 2021
Ftr:  Use invoker with same ip as client first.
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.

None yet

7 participants