Leave tenant isolation unset in the generated controller config - #19393
Open
Jackie-Jiang wants to merge 1 commit into
Open
Leave tenant isolation unset in the generated controller config#19393Jackie-Jiang wants to merge 1 commit into
Jackie-Jiang wants to merge 1 commit into
Conversation
PinotConfigUtils#generateControllerConf always wrote cluster.tenant.isolation.enable into the config it generates from individual options, so the generated config could not express "the caller did not choose". Any value a ControllerStarter implementation supplies from applyCustomConfigs was therefore 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: - StartControllerCommand#_tenantIsolation defaults to null instead of true. The field has no @CommandLine.Option, so it can only be set programmatically, and it was previously impossible to launch the command without pinning the key. - StartServiceManagerCommand#getDefaultConfig hardcoded true for CONTROLLER, which pinned the key for every controller the service manager bootstraps without an explicit config. It now leaves it unset. - QuickstartRunner still calls setTenantIsolation explicitly, so quickstarts keep pinning the value they ask for. PerfBenchmarkDriver and ControllerStarter#startDefault are unchanged. Behavior is unchanged for OSS: ControllerConf#tenantIsolationEnabled already falls back to true when the key is absent, which is exactly what the removed literals wrote. It becomes the single place the default lives. isTenantIsolation() is renamed to getTenantIsolation() and returns a nullable Boolean, so that callers see the tri-state rather than auto-unboxing a possibly-null value. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Jackie-Jiang
added
tools
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #19393 +/- ##
============================================
+ Coverage 57.68% 67.60% +9.91%
- Complexity 7 1430 +1423
============================================
Files 2686 3486 +800
Lines 163955 224134 +60179
Branches 26623 35376 +8753
============================================
+ Hits 94579 151519 +56940
+ Misses 61379 60582 -797
- Partials 7997 12033 +4036
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
PinotConfigUtils#generateControllerConfalways wrotecluster.tenant.isolation.enableinto 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 aControllerStarterimplementation supplies fromapplyCustomConfigswas 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#generateControllerConftakes@Nullable Boolean tenantIsolationand only puts the key when it is non-null.StartControllerCommand#_tenantIsolationdefaults tonullinstead oftrue. 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#getDefaultConfighardcodedtrueforCONTROLLER, pinning the key for every controller the service manager bootstraps without an explicit config. It now leaves it unset.isTenantIsolation()is renamed togetTenantIsolation()and returns a nullableBoolean, so callers see the tri-state instead of auto-unboxing a possibly-null value.Unchanged:
QuickstartRunnerstill callssetTenantIsolation(...)explicitly, so quickstarts keep pinning whatever they ask for.PerfBenchmarkDriverandControllerStarter#startDefaultstill pin their values directly.setTenantIsolation(boolean)keeps its signature, so existing callers compile as-is.Behavior
No change for Pinot.
ControllerConf#tenantIsolationEnabledalready falls back totruewhen 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
ControllerStartersubclass that wants a different tenant isolation default can now express it throughapplyCustomConfigs, on every launch path rather than only when a config file is supplied.API note
isTenantIsolation()→getTenantIsolation()is a public signature change onStartControllerCommand. 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, thoughisX()returning a boxed nullable invites an auto-unboxing NPE at call sites.