Address review feedback from the deprecation-removal PRs

[xiangfu0]

Part of #19147. Addresses the review comments left on #19139 and #19142, kept out of those PRs so their approvals weren't churned.

Originally stacked on #19139; now that it has merged, this is rebased onto master and the diff stands alone — 5 files, +22/−20. No API is removed, so it does not trip the Binary Compatibility Check.

1. Redundant FieldConfig casts — addresses IndexCombinationValidationTest and AvgAggregationFunctionTest

Copilot flagged the (List<IndexType>) null casts as noise. Correct: they existed only to disambiguate

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 at 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 had added purely for those casts and which is now unused.

2. GET /tables/livebrokers reports 404 for server-side failures — addresses this comment

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 targets the surviving plural endpoint, not the singular one removed in #19142.

3. Dead _minionConf null-branch in MaterializedViewTaskExecutorFactory

Raised in review of #19139. The factory did:

PinotConfiguration grpcClientConfig = _minionConf != null
    ? _minionConf.subset(MaterializedViewTask.MINION_BROKER_GRPC_CONFIG_PREFIX)
    : new PinotConfiguration();

_minionConf is assigned only by init(zkMetadataManager, minionConf). With the deprecated 1-arg init removed in #19139, that is the only initialization path, so the fallback is unreachable. It is also the wrong thing 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.

Verification

Rebased onto master after #19139/#19141/#19142/#19143 merged, then re-verified: full test-compile plus checkstyle:check and license:check across all five touched modules, and the three affected test classes pass — AvgAggregationFunctionTest (54), IndexCombinationValidationTest (47), OpenStructIndexConfigTest (14).

[Copilot]

Pull request overview

This PR follows up on review feedback from earlier deprecation-removal PRs by (1) cleaning up now-redundant FieldConfig null-casts in tests, (2) fixing an incorrect HTTP status mapping in the controller’s surviving /tables/livebrokers endpoint, and (3) removing a silently-insecure fallback in MaterializedViewTaskExecutorFactory by failing fast if the factory is used before initialization.

Changes:

• Remove redundant (List<IndexType>) null casts at FieldConfig call sites in multiple test classes (and drop the now-unused java.util.List import in AvgAggregationFunctionTest).
• Update GET /tables/livebrokers to surface server-side failures as 500 INTERNAL_SERVER_ERROR (and log the original cause) instead of incorrectly reporting 404 NOT_FOUND.
• Replace an unreachable/unsafe “default empty gRPC config” fallback in MaterializedViewTaskExecutorFactory with a precondition check to ensure init(...) was called before create().

Reviewed changes

Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.

Show a summary per file

File	Description
pinot-spi/src/test/java/org/apache/pinot/spi/config/table/OpenStructIndexConfigTest.java	Drops redundant null casts in FieldConfig construction within open-struct index config tests.
pinot-segment-local/src/test/java/org/apache/pinot/segment/local/utils/IndexCombinationValidationTest.java	Drops redundant null casts in FieldConfig construction across index-combination validation tests.
pinot-plugins/pinot-minion-tasks/pinot-minion-builtin-tasks/src/main/java/org/apache/pinot/plugin/minion/tasks/materializedview/MaterializedViewTaskExecutorFactory.java	Fails fast (instead of silently defaulting) when minion config isn’t initialized before building the gRPC client config.
pinot-core/src/test/java/org/apache/pinot/core/query/aggregation/function/AvgAggregationFunctionTest.java	Removes now-unnecessary List import and redundant null casts in FieldConfig construction.
pinot-controller/src/main/java/org/apache/pinot/controller/api/resources/PinotTableInstances.java	Corrects exception-to-HTTP-status mapping for /tables/livebrokers to return 500 with cause logging.

[codecov-commenter]

Codecov Report

❌ Patch coverage is 0% with 5 lines in your changes missing coverage. Please review.
✅ Project coverage is 65.67%. Comparing base (9d6545b) to head (5f3ca4c).
⚠️ Report is 6 commits behind head on master.

Files with missing lines	Patch %	Lines
...lizedview/MaterializedViewTaskExecutorFactory.java	0.00%	4 Missing ⚠️
.../controller/api/resources/PinotTableInstances.java	0.00%	1 Missing ⚠️

Additional details and impacted files

@@             Coverage Diff              @@
##             master   #19148      +/-   ##
============================================
+ Coverage     65.62%   65.67%   +0.05%     
  Complexity     1423     1423              
============================================
  Files          3439     3439              
  Lines        218236   218063     -173     
  Branches      34679    34678       -1     
============================================
+ Hits         143207   143216       +9     
+ Misses        63479    63295     -184     
- Partials      11550    11552       +2     

Flag	Coverage Δ
custom-integration1	100.00% <ø> (ø)
integration	100.00% <ø> (ø)
integration1	100.00% <ø> (ø)
integration2	0.00% <ø> (ø)
java-25	65.67% <0.00%> (+0.05%)	⬆️
temurin	65.67% <0.00%> (+0.05%)	⬆️
unittests	65.67% <0.00%> (+0.05%)	⬆️
unittests1	57.02% <ø> (+0.03%)	⬆️
unittests2	37.99% <0.00%> (+0.03%)	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.