-
Notifications
You must be signed in to change notification settings - Fork 3.9k
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
cli/start: use the same format for server details in multi-tenant #73676
Conversation
This will be used to report server identifiers in a later commit. Release note: None
NB: there were no tests to assert the output of server details in the multi-tenant case. I'm not adding them in this PR; they will be in #73306 which will be rebased on top. |
a51daed
to
37160f5
Compare
37160f5
to
c631d86
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed 1 of 1 files at r1.
Reviewable status:complete! 1 of 0 LGTMs obtained (waiting on @knz and @stevendanna)
a discussion (no related file):
pkg/cli/start.go, line 909 at r3 (raw file):
// earlier (e.g. above, in the runStart function) because // at this time the address and port have not been resolved yet. sCtx := rpc.MakeSecurityContext(serverCfg.Config, security.ClusterTLSSettings(serverCfg.Settings), roachpb.SystemTenantID)
note to self: It looks like the passed roachpb.SystemTenantID is irrelevant to the PGRUL function that we need this context for, so this is fine for both callers.
pkg/cli/start.go, line 950 at r3 (raw file):
buf.Printf("store[%d]:\t%s\n", i, spec) } buf.Printf("storage engine: \t%s\n", &serverCfg.StorageEngine)
Is this relevant for SQL servers? No harm in including it either way I suppose.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status:
complete! 1 of 0 LGTMs obtained (waiting on @knz and @stevendanna)
pkg/cli/start.go, line 909 at r3 (raw file):
Previously, stevendanna (Steven Danna) wrote…
note to self: It looks like the passed roachpb.SystemTenantID is irrelevant to the PGRUL function that we need this context for, so this is fine for both callers.
Yes, correct - this function creates a client context, which doesn't care about the tenant ID.
pkg/cli/start.go, line 950 at r3 (raw file):
Previously, stevendanna (Steven Danna) wrote…
Is this relevant for SQL servers? No harm in including it either way I suppose.
Yep it is - SQL servers use the storage package for spilling SQL data to disk.
c631d86
to
ab834ab
Compare
TFYR! bors r=stevendanna |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status:
complete! 0 of 0 LGTMs obtained (and 1 stale) (waiting on @knz and @stevendanna)
pkg/cli/start.go, line 950 at r3 (raw file):
Previously, knz (kena) wrote…
Yep it is - SQL servers use the storage package for spilling SQL data to disk.
Ah, right. Thanks.
This ensures that the server details are printed using the same format as regular nodes for multi-tenant SQL servers. Before: ``` SQL server for tenant 123 listening at 127.0.0.1:26258, http at 127.0.0.1:8081 ``` After: ``` CockroachDB SQL server starting at 2021-12-10 12:11:57.684183707 +0000 UTC (took 0.1s) build: CCL v21.2.0-alpha.00000000-6604-g6f243c67e2-dirty @ (go1.17.4) webui: http://kenax:8081 sql: postgresql://root@kenax:26258/defaultdb?sslmode=disable sql (JDBC): jdbc:postgresql://kenax:26258/defaultdb?sslmode=disable&user=root RPC client flags: ./cockroach <client cmd> --host=kenax:26258 --insecure temp dir: /data/home/kena/src/go/src/github.com/cockroachdb/cockroach/mt-data/cockroach-temp981776782 external I/O path: <disabled> store[0]: path=/data/home/kena/src/go/src/github.com/cockroachdb/cockroach/mt-data storage engine: pebble clusterID: 6622219f-89b7-4d0d-919f-22db8fa83dfd tenantID: 123 instanceID: 3 ``` Release note: None
lint failure |
Canceled. |
ab834ab
to
d7f8ebe
Compare
bors r=stevendanna |
I don't remember wanting this, but looks nice nevertheless!
|
Build succeeded: |
cc @tbg who wanted this.
This ensures that the server details are printed using the same format
as regular nodes for multi-tenant SQL servers.
Before:
After:
Release note: None