xiangfu0 opened a new pull request, #19148: URL: https://github.com/apache/pinot/pull/19148
Part of #19147. Addresses the review comments left on #19139 and #19142, kept out of those PRs so their approvals aren't churned. > [!IMPORTANT] > **Stacked on #19139.** Its base is `xiangfu0/remove-deprecated-pre-1.0`, so the diff shown against `master` currently includes that PR's commits too. **Review only the last commit** — [646e007](https://github.com/xiangfu0/pinot/commit/646e007747) — which is the 5-file, +22/−20 change described below. The diff collapses to just that commit once #19139 merges. > > The stacking is not cosmetic: two of the three fixes do not compile without #19139 (details inline). No API is removed here, so this one does not trip the Binary Compatibility Check. ## 1. Redundant `FieldConfig` casts — addresses [IndexCombinationValidationTest](https://github.com/apache/pinot/pull/19139#discussion_r3696831973) and [AvgAggregationFunctionTest](https://github.com/apache/pinot/pull/19139#discussion_r3696831979) Copilot flagged the `(List<IndexType>) null` casts as noise. Correct: they existed only to disambiguate ```java FieldConfig(String, EncodingType, IndexType, CompressionCodec, Map) // removed by #19139 FieldConfig(String, EncodingType, List<IndexType>, CompressionCodec, Map) // stays ``` With the singular overload gone, the 5-arg form is unique (the remaining constructors are 7- and 9-arg), so bare `null` resolves unambiguously. Dropped in 12 call sites across three test files — including `OpenStructIndexConfigTest`, which has the same pattern but wasn't touched by #19139. `AvgAggregationFunctionTest` also loses its `java.util.List` import, which #19139 added purely for those casts and which is now unused. **This is why the PR is stacked:** on `master` the singular constructor still exists, so bare `null` is genuinely ambiguous and does not compile. Verified — the change fails to build against a stale `pinot-spi` and passes when `pinot-spi` is built in-reactor from #19139. ## 2. `GET /tables/livebrokers` reports 404 for server-side failures — addresses [this comment](https://github.com/apache/pinot/pull/19142#discussion_r3696839996) The endpoint declares `200` and `500`, but caught `Exception` and mapped **everything** to `404 NOT_FOUND`. Checking `getTableToLiveBrokersMapping`, unknown tables are silently filtered out rather than reported — so nothing it throws is a lookup miss. What it *does* throw is `IllegalStateException` when the broker `ExternalView` is missing, plus whatever Helix/ZK raises. So a cluster-level misconfiguration was being reported to clients as "not found", and the documented `500` was unreachable. Now maps to `INTERNAL_SERVER_ERROR`, matching the declared responses, and passes the cause through so it is logged. This one is independent of #19139 and #19142 — it touches the surviving plural endpoint, not the singular one #19142 deletes. ## 3. Dead `_minionConf` null-branch in `MaterializedViewTaskExecutorFactory` Raised in review of #19139. The factory did: ```java PinotConfiguration grpcClientConfig = _minionConf != null ? _minionConf.subset(MaterializedViewTask.MINION_BROKER_GRPC_CONFIG_PREFIX) : new PinotConfiguration(); ``` `_minionConf` is assigned only by `init(zkMetadataManager, minionConf)`. Once #19139 removes the deprecated 1-arg `init`, that is the only initialization path, so the fallback is unreachable. It is also the wrong default to fall back to — an empty configuration builds a **plaintext** gRPC client with default message limits rather than the operator's configured TLS client, and does so silently. Replaced with a `Preconditions.checkState`, so a factory used before `init` fails loudly instead of quietly downgrading the connection. Also depends on #19139 (on `master` the 1-arg `init` still exists, so the branch is genuinely reachable there). ## Verification Full `test-compile` plus `checkstyle:check` and `license:check` across all five touched modules. The three affected test classes pass: `AvgAggregationFunctionTest` (54), `IndexCombinationValidationTest` (47), `OpenStructIndexConfigTest` (14). -- 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]
