yashmayya opened a new pull request, #19644:
URL: https://github.com/apache/pinot/pull/19644
## The bug
`ServiceStartableUtils.applyClusterConfig` copies cluster configs into the
instance config with `PinotConfiguration.setProperty`. This method keeps a
comma-separated value as one string, so `getProperty(key, List)` returns one
element, for example `"table1,table2"`.
The same value in the instance config file works, because a config that is
built from a map or a file splits on commas. List configs that are read from a
`subset()` copy also work, because `subset()` splits again.
Pinot reads these list configs from the full instance config. A cluster
config value with 2 or more items does not work for them:
| Config | Effect |
|---|---|
| `pinot.broker.allowedTablesForEmittingMetrics` | The listed tables emit no
table-level metrics. |
| `pinot.server.allowedTablesForEmittingMetrics` | The listed tables emit no
table-level metrics. |
| `pinot.server.transforms` | `Class.forName("a,b")` fails, so the server
does not start. |
| `controller.access.protocols` | `getControllerVipPort()` does not find the
protocol that has `vip` set. It uses `controller.port` instead. |
`pinot.broker.mse.planner.disabled.rules` has the same problem. #19642 fixes
it.
A related bug: the `usePlannerRules` and `skipPlannerRules` query options
split on commas, but do not trim. For example,
`usePlannerRules='SortJoinTranspose, AggregateJoinTransposeExtended'` gives `"
AggregateJoinTransposeExtended"`, and no rule has that name.
## The fix
- New method `PinotConfiguration.getCommaSeparatedList(name,
defaultValues)`. It splits each value on commas, trims it, and drops empty
values. If no value is left, it returns the default values, like
`getProperty(String, List)`.
- The 4 reads above use this method.
- `QueryOptionsUtils` trims each rule name in `usePlannerRules` and
`skipPlannerRules`, and drops empty names.
The code splits each list config where it reads it, not in `setProperty` or
`applyClusterConfig`. A split there has a side effect: `getRawProperty` and the
typed getters return a List for every cluster config value that contains a
comma.
## Testing
- New tests set each config with `setProperty`, the same call that
`applyClusterConfig` makes, and with a map: `ServerConfTest`,
`BaseBrokerStarterTest`, `ControllerConfTest` and `PinotConfigurationTest`.
- `QueryOptionsUtilsTest` and `QueryPlannerRuleOptionsTest` cover rule names
with spaces. The planner test checks that the second rule in
`'SortJoinTranspose, SortProjectTranspose'` applies.
- Each cluster config test and the planner test fail on the old code.
- `spotless`, `checkstyle` and `license` are clean.
## Upgrade note
If one of these configs is in the cluster config with 2 or more items, the
upgrade changes this behavior:
- The listed tables start to emit table-level metrics.
- The controller VIP port comes from the protocol that has `vip` set, not
from `controller.port`. The URL from `generateVipUrl()` changes.
If a query lists rules with spaces after the commas, it now gets all of the
listed rules. Its plan can change.
The code now ignores empty items in these lists, for example from a trailing
comma.
## Notes
- `ListenerConfigUtil.buildListenerConfigs` also splits `*.access.protocols`
without trimming. A cluster config value like `"http, https"` stops controller
startup. This bug is older than this PR, and this PR does not change it.
- After this PR and #19642 merge, `MultiStageBrokerRequestHandler` can use
`getCommaSeparatedList`.
--
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]
