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

router, backend: retry connecting to different backends #171

Merged
merged 4 commits into from
Jan 5, 2023

Conversation

djshow832
Copy link
Collaborator

@djshow832 djshow832 commented Jan 4, 2023

What problem does this PR solve?

Issue Number: ref #168

Problem Summary:
To adapt to the serverless tier, retry connecting to different backends one by one.
Note that this PR has not adapted to the dedicated tier yet.

What is changed and how it works:

  • Router.Route() returns BackendSelector now, which returns a backend each time for calling Next().
  • BackendConnMgr tries connecting to the backends at max 5 seconds.

Check List

Tests

  • Unit test
  • Integration test
  • Manual test (add detailed scripts or steps below)
  • No code

Notable changes

  • Has configuration change
  • Has HTTP API interfaces change (Don't forget to add the declarative for API)
  • Has tiproxyctl change
  • Other user behavior changes

Release note

Please refer to Release Notes Language Style Guide to write a quality release note.

None

@djshow832 djshow832 requested a review from xhebox January 4, 2023 12:55
Copy link
Collaborator

@xhebox xhebox left a comment

Choose a reason for hiding this comment

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

Rest LGTM

pkg/manager/router/router.go Outdated Show resolved Hide resolved
pkg/proxy/backend/backend_conn_mgr.go Outdated Show resolved Hide resolved
pkg/proxy/backend/backend_conn_mgr.go Outdated Show resolved Hide resolved
pkg/manager/router/backend_selector.go Show resolved Hide resolved
pkg/proxy/backend/backend_conn_mgr_test.go Outdated Show resolved Hide resolved
pkg/proxy/backend/backend_conn_mgr_test.go Show resolved Hide resolved
pkg/proxy/backend/backend_conn_mgr_test.go Outdated Show resolved Hide resolved
@@ -160,7 +161,7 @@ func (auth *Authenticator) handshakeFirstTime(logger *zap.Logger, clientIO *pnet
auth.attrs = resp.Attrs

// In case of testing, backendIO is passed manually that we don't want to bother with the routing logic.
backendIO, err := getBackendIO(auth, auth, resp)
backendIO, err := getBackendIO(auth, auth, resp, 5*time.Second)
Copy link
Collaborator

Choose a reason for hiding this comment

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

There is a constant, DialTimeout in backend_conn.go.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It's different. The one passed to getBackendIO is the total timeout for all retries.
BTW, DialTimeout is also 5 seconds. Maybe it should be less?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think both.I think that DialTimeout should be less, but backoff duration should be larger, e.g. three or four times of DialTimeout.

@xhebox xhebox merged commit 66e47e3 into pingcap:main Jan 5, 2023
@djshow832 djshow832 deleted the retry_conn branch January 6, 2023 02:00
xhebox pushed a commit to xhebox/TiProxy that referenced this pull request Mar 7, 2023
xhebox pushed a commit to xhebox/TiProxy that referenced this pull request Mar 13, 2023
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.

2 participants