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: support fetching backend list from external interface #135

Merged
merged 3 commits into from
Nov 24, 2022

Conversation

djshow832
Copy link
Collaborator

@djshow832 djshow832 commented Nov 23, 2022

What problem does this PR solve?

Issue Number: close None

Problem Summary:

In the serverless tier, the gateway offers an interface to query the backend list.

What is changed and how it works:

  • Add an interface BackendFetcher
  • There are 3 implementations of BackendFetcher: StaticFetcher, PDFetcher, and ExternalFetcher.

Some TODOs in next PRs:

  • Not always init an ETCD client (TBD)
  • Update router
  • Stop the observer when there's no connections

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

}

// ExternalFetcher fetches backend list from a given callback.
type ExternalFetcher struct {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why can not let they pass an implementation of interface BackendFetcher?

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 OK to pass a BackendFetcher from the Router, but it exposes map[string]*BackendInfo.
But it's not easy to pass it from the gateway. The fetcher requires a token as the parameter, and the router can wrap it.

Copy link
Collaborator

Choose a reason for hiding this comment

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

But it's not easy to pass it from the gateway. The fetcher requires a token as the parameter, and the router can wrap it.

I don't understand. It is completely possible to do:

type gatewayFetcher struct {
  token string
}

func (f *gatewayFetcher) GetBackendList(context.Context) map[string]*BackendInfo {
	...
}

The only imperfect is that we exposed BackendInfo.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Updated, the BackendObserver receives a customFetcher. But I still think the fetcher should be passed from the router because of the token parameter.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

type gatewayFetcher struct {
  token string
}

This is different from the interface we negotiated with them. This just makes things complicated. Besides offering a GetBackends, they have to also create a fetcher.

Copy link
Collaborator

Choose a reason for hiding this comment

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

This is different from the interface we negotiated with them. This just makes things complicated. Besides offering a GetBackends, they have to also create a fetcher.

If the only method of the interface is GetBackendList(context.Context) map[string]*BackendInfo, with all BackendInfo an empty struct, what is the difference?

Three lines of code to declare gatewayFetcher type? They will need to store that token somewhere anyway, for example, a variable captured by the lambda function.

If we want to support, grpc pushing/notification of backends changes, BackendFetcher interface will change somehow, i.e. not just a fetcher. It is likely that ExternalFetcher will not work anymore.

@xhebox xhebox merged commit 2efc1db into pingcap:main Nov 24, 2022
@djshow832 djshow832 deleted the observer branch November 24, 2022 08:37
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