xiangfu0 opened a new issue, #19147:
URL: https://github.com/apache/pinot/issues/19147
Tracking issue for an audit-driven cleanup of Pinot's deprecated public
surface.
## Background
Master carries 371 `@Deprecated` annotations, some dating to 2016. I audited
all of them, plus unmarked legacy config keys, stale feature flags, and
obsolete code paths, to establish for each one: what replaced it, whether any
non-deprecated production code still calls it, and whether removal is safe with
respect to ZooKeeper-serialized config, wire formats, on-disk segment format,
and third-party SPI implementations.
## Approach
Rather than one large sweep, the removals are split **one PR per deprecating
release line**. Each PR removes only APIs whose deprecation shipped in that
release, so a reviewer can reason about a single deprecation window, and a
future reader auditing "when was X deprecated?" gets a truthful answer from the
PR that removed it.
Every removed member was verified to have zero non-deprecated production
callers. Each branch is a single commit, green on full-reactor `test-compile`,
`checkstyle:check`, and `license:check` on JDK 25.
## Open PRs
- [ ] #19139 — pre-1.0 releases (0.1.0–0.12.0 era) · 36 files, −500
- [ ] #19140 — 1.0.0 · 5 files
- [ ] #19141 — 1.1.0 · 13 files, −349
- [ ] #19142 — 1.2.0 · 3 files
- [ ] #19143 — 1.3.0 · 18 files, −216
Roughly 1,140 lines of dead surface. All are labelled `backward-incompat`;
each PR body carries the removed→replacement mapping and the operator-facing
notes for its own scope.
### Note on the Binary Compatibility Check
`Pinot Binary Compatibility Check` fails by design on #19139, #19141 and
#19143. It runs japicmp with `--error-on-binary-incompatibility` over
`pinot-spi` / `pinot-segment-spi`, and removing public members from those
modules is precisely the intent of this work. The exact violations are
enumerated in each PR body so they can be eyeballed against the intended
removals. It is not a required check, so it does not block merge.
## Deliberately excluded
These came up during the audit and were **not** removed, each for a specific
reason. Recording them here so they are not re-proposed without the context:
| Item | Why it stays |
|---|---|
| `POST /segments/{tableName}/delete` | The client migration to `DELETE
/segments/{tableName}` is unsafe as written — see the bugs section below. Needs
the client fixed first. |
| `SegmentLocks` static factories |
`ConcurrentMapPartitionUpsertMetadataManagerForConsistentDeletes` still calls
the deprecated static; migrating changes lock scoping from a global cache to
per-`TableDataManager` locks, which is a concurrency change, not a deprecation
cleanup. |
| `TableAndSchemaConfig` | Still the request-body DTO of
`PinotUpsertRestletResource`, and its JSON shape differs from `TableConfigs`;
swapping would break the public REST contract. |
| `ControllerGauge.OFFLINE_TABLE_ESTIMATED_SIZE` | Still emitted by
`StorageQuotaChecker`. |
| `POST /instances/{instanceName}/state` |
`InstanceAdminClient.setInstanceState` is a live caller, and the POST handler
supports a `drop` operation the replacement PUT handler does not. |
| `HeaderAdder` ThirdEye URI-rewrite hack | Never `@Deprecated`, runs
`@PreMatching` on every controller request, and has no test coverage — an
unverifiable live behavior change that should not ride along with mechanical
deletions. Deserves its own PR. |
## Follow-up work
**Staged endpoint deletions.** Three endpoints had their callers migrated in
the PRs above but were intentionally left in place, so that an N-1 component
still works against an N component during a rolling upgrade. They can be
deleted once these PRs have shipped in a release:
- [ ] Server `GET /table/{tableName}/size` (singular) — caller repointed in
#19139
- [ ] Controller `GET /segments/{tableName}/select` — caller repointed in
#19140
**Remaining release lines.** The audit identified further removable APIs
deprecated in 1.4.0, 1.5.0, and within the unreleased 1.6.0 cycle. Following
the same one-PR-per-release pattern:
- [ ] 1.4.0-era removals (includes the `InstancePlanMakerImplV2` legacy
constant block, whose own comment set a "remove after 1.4" deadline that has
now passed twice, and `PinotHelixResourceManager.getSchemaForTableConfig`,
which rode along in #19140 with its true vintage noted)
- [ ] 1.5.0-era removals
- [ ] 1.6.0-cycle removals — these have never shipped in a release, so
removing them before 1.6.0 cuts means the deprecated surface never becomes
public API
- [ ] Post-1.6.0 scheduled sweep: items that declare their own exit
criteria, e.g. the `segment.local.function` forwarding shims and the
`V1Constants` keys that need a stop-writing-then-stop-reading two-step
**New deprecations to add.** The audit also found legacy surface that is not
yet marked at all. Worth its own discussion rather than bundling here — notably
the `HttpClient` auth-less overloads (documented as deprecated but never
annotated), the silent old→new config-key fallbacks that log nothing when the
legacy key is used, and the pre-`fieldConfigList` index-declaration fields on
`IndexingConfig`.
## Bugs found in passing
These exist on master today and are unrelated to the PRs above. Filing
separately, listed here for traceability:
- `SegmentAdminClient` DELETE path: an empty/null segment list omits the
`segments` query parameter, which the controller interprets as "delete all
segments of the table"
- `SegmentAdminClient` DELETE path: multi-segment deletes comma-join into a
single parameter value that never matches the controller's repeated
`@QueryParam("segments")` binding, so they silently no-op
- `TenantAdminClient.changeTenantState` issues `PUT
/tenants/{tenantName}/metadata`, but that path declares only `@GET` and `@POST`
— it can only ever return 405
--
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]
