Add quickstart process-specific config override hooks.#17959
Add quickstart process-specific config override hooks.#17959gortiz wants to merge 1 commit intoapache:masterfrom
Conversation
Allow quickstart wiring to provide broker and server scoped configuration maps when launching local components. Made-with: Cursor
There was a problem hiding this comment.
Pull request overview
Adds per-process (broker/server) config override hooks to Pinot quickstart startup wiring, enabling role-specific runtime properties while preserving existing behavior when no overrides are provided.
Changes:
- Extend
QuickstartRunnerwith optionalIntFunction<Map<String, Object>>providers to supply per-broker and per-server override maps. - Apply per-instance override maps when assembling broker/server launch config overrides.
- Add override hook methods in
Quickstartand wire them into the runner constructor.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| pinot-tools/src/main/java/org/apache/pinot/tools/admin/command/QuickstartRunner.java | Adds per-broker/server override providers and merges them into process config overrides during startup. |
| pinot-tools/src/main/java/org/apache/pinot/tools/Quickstart.java | Introduces protected override hooks for per-broker/server config and passes them to QuickstartRunner. |
| _individualBrokerConfigOverridesFunction = individualBrokerConfigOverridesFunction; | ||
| _individualServerConfigOverridesFunction = individualServerConfigOverridesFunction; |
There was a problem hiding this comment.
The new constructor stores individualBrokerConfigOverridesFunction / individualServerConfigOverridesFunction without null-checking. If a caller passes null, quickstart will later throw an NPE in startBrokers()/startServers(). Consider validating these parameters (e.g., require non-null) or defaulting null to an empty-map provider to keep the API robust.
| _individualBrokerConfigOverridesFunction = individualBrokerConfigOverridesFunction; | |
| _individualServerConfigOverridesFunction = individualServerConfigOverridesFunction; | |
| _individualBrokerConfigOverridesFunction = individualBrokerConfigOverridesFunction != null | |
| ? individualBrokerConfigOverridesFunction | |
| : i -> Collections.emptyMap(); | |
| _individualServerConfigOverridesFunction = individualServerConfigOverridesFunction != null | |
| ? individualServerConfigOverridesFunction | |
| : i -> Collections.emptyMap(); |
| Map<String, Object> individualControllerConfigOverrides = _individualServerConfigOverridesFunction.apply(i); | ||
| if (!individualControllerConfigOverrides.isEmpty()) { | ||
| Map<String, Object> config = new HashMap<>(_configOverrides); | ||
| config.putAll(individualControllerConfigOverrides); |
There was a problem hiding this comment.
In startServers(), the local variable is named individualControllerConfigOverrides but it actually holds per-server overrides from _individualServerConfigOverridesFunction. Renaming it to reflect 'server' would avoid confusion and copy/paste mistakes during future edits.
| Map<String, Object> individualControllerConfigOverrides = _individualServerConfigOverridesFunction.apply(i); | |
| if (!individualControllerConfigOverrides.isEmpty()) { | |
| Map<String, Object> config = new HashMap<>(_configOverrides); | |
| config.putAll(individualControllerConfigOverrides); | |
| Map<String, Object> individualServerConfigOverrides = _individualServerConfigOverridesFunction.apply(i); | |
| if (!individualServerConfigOverrides.isEmpty()) { | |
| Map<String, Object> config = new HashMap<>(_configOverrides); | |
| config.putAll(individualServerConfigOverrides); |
| IntFunction<Map<String, Object>> individualServerConfigOverridesFunction | ||
| ) throws Exception { |
There was a problem hiding this comment.
Constructor formatting is inconsistent with the other constructors in this class: the closing ) is on its own line before throws Exception. Consider aligning it with the established style used above (closing parenthesis on the same line as the last parameter) to keep diffs and formatting consistent.
| IntFunction<Map<String, Object>> individualServerConfigOverridesFunction | |
| ) throws Exception { | |
| IntFunction<Map<String, Object>> individualServerConfigOverridesFunction) throws Exception { |
| protected Map<String, Object> getIndividualBrokerConfigOverridesFunction(int brokerId) { | ||
| return Map.of(); | ||
| } | ||
| | ||
| protected Map<String, Object> getIndividualServerConfigOverridesFunction(int serverId) { | ||
| return Map.of(); |
There was a problem hiding this comment.
The new hook methods getIndividualBrokerConfigOverridesFunction(int) / getIndividualServerConfigOverridesFunction(int) return a config Map, but their names suggest they return a function. Consider renaming them to something like getIndividualBrokerConfigOverrides(int) / getIndividualServerConfigOverrides(int) to make the override contract clearer for subclasses.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@ ## master #17959 +/- ## ============================================ - Coverage 63.30% 63.23% -0.07% Complexity 1525 1525 ============================================ Files 3193 3193 Lines 193330 193330 Branches 29732 29732 ============================================ - Hits 122381 122253 -128 - Misses 61356 61491 +135 + Partials 9593 9586 -7
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
yashmayya added enhancement 
Summary
Why
Quickstart currently uses mostly shared/default process config at startup. To support extension scenarios and targeted local testing, we need a simple way to inject per-process overrides (for example, different broker/server settings) without rewriting quickstart flows.
This, for example, opens the ability to explicitly set the ports used by each service.
Changes Included (with effect)
pinot-tools/src/main/java/org/apache/pinot/tools/Quickstart.javapinot-tools/src/main/java/org/apache/pinot/tools/admin/command/QuickstartRunner.javaIntFunction<Map<String, Object>>) for broker/server process overrides.Behavior / Compatibility
Test Plan
./mvnw -pl pinot-tools -am -DskipTests compile