Jackie-Jiang opened a new pull request, #19393: URL: https://github.com/apache/pinot/pull/19393
## Summary `PinotConfigUtils#generateControllerConf` always wrote `cluster.tenant.isolation.enable` into the config it generates from individual options. The generated config therefore had no way to express "the caller did not choose a value", and any default a `ControllerStarter` implementation supplies from `applyCustomConfigs` was dead on that path — the key was already present, so a set-if-missing default could never win. The value is now a nullable `Boolean`, and the key is written only when a caller passes one. ### Changes - `PinotConfigUtils#generateControllerConf` takes `@Nullable Boolean tenantIsolation` and only puts the key when it is non-null. - `StartControllerCommand#_tenantIsolation` defaults to `null` instead of `true`. Worth noting this field has no `@CommandLine.Option` — it can only be set programmatically — so previously there was no way to run the command *without* pinning the key. - `StartServiceManagerCommand#getDefaultConfig` hardcoded `true` for `CONTROLLER`, pinning the key for every controller the service manager bootstraps without an explicit config. It now leaves it unset. - `isTenantIsolation()` is renamed to `getTenantIsolation()` and returns a nullable `Boolean`, so callers see the tri-state instead of auto-unboxing a possibly-null value. Unchanged: `QuickstartRunner` still calls `setTenantIsolation(...)` explicitly, so quickstarts keep pinning whatever they ask for. `PerfBenchmarkDriver` and `ControllerStarter#startDefault` still pin their values directly. `setTenantIsolation(boolean)` keeps its signature, so existing callers compile as-is. ## Behavior No change for Pinot. `ControllerConf#tenantIsolationEnabled` already falls back to `true` when the key is absent, which is exactly what the removed literals wrote. The effect is that this fallback becomes the single place the default lives, instead of being shadowed by three call sites that wrote the same value into generated configs. The motivation is downstream: a `ControllerStarter` subclass that wants a different tenant isolation default can now express it through `applyCustomConfigs`, on every launch path rather than only when a config file is supplied. ## API note `isTenantIsolation()` → `getTenantIsolation()` is a public signature change on `StartControllerCommand`. It has no callers in the repo. Happy to keep the old name and just widen the return type if reviewers prefer the smaller surface, though `isX()` returning a boxed nullable invites an auto-unboxing NPE at call sites. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
