Remove APIs deprecated in 1.2.0 - #19142
Merged
Merged
Conversation
- Remove deprecated controller endpoint GET /tables/{tableName}/livebrokers
(PinotTableInstances); the plural GET /tables/livebrokers?tables=...
endpoint remains.
- Remove deprecated RequestUtils.getFunctionExpression(String) (no-operand
variant); all callers already use the Function or name+operands overloads.
- Remove deprecated RequestUtils.getOptionsFromJson(JsonNode, String);
no callers.
- Remove deprecated BrokerResponse.getProcessingExceptions() default method;
use getExceptions() instead.
xiangfu0
added
the
backward-incompat
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #19142 +/- ##
============================================
- Coverage 65.62% 65.61% -0.02%
Complexity 1423 1423
============================================
Files 3439 3439
Lines 218195 218184 -11
Branches 34669 34669
============================================
- Hits 143198 143154 -44
- Misses 63443 63462 +19
- Partials 11554 11568 +14
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:
|
Contributor
There was a problem hiding this comment.
🟡 Not ready to approve
GET /tables/livebrokers currently converts all exceptions into 404 NOT_FOUND despite documenting 500, and this should be corrected before the singular endpoint removal makes the plural endpoint the primary path.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
This review doesn't count toward merge requirements. Sign up for the private preview to control whether Copilot approvals count.
Pull request overview
Removes long-deprecated public APIs (controller REST + pinot-common helpers) as part of the 1.2.0-cycle deprecation sweep, shrinking the supported surface area while keeping the non-deprecated replacements intact.
Changes:
- Removed deprecated controller endpoint
GET /tables/{tableName}/livebrokers(singular), keeping the cluster-scopedGET /tables/livebrokers. - Removed deprecated
RequestUtilsstaticsgetFunctionExpression(String)andgetOptionsFromJson(JsonNode, String). - Removed deprecated
BrokerResponse.getProcessingExceptions()default method (JSON response shape remains unchanged since it was@JsonIgnore).
File summaries
| File | Description |
|---|---|
pinot-controller/src/main/java/org/apache/pinot/controller/api/resources/PinotTableInstances.java |
Deletes the deprecated singular live-brokers endpoint and its unused import. |
pinot-common/src/main/java/org/apache/pinot/common/utils/request/RequestUtils.java |
Removes deprecated helper overloads with no remaining in-repo callers. |
pinot-common/src/main/java/org/apache/pinot/common/response/BrokerResponse.java |
Removes deprecated getProcessingExceptions() default method from the broker response interface. |
Review details
- Files reviewed: 3/3 changed files
- Comments generated: 1
- Review effort level: Lite
We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.
Comment on lines
160
to
162
| @GET | ||
| @Path("/tables/livebrokers") | ||
| @Authorize(targetType = TargetType.CLUSTER, action = Actions.Cluster.GET_BROKER) |
Contributor
Author
There was a problem hiding this comment.
Good catch — this is a real bug, fixed in #19148.
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 and passes the cause through so it gets logged.
Note this is the surviving plural endpoint, independent of the singular one removed here, so the fix applies cleanly either way.
yashmayya
approved these changes
Aug 2, 2026
11 tasks
xiangfu0
added a commit
to xiangfu0/pinot
that referenced
this pull request
Aug 2, 2026
Follow-ups to the review comments on apache#19139 and apache#19142. No API is removed here; this is cleanup of loose ends left by those PRs plus one bug fix. - Drop the now-redundant (List<IndexType>) null casts at FieldConfig call sites. Those casts existed only to disambiguate the singular-IndexType 5-arg constructor from the List one; apache#19139 removes the former, so bare null now resolves unambiguously. Also drops the java.util.List import in AvgAggregationFunctionTest, which is unused once the casts are gone. - GET /tables/livebrokers mapped every exception to 404 NOT_FOUND while declaring only 200 and 500. getTableToLiveBrokersMapping filters unknown tables rather than reporting them, so nothing it throws is a lookup miss: a missing broker ExternalView surfaces as IllegalStateException and was being reported to clients as "not found", and the documented 500 was unreachable. Map to INTERNAL_SERVER_ERROR and keep the cause. - MaterializedViewTaskExecutorFactory fell back to an empty PinotConfiguration when _minionConf was null. Once apache#19139 removes the deprecated 1-arg init, _minionConf is always set, so the branch is dead; if it were ever reached it would silently build a plaintext gRPC client with default limits instead of the configured TLS one. Replaced with a Preconditions check.
xiangfu0
added a commit
that referenced
this pull request
Aug 3, 2026
Follow-ups to the review comments on #19139 and #19142. No API is removed. - Drop the now-redundant (List<IndexType>) null casts at FieldConfig call sites. They existed only to disambiguate the singular-IndexType 5-arg constructor from the List one; with the former removed in #19139, bare null resolves unambiguously. Also drops the java.util.List import in AvgAggregationFunctionTest, unused once the casts are gone, and collapses the call sites that no longer need to wrap. - GET /tables/livebrokers mapped every exception to 404 NOT_FOUND while declaring only 200 and 500. getTableToLiveBrokersMapping filters unknown tables rather than reporting them, so nothing it throws is a lookup miss: a missing broker ExternalView surfaced as IllegalStateException and was reported to clients as 'not found', and the documented 500 was unreachable. Map to INTERNAL_SERVER_ERROR with a stable message, keeping the cause for the logs. - MaterializedViewTaskExecutorFactory fell back to an empty PinotConfiguration when _minionConf was null, which would silently build a plaintext gRPC client with default limits instead of the configured TLS one. With the deprecated 1-arg init removed, that branch is dead. Replaced with Preconditions checks covering both _minionConf and _zkMetadataManager, at the top of create() so they run on every call rather than only the first.
xiangfu0
added a commit
to pinot-contrib/pinot-docs
that referenced
this pull request
Aug 3, 2026
Documents the upgrade impact of apache/pinot#19142, including the replacement live-brokers endpoint, response-shape migration, and removed RequestUtils APIs. Validation: git diff --check. Co-authored-by: Xiang Fu <xiangfu@xiangs-macbook-pro-1.wyvern-sun.ts.net>
Contributor
Author
|
Documentation follow-up: pinot-contrib/pinot-docs#958 |
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
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.
Part of #19147.
Removes APIs whose deprecations shipped in the 1.2.0 cycle (2024). Part of a series of per-release deprecation sweeps.
All removed members were verified to have zero non-deprecated production callers. Full-reactor
test-compile,checkstyle:check, andlicense:checkpass. No test changes were needed — nothing referenced any removed API.Removed
GET /tables/{tableName}/livebrokers(singular). The pluralGET /tables/livebrokersremains. Note the two return different shapes, so external callers of the singular form need more than a path change — see below.RequestUtils.getFunctionExpression(String)andgetOptionsFromJson(JsonNode, String). Both had zero callers; the non-deprecatedgetFunctionExpression(Function)overload is untouched and still used byClpRewriter.BrokerResponse.getProcessingExceptions()— a@JsonIgnoredefault method with no callers, so the broker JSON response shape is unchanged. (The same-named methods onRequestContextare an unrelated interface and are untouched.)Deliberately excluded
Two items from the original sweep were dropped after investigation:
POST /segments/{tableName}/delete— removing this requires migratingSegmentAdminClienttoDELETE /segments/{tableName}first, and that migration turned out to be unsafe in two independent ways: the DELETE endpoint treats a missingsegmentsparameter as "delete every segment in the table" (so an empty list silently escalates from a 400 into whole-table deletion), and its@QueryParam("segments") List<String>binding only populates from repeatedsegments=a&segments=bparameters, so a comma-joined value arrives as one bogus segment name and silently no-ops. Both bugs appear to exist in the current client's DELETE path independently of this PR and deserve their own fix.SegmentLocksstatic factories —ConcurrentMapPartitionUpsertMetadataManagerForConsistentDeletesstill calls the deprecated staticgetSegmentLock. Migrating it to the instance method would change lock scoping from a global static cache to per-TableDataManagerlocks, which is a concurrency-semantics change that does not belong in a deprecation sweep.TableAndSchemaConfigwas also left in place: it is still the request-body DTO ofPinotUpsertRestletResource, and its JSON shape ({tableConfig, schema}) differs fromTableConfigs({tableName, schema, offline, realtime}), so swapping it would break the public REST contract.backward-incompat
Please apply the
backward-incompatlabel.The removed
GET /tables/{tableName}/livebrokersendpoint 404s for external clients after upgrade, and the replacement is not a drop-in:GET /tables/livebrokersreturnsMap<String, List<InstanceInfo>>rather than the singular endpoint'sList<String>. The removedRequestUtilspublic statics are source-breaking for out-of-tree code compiled againstpinot-common.