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

enhance client's connectivity #800

Merged
merged 3 commits into from
Oct 27, 2020
Merged

enhance client's connectivity #800

merged 3 commits into from
Oct 27, 2020

Conversation

beiwei30
Copy link
Member

What this PR does:

when the server is down by accident, we need a mechanism to detect this situation quickly, so that the health check router will not route the request to the problematic provider.

Which issue(s) this PR fixes:

Fixes #

Special notes for your reviewer:

Does this PR introduce a user-facing change?:


@codecov-io
Copy link

Codecov Report

Merging #800 into master will decrease coverage by 0.16%.
The diff coverage is 0.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #800      +/-   ##
==========================================
- Coverage   59.86%   59.70%   -0.17%     
==========================================
  Files         259      259              
  Lines       12751    12758       +7     
==========================================
- Hits         7634     7617      -17     
- Misses       4163     4185      +22     
- Partials      954      956       +2     
Impacted Files Coverage Δ
cluster/router/healthcheck/default_health_check.go 94.11% <0.00%> (-5.89%) ⬇️
protocol/dubbo/dubbo_invoker.go 75.00% <0.00%> (-1.06%) ⬇️
remoting/getty/getty_client.go 39.32% <0.00%> (-1.86%) ⬇️
remoting/getty/pool.go 47.34% <0.00%> (-12.84%) ⬇️
...tocol/rest/server/server_impl/go_restful_server.go 46.51% <0.00%> (-4.66%) ⬇️
cluster/cluster_impl/failback_cluster_invoker.go 78.31% <0.00%> (+2.40%) ⬆️
metadata/report/delegate/delegate_report.go 39.83% <0.00%> (+10.16%) ⬆️

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 a77747e...042387d. Read the comment docs.

Comment on lines 209 to 212
if client == nil || !client.isAvailable() || err != nil {
return false
}
return true
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
if client == nil || !client.isAvailable() || err != nil {
return false
}
return true
return client != nil && client.isAvailable() && err == nil

Copy link
Member Author

Choose a reason for hiding this comment

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

I have changed the implementation to

// isAvailable returns true if the connection is available, or it can be re-established.
func (c *Client) IsAvailable() bool {
	_, _, err := c.selectSession(c.addr)
	return err == nil
}

since we need to re-establish the connection if possible.

@zouyx zouyx added this to the v1.5.4 milestone Oct 22, 2020
@zouyx zouyx added the improve Refactor or improve label Oct 22, 2020
@wenxuwan
Copy link
Member

selectSession function will try to get conn or create new conn with lock, If we just want to check whether the conn of the gettyRPCClientPool is healthy, we can add a flag to reduce the use of locks through atomic operations。

@wenxuwan
Copy link
Member

LGTM

@watermelo watermelo changed the base branch from master to develop October 27, 2020 06:00
Comment on lines 209 to 210
_, _, err := c.selectSession(c.addr)
return err == nil
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
_, _, err := c.selectSession(c.addr)
return err == nil
c, _, err := c.selectSession(c.addr)
return c!=nil && err == nil

Copy link
Member Author

Choose a reason for hiding this comment

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

add this defensive check, pls. check again.

@AlexStocks AlexStocks merged commit 40997f6 into apache:develop Oct 27, 2020
AlexStocks added a commit that referenced this pull request Apr 14, 2021
enhance client's connectivity
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
improve Refactor or improve
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants