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

namespace: create default namespace fallback if not any #65

Merged
merged 7 commits into from
Sep 2, 2022

Conversation

xhebox
Copy link
Collaborator

@xhebox xhebox commented Aug 31, 2022

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

What problem does this PR solve?

Issue Number: close #47

Problem Summary: As title.

What is changed and how it works:

  1. give default fallback on the first run by detecting cfg.Workdir
  2. ./weirctl namespace import conf/namespace if yout need to init something different
  3. ./weirctl namespace del default if you don't want the fallback
  4. "" is renamed to "default". And empty namespace name is forbidden

Check List

Tests

  • Unit test
  • Integration test
  • Manual test (add detailed scripts or steps below)
./bin/weirproxy
./bin/weirctl namespace list // should have this default namespace even without importing
  • 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.

None

@xhebox xhebox requested a review from djshow832 August 31, 2022 03:35
@@ -129,6 +132,20 @@ func NewServer(ctx context.Context, cfg *config.Config, logger *zap.Logger, name
return
}

if errors.Is(dirErr, os.ErrNotExist) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Maybe it's a better idea to check whether there are any namespaces after etcd starts? Is it easy to implement?
work exists doesn't really mean the etcd database exists.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes, we can implement it with an extra RPC.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Why change it back to check the dir?

Copy link
Collaborator Author

@xhebox xhebox Sep 1, 2022

Choose a reason for hiding this comment

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

Because you said, one may don't want the default fallback. IMO, that means the proxy should fail if no namespace matched. But the default fallback works by detecting if there is any namespace. So there is always a default fallback, as long as you started a new TiProxy instance.

I've changed it back to workdir detection it only ran once. Then one should be able to delete the default fallback by weirctl.

But after a second thought, I think we can solve this problem after supporting PD per namespace later. An empty namespace will prevent client connections, and only a non-empty/effective namespace will serve connections. So I changed it back to detect if there is any namespace again.

pkg/server/server.go Outdated Show resolved Hide resolved
@xhebox xhebox changed the title namespace: create default namespace at the first run namespace: create default namespace if no existed namespace at startup Aug 31, 2022
@xhebox xhebox changed the title namespace: create default namespace if no existed namespace at startup namespace: create default namespace fallback at the first run Aug 31, 2022
@xhebox xhebox requested a review from djshow832 August 31, 2022 08:58
pkg/server/server.go Outdated Show resolved Hide resolved
Signed-off-by: xhe <xw897002528@gmail.com>
Signed-off-by: xhe <xw897002528@gmail.com>
Signed-off-by: xhe <xw897002528@gmail.com>
Signed-off-by: xhe <xw897002528@gmail.com>
Signed-off-by: xhe <xw897002528@gmail.com>
Signed-off-by: xhe <xw897002528@gmail.com>
conf/namespace/test_namespace.yaml Show resolved Hide resolved
@@ -129,6 +132,20 @@ func NewServer(ctx context.Context, cfg *config.Config, logger *zap.Logger, name
return
}

if errors.Is(dirErr, os.ErrNotExist) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why change it back to check the dir?

@xhebox xhebox changed the title namespace: create default namespace fallback at the first run namespace: create default namespace fallback if not any Sep 1, 2022
Signed-off-by: xhe <xw897002528@gmail.com>
@djshow832 djshow832 merged commit a4b095d into pingcap:main Sep 2, 2022
@xhebox xhebox deleted the config_5 branch September 9, 2022 02:55
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.

Create a default namespace if the namespace configuration is not specified
2 participants