Skip to content

Commit

Permalink
cli,server: rename "InProcessTenant" to "UseServerController"
Browse files Browse the repository at this point in the history
In `cockroach demo` and unit tests, all tenant servers regardless of
origin are "in process". The flag was really pointing to the
server controller, not the process-ness. So this patch renames
it accordingly.

Consequently, the corresponding `cockroach demo` flag becomes
`--disable-server-controller`.

Release note: None
  • Loading branch information
knz committed Jan 6, 2023
1 parent fa71be3 commit 3c2bff1
Show file tree
Hide file tree
Showing 7 changed files with 23 additions and 24 deletions.
6 changes: 3 additions & 3 deletions pkg/base/test_server_args.go
Original file line number Diff line number Diff line change
Expand Up @@ -340,7 +340,7 @@ type TestTenantArgs struct {
// Cockroach Labs. Should remain disabled during unit testing.
StartDiagnosticsReporting bool

// InProcessTenant is used to initialize tenants through the
// kv server's `serverController` in the same process.
InProcessTenant bool
// UseServerController tells testserver.StartTenant() to use
// its serverController to start the secondary tenant.
UseServerController bool
}
12 changes: 5 additions & 7 deletions pkg/cli/cliflags/flags.go
Original file line number Diff line number Diff line change
Expand Up @@ -1343,16 +1343,14 @@ More information about the geo-partitioned replicas topology can be found at:
Description: `
If set, cockroach demo will start separate in-memory KV and SQL servers in multi-tenancy mode.
The SQL shell will be connected to the first tenant, and can be switched between tenants
and the system tenant using the \connect command. By default the tenant will be initialized
in-process. Use --simulate-separate-tenant-process to emulate a separate tenant process.
`,
and the system tenant using the \connect command.`,
}

DemoInProcessTenant = FlagInfo{
Name: "in-process-tenant",
DemoDisableServerController = FlagInfo{
Name: "disable-server-controller",
Description: `
If set, and --multitenant is set to true, the tenant process will be started
in-process.`,
If set, the server controller will not be used to start secondary
tenant servers.`,
}

DemoNoLicense = FlagInfo{
Expand Down
2 changes: 1 addition & 1 deletion pkg/cli/context.go
Original file line number Diff line number Diff line change
Expand Up @@ -618,7 +618,7 @@ func setDemoContextDefaults() {
demoCtx.HTTPPort, _ = strconv.Atoi(base.DefaultHTTPPort)
demoCtx.WorkloadMaxQPS = 25
demoCtx.Multitenant = true
demoCtx.InProcessTenant = true
demoCtx.DisableServerController = false
demoCtx.DefaultEnableRangefeeds = true

demoCtx.pidFile = ""
Expand Down
5 changes: 3 additions & 2 deletions pkg/cli/democluster/context.go
Original file line number Diff line number Diff line change
Expand Up @@ -107,8 +107,9 @@ type Context struct {
// out enabled.
DefaultEnableRangefeeds bool

// InProcessTenant is true if we want to emulate an in-process tenant.
InProcessTenant bool
// DisableServerController is true if we want to avoid the server
// controller to instantiate tenant secondary servers.
DisableServerController bool
}

// IsInteractive returns true if the demo cluster configuration
Expand Down
16 changes: 8 additions & 8 deletions pkg/cli/democluster/demo_cluster.go
Original file line number Diff line number Diff line change
Expand Up @@ -218,7 +218,7 @@ func NewDemoCluster(
// tenant.
// Note: this logic can be removed once we use a single
// listener for HTTP and SQL.
if !c.demoCtx.InProcessTenant {
if c.demoCtx.DisableServerController {
c.httpFirstPort += c.demoCtx.NumNodes
}
c.sqlFirstPort += c.demoCtx.NumNodes
Expand Down Expand Up @@ -430,7 +430,7 @@ func (c *transientCluster) Start(ctx context.Context) (err error) {
DisableCreateTenant: !createTenant,
TenantName: roachpb.TenantName("demo-tenant"),
TenantID: roachpb.MustMakeTenantID(secondaryTenantID),
InProcessTenant: c.demoCtx.InProcessTenant,
UseServerController: !c.demoCtx.DisableServerController,
TestingKnobs: base.TestingKnobs{
Server: &server.TestingKnobs{
ContextTestingKnobs: rpc.ContextTestingKnobs{
Expand All @@ -442,7 +442,7 @@ func (c *transientCluster) Start(ctx context.Context) (err error) {
}

var tenantStopper *stop.Stopper
if !c.demoCtx.InProcessTenant {
if c.demoCtx.DisableServerController {
tenantStopper = stop.NewStopper()
args.Stopper = tenantStopper
args.ForceInsecure = c.demoCtx.Insecure
Expand All @@ -455,9 +455,9 @@ func (c *transientCluster) Start(ctx context.Context) (err error) {
}

ts, err := c.servers[i].StartTenant(ctx, args)
if !c.demoCtx.InProcessTenant {
// InProcessTenant means that the server controller is
// already taking care of shutdown.
if c.demoCtx.DisableServerController {
// If we use the server controller, it is already taking
// care of shutdown.
c.stopper.AddCloser(stop.CloserFn(func() {
stopCtx := context.Background()
if ts != nil {
Expand Down Expand Up @@ -858,7 +858,7 @@ func (demoCtx *Context) testServerArgsForTransientCluster(
args.SQLAddr = fmt.Sprintf("127.0.0.1:%d", sqlPort)
args.HTTPAddr = fmt.Sprintf("127.0.0.1:%d", httpPort)

if demoCtx.InProcessTenant {
if !demoCtx.DisableServerController {
// The code in NewDemoCluster put the KV ports higher
// so we need to subtract the number of nodes to get
// back to the "good" ports.
Expand Down Expand Up @@ -1820,7 +1820,7 @@ func (c *transientCluster) ListDemoNodes(w, ew io.Writer, justOne, verbose bool)
// Connection parameters for the system tenant follow.

uiURL := s.Cfg.AdminURL()
if q := uiURL.Query(); c.demoCtx.Multitenant && c.demoCtx.InProcessTenant && !q.Has(server.TenantNameParamInQueryURL) {
if q := uiURL.Query(); c.demoCtx.Multitenant && !c.demoCtx.DisableServerController && !q.Has(server.TenantNameParamInQueryURL) {
q.Add(server.TenantNameParamInQueryURL, catconstants.SystemTenantName)
uiURL.RawQuery = q.Encode()
}
Expand Down
4 changes: 2 additions & 2 deletions pkg/cli/flags.go
Original file line number Diff line number Diff line change
Expand Up @@ -795,11 +795,11 @@ func init() {
cliflagcfg.BoolFlag(f, &demoCtx.DefaultEnableRangefeeds, cliflags.DemoEnableRangefeeds)

cliflagcfg.BoolFlag(f, &demoCtx.Multitenant, cliflags.DemoMultitenant)
cliflagcfg.BoolFlag(f, &demoCtx.InProcessTenant, cliflags.DemoInProcessTenant)
cliflagcfg.BoolFlag(f, &demoCtx.DisableServerController, cliflags.DemoDisableServerController)
// TODO(knz): Currently the multitenant UX for 'demo' is not
// satisfying for end-users. Let's not advertise it too much.
_ = f.MarkHidden(cliflags.DemoMultitenant.Name)
_ = f.MarkHidden(cliflags.DemoInProcessTenant.Name)
_ = f.MarkHidden(cliflags.DemoDisableServerController.Name)

cliflagcfg.BoolFlag(f, &demoCtx.SimulateLatency, cliflags.Global)
// We also support overriding the GEOS library path for 'demo'.
Expand Down
2 changes: 1 addition & 1 deletion pkg/server/testserver.go
Original file line number Diff line number Diff line change
Expand Up @@ -831,7 +831,7 @@ func (ts *TestServer) StartTenant(
}
}

if params.InProcessTenant {
if params.UseServerController {
onDemandServer, err := ts.serverController.getOrCreateServer(ctx, params.TenantName)
if err != nil {
return nil, err
Expand Down

0 comments on commit 3c2bff1

Please sign in to comment.