config: dynamic proxy config #56

xhebox · 2022-08-24T08:04:30Z

Signed-off-by: xhe xw897002528@gmail.com

What problem does this PR solve?

Issue Number: ref #11

Problem Summary: Dynamic config keep alive and max connections for proxy server.

What is changed and how it works:

In short, added background service for config manager to poll online config regularlly/watch etcd event. It will then be applied to the running server.
API return value minor improvement. Added /api/admin/config/proxy for online proxy config.
Configs have yaml, toml, json all struct tags
errors.Wrap(f) will return nil for nil error
Makefile minor improvement

Check List

Tests

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

./bin/weirproxy ...
echo '{"max_connections": 0}' | ./bin/weirctl config proxy
zero connection error
echo '{"max_connections": 1}' | ./bin/weirctl config proxy

start two mysql client, and one of them will be rejected by the server

No code

Notable changes

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

Release note

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

Support dynamic configuration for some proxy server options

Signed-off-by: xhe <xw897002528@gmail.com>

djshow832

https://github.com/pingcap/TiProxy/blob/a9b15514f5f0bc282b35678df69b0ab07f053b61/cmd/weirctl/util.go#L46
I think it should be for _, i := range rand.Perm(len(bctx.CUrls)) {?

xhebox · 2022-08-24T09:45:29Z

https://github.com/pingcap/TiProxy/blob/a9b15514f5f0bc282b35678df69b0ab07f053b61/cmd/weirctl/util.go#L46

I think it should be for _, i := range rand.Perm(len(bctx.CUrls)) {?

Nope, I am using it as bctx.CUrls[i]. If you like this style, I can change it.

pkg/manager/config/proxy.go

pkg/manager/config/manager.go

djshow832 · 2022-08-24T11:57:18Z

https://github.com/pingcap/TiProxy/blob/a9b15514f5f0bc282b35678df69b0ab07f053b61/cmd/weirctl/util.go#L46

I think it should be for _, i := range rand.Perm(len(bctx.CUrls)) {?

Nope, I am using it as bctx.CUrls[i]. If you like this style, I can change it.

for i := range rand.Perm(len(bctx.CUrls)) {
	req.URL.Host = bctx.CUrls[i]

is just equivalent to

for _, host := bctx.CUrls {
	req.URL.Host = host

because i is always an increasing integer. It's the index of the array rand.Perm(len(bctx.CUrls)).

Co-authored-by: djshow832 <zhangming@pingcap.com>

xhebox · 2022-08-24T12:46:43Z

https://github.com/pingcap/TiProxy/blob/a9b15514f5f0bc282b35678df69b0ab07f053b61/cmd/weirctl/util.go#L46

I think it should be for _, i := range rand.Perm(len(bctx.CUrls)) {?

Nope, I am using it as bctx.CUrls[i]. If you like this style, I can change it.
for i := range rand.Perm(len(bctx.CUrls)) {
	req.URL.Host = bctx.CUrls[i]
is just equivalent to
for _, host := bctx.CUrls {
	req.URL.Host = host
because i is always an increasing integer. It's the index of the array rand.Perm(len(bctx.CUrls)).

Addressed.

Signed-off-by: xhe <xw897002528@gmail.com>

pkg/manager/config/manager.go

pkg/manager/config/manager_test.go

Co-authored-by: djshow832 <zhangming@pingcap.com>

Signed-off-by: xhe <xw897002528@gmail.com>

djshow832 · 2022-08-25T03:41:38Z

pkg/config/proxy.go

 }

 type ConfigManager struct {
-	IgnoreWrongNamespace bool `yaml:"ignore_wrong_namespace"`
+	IgnoreWrongNamespace bool   `yaml:"ignore_wrong_namespace" toml:"ignore_wrong_namespace" json:"ignore_wrong_namespace"`
+	WatchInterval        string `yaml:"watch_interval" toml:"watch_interval" json:"watch_interval"`


I thought about this, it's hard to explain what this really means to the user.
For common cases, the config will be refreshed once updated. This interval is only used for defensive programming.
As for TiDB, the refresh interval for privileges and global variables are not exposed to users either, and nobody complains about it.

We could categorize them into KNOW WHAT YOU ARE DOING options and provide a resonable default. If one don't understand, just don't set.

You can choose to use the default good config, or make one step further into the internal. That is users' freedom. BTW, these are possibly useful for debugging online.

As for TiDB, the refresh interval for privileges and global variables are not exposed to users either, and nobody complains about it.

There is no choice. They don't even know about that. Maybe they have a guess in their mind, and thought it is correct. If we have a documentation said the priv interval is XXX. Maybe someone will complain that it is too short or too long.

As for TiDB, the refresh interval for privileges and global variables are not exposed to users either, and nobody complains about it.

There is no choice. They don't even know about that. Maybe they have a guess in their mind, and thought it is correct. If we have a documentation said the priv interval is XXX. Maybe someone will complain that it is too short or too long.

If we make it as a config, we must document it, and then users are confused and complain. I sometimes encounter the situation where a user saw a config and doesn't understand what it means and then asks us. It just brings us trouble. If we're removing it, it again brings compatibility problems.

I would not consider it a problem. First of all, TiProxy will have much less options. It is much simpler and not that hard to understand.

My point is that documentation of complex options can be highlighted, in another section or another page with a large NOTE. The example for these options can be separated into another file, or commented by default.

If users feel normal options are enough, they surely won't dig deep into these complicated options. Moreover, we recommend you not to use these options unless you understand it. TiDB docs has none of these guidance or classifications. And description for system variables usually do not have any external link to the related page or doc.

pkg/proxy/proxy.go

Signed-off-by: xhe <xw897002528@gmail.com>

config: dynamic proxy config

24236e7

Signed-off-by: xhe <xw897002528@gmail.com>

xhebox requested a review from djshow832 August 24, 2022 08:04

*: revert test changes

318f035

Signed-off-by: xhe <xw897002528@gmail.com>

xhebox mentioned this pull request Aug 24, 2022

Dynamic configurations #57

Closed

util/errors: fix CI

a9b1551

Signed-off-by: xhe <xw897002528@gmail.com>

djshow832 reviewed Aug 24, 2022

View reviewed changes

manager: Update pkg/manager/config/manager.go

efe1b84

Co-authored-by: djshow832 <zhangming@pingcap.com>

xhebox added 4 commits August 24, 2022 20:51

server: zero for unlimited connections

4e2a2ba

Signed-off-by: xhe <xw897002528@gmail.com>

manager: reword

286672b

Signed-off-by: xhe <xw897002528@gmail.com>

weirctl: fix random selector

f218295

Signed-off-by: xhe <xw897002528@gmail.com>

manager: reset interval

3a005bc

Signed-off-by: xhe <xw897002528@gmail.com>

xhebox requested a review from djshow832 August 24, 2022 14:52

*: add and fix test

6b7228e

Signed-off-by: xhe <xw897002528@gmail.com>

xhebox force-pushed the config_2 branch from bcd2ff5 to 6b7228e Compare August 24, 2022 14:53

djshow832 reviewed Aug 25, 2022

View reviewed changes

pkg/manager/config/manager.go Outdated Show resolved Hide resolved

pkg/manager/config/manager.go Outdated Show resolved Hide resolved

pkg/manager/config/manager.go Show resolved Hide resolved

pkg/manager/config/manager_test.go Show resolved Hide resolved

xhebox and others added 4 commits August 25, 2022 11:31

manager: rewor pkg/manager/config/manager.go

1b238c5

Co-authored-by: djshow832 <zhangming@pingcap.com>

*: address comments

f9cd4c6

Signed-off-by: xhe <xw897002528@gmail.com>

manager: add created event

8000

a575477

Signed-off-by: xhe <xw897002528@gmail.com>

manager: no more than polling interval

2c509e6

Signed-off-by: xhe <xw897002528@gmail.com>

xhebox requested a review from djshow832 August 25, 2022 03:47

djshow832 reviewed Aug 25, 2022

View reviewed changes

pkg/proxy/proxy.go Outdated Show resolved Hide resolved

*: fix connections

8aff251

Signed-off-by: xhe <xw897002528@gmail.com>

xhebox requested a review from djshow832 August 25, 2022 06:52

xhebox added 2 commits August 25, 2022 14:55

*: remove useless error

75311ca

Signed-off-by: xhe <xw897002528@gmail.com>

*: revert back the config

a92f27a

Signed-off-by: xhe <xw897002528@gmail.com>

djshow832 approved these changes Aug 26, 2022

View reviewed changes

djshow832 merged commit bacaced into pingcap:main Aug 26, 2022

xhebox deleted the config_2 branch August 26, 2022 03:10

This was referenced Aug 31, 2022

New namespace manager #11

Closed

TiProxy tracking issue #7

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

config: dynamic proxy config #56

config: dynamic proxy config #56

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

config: dynamic proxy config #56

config: dynamic proxy config #56

Uh oh!

Conversation

Uh oh!

What problem does this PR solve?

Check List

Release note

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!