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

*: support clustering #112

Merged
merged 2 commits into from
Oct 11, 2022
Merged

*: support clustering #112

merged 2 commits into from
Oct 11, 2022

Conversation

xhebox
Copy link
Collaborator

@xhebox xhebox commented Oct 11, 2022

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

What problem does this PR solve?

Issue Number: ref #69

Problem Summary: Support a cluster of tiproxy nodes.

What is changed and how it works: Add five new cmd flags.

Check List

Tests

  • Unit test
  • Integration test
  • Manual test (add detailed scripts or steps below)
n1: 3080,3081,6000
n2: 4080,4081,7000

 rm -rf work && ./bin/tiproxy --node_name=n1 --bootstrap_clusters "n2=127.0.0.1:4081" --config conf/proxy.yaml
rm -rf work2 && ./bin/tiproxy --node_name=n2 --bootstrap_clusters "n1=127.0.0.1:3081" --config conf2/proxy.yaml

After bootstraping

stop one node and restart it by ./bin/tiproxy  --config conf/proxy.yaml
  • 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

Signed-off-by: xhe <xw897002528@gmail.com>
@xhebox xhebox mentioned this pull request Oct 11, 2022
9 tasks
@xhebox xhebox requested a review from djshow832 October 11, 2022 05:20
Comment on lines +263 to +275
cnt := 0
if len(cfg.Cluster.BootstrapClusters) != 0 {
cnt++
}
if cfg.Cluster.BootstrapDurl != "" {
cnt++
}
if cfg.Cluster.BootstrapDdns != "" {
cnt++
}
if cnt > 1 {
return nil, errors.New("you can only pass one 'bootstrap_xxx' to bootstrap the node, or leave them empty to start a single-node cluster")
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why make it so complicated, are other components also like this?

Copy link
Collaborator Author

@xhebox xhebox Oct 11, 2022

Choose a reason for hiding this comment

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

This is common for etcd and other cloud native applications. Other pingcap components relies on the central PD(they only need PD).

PD only provide bootstrapClusters options, and a sugar option of bootstrapCluster by -join xxx:2380 if you don't want to write all clusters by hands. I have no plan to include that sugar option in this PR.

Honestly, discovery or dns is more convenient.

To use --initial-clusters, you need to specify all nodes. To use -join, the cluster must be bootstraped already. Whatever you need to pass different options fot different PD and probably follow some orders(-join).

Operator has implemented some very complex code. But I hope I can start all tiproxy nodes by some etcd discovery like ./bin/tiproxy --bootstrap-discovery-etcd xxxx.

Copy link
Collaborator

Choose a reason for hiding this comment

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

In my option, there's even no need to merge this PR, right? Proxies don't need to know each other, except for updating configurations. But updating configurations can be achieved by configuring the etcd address.

Copy link
Collaborator Author

@xhebox xhebox Oct 11, 2022

Choose a reason for hiding this comment

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

We have no etcd address. Tiproxy itself is a etcd server.

Unless you want a 'master tiproxy node' explicitly, again you need you need to first bootstrap a central single-node cluster. Other nodes need to -central-etcd xxx.

Or if you want to propagate global configurations to every node manually by tiproxyctl. For example, tls certs, namespace/routing/filter rules.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Only if you want to remove etcd from tiproxy, and relies on an external ETCD. I think it makes things complicated. If you have PD, you don't want another etcd. If you use tiproxy with static servers, you need another etcd.

pkg/server/server.go Outdated Show resolved Hide resolved
Signed-off-by: xhe <xw897002528@gmail.com>
@djshow832 djshow832 merged commit 458b03c into pingcap:main Oct 11, 2022
@xhebox xhebox deleted the t branch October 26, 2022 06:22
xhebox added a commit to xhebox/TiProxy that referenced this pull request Mar 7, 2023
xhebox added 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