Deprecate TimeFieldSpec; block new TimeFieldSpec schema creation and add a cluster-health checker by xiangfu0 · Pull Request #18502 · apache/pinot

xiangfu0 · 2026-05-14T21:12:55Z

Summary

This is the next step toward removing the long-deprecated TimeFieldSpec (#2756). It does three things:

Formal deprecation — TimeFieldSpec had only Javadoc @deprecated for years; this adds the @Deprecated annotation so the compiler/IDE actually warn on every remaining call site. Schema#getTimeFieldSpec() is also annotated. Internal SPI bridges that intentionally keep handling TimeFieldSpec for backward-compat JSON deserialization (Jackson @JsonSubTypes, Schema#addField, getSpecForTimeColumn, convertToDateTimeFieldSpec) carry a narrow @SuppressWarnings("deprecation") so the SPI module stays warning-clean.
Block creation of new TimeFieldSpec schemas — POST /schemas now rejects payloads with fieldType=TIME and returns 400 pointing at DateTimeFieldSpec. Lenience is preserved for the realistic backward-compat paths:
- PUT (update) is unchanged — legacy schemas already in ZK can still be edited.
- POST with force=true is unchanged (operator escape hatch).
- POST re-applying an existing legacy schema (same name, ZK already has TimeFieldSpec) is allowed, so restore / reconcile / IaC workflows keep working. Only genuinely new TimeFieldSpec schemas are blocked.
DeprecatedFieldSpecChecker controller periodic task — runs hourly by default, scans tables this controller leads, logs a warning per affected table, and emits two new gauges:
- tableUsesDeprecatedTimeFieldSpec (per-table, 1 / 0)
- tablesWithDeprecatedTimeFieldSpec (global count; hybrid tables dedupe to one)
Overrides nonLeaderCleanup to drop stale per-table gauges on leadership shifts, and preserves prior gauge values on transient schema-fetch failures rather than silently flipping a flagged table to clean.

Configs added

controller.deprecatedFieldSpecChecker.frequencyPeriod (default 1h)
controller.deprecatedFieldSpecChecker.initialDelayInSeconds

Rolling-upgrade note

During a mixed-version controller deployment, newer controllers will reject brand-new TimeFieldSpec schema POSTs while older controllers still accept them. Everything below continues to work on both versions during the rollout: existing schemas in ZK, PUT updates, force=true, and re-applies of pre-existing legacy schemas via POST. No wire format or DataTable version changes.

Test plan

mvn test -pl pinot-spi -Dtest=FieldSpecTest,SchemaTest,SchemaSerializationTest — all 60 SPI tests pass; backward-compat JSON deserialization of "fieldType":"TIME" is unchanged.
mvn test -pl pinot-controller -Dtest=PinotSchemaRestletResourceTest — 6 tests pass, including the new testRejectDeprecatedTimeFieldSpec which covers: POST rejects, POST force=true accepts, PUT accepts, POST re-apply of existing legacy schema accepts.
mvn test -pl pinot-controller -Dtest=DeprecatedFieldSpecCheckerTest — 5 tests pass, covering: positive case, no-affected-tables, hybrid-table dedupe in global gauge, null-schema fetch preserving prior gauge, non-leader cleanup removing stale gauges.
mvn spotless:apply checkstyle:check license:check -pl pinot-common,pinot-controller,pinot-spi — clean.

🤖 Generated with Claude Code

@deprecated

- Mark TimeFieldSpec @deprecated and add @deprecated to Schema#getTimeFieldSpec (Javadoc already pointed at DateTimeFieldSpec; this surfaces compiler/IDE warnings on the remaining call sites). Internal SPI bridges that legitimately keep handling TimeFieldSpec for backward-compat JSON deserialization (FieldSpec @JsonSubTypes, Schema#addField, getSpecForTimeColumn, convertToDateTimeFieldSpec, the _timeFieldSpec field) carry a narrow @SuppressWarnings("deprecation") so the SPI module stays warning-clean. - Block POST /schemas from creating a *new* schema whose payload contains a TimeFieldSpec; the error points at DateTimeFieldSpec and apache#2756. PUT (update) remains lenient so legacy schemas already in ZK can still be edited. POST with force=true is also lenient (operator escape hatch). Re-applying an EXISTING legacy schema via POST (override=true / restore / reconcile workflows) is allowed -- only genuinely new TimeFieldSpec schemas are blocked, matching the stated intent. - Add DeprecatedFieldSpecChecker, a controller periodic task (default 1h) that scans tables led by this controller, logs a warning per affected table, and emits: * tableUsesDeprecatedTimeFieldSpec (per-table, 1/0) * tablesWithDeprecatedTimeFieldSpec (global count; hybrid tables count once via raw-table-name dedupe). Overrides nonLeaderCleanup to drop stale per-table gauges on leadership shift, and preserves prior gauge values on transient schema-fetch failures rather than silently flipping a flagged table to clean. Configs: controller.deprecatedFieldSpecChecker.{frequencyPeriod, initialDelayInSeconds}. Rolling-upgrade note: during a mixed-version controller deployment, newer controllers will reject brand-new TimeFieldSpec schema POSTs while older controllers accept them. Existing schemas, PUT updates, force=true, and re-applies of pre-existing legacy schemas continue to work on both versions. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>

Integration tests post legacy schemas (TimeFieldSpec-based) via the shared ControllerTest test helper as part of cluster setup. With the new "reject new TimeFieldSpec" check on POST /schemas, those setups failed with HTTP 400. The fix routes the test helper through the existing force=true escape hatch -- production callers still hit the strict path. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>

Copilot

Pull request overview

This PR advances removal of the deprecated TimeFieldSpec by making its usage compiler-visible, blocking new schema creation through the controller create path, and adding controller health metrics to surface remaining legacy schemas.

Changes:

Annotates TimeFieldSpec and Schema#getTimeFieldSpec() as deprecated with targeted suppressions for compatibility paths.
Rejects new POST schema payloads using TimeFieldSpec, while preserving forced, update, and existing-legacy reapply flows.
Adds DeprecatedFieldSpecChecker, controller config, metrics, and tests for detecting/reporting deprecated time fields.

Reviewed changes

Copilot reviewed 11 out of 11 changed files in this pull request and generated 1 comment.

Show a summary per file

File	Description
`pinot-spi/src/main/java/org/apache/pinot/spi/data/TimeFieldSpec.java`	Adds compiler-level deprecation annotation.
`pinot-spi/src/main/java/org/apache/pinot/spi/data/Schema.java`	Deprecates legacy getter and suppresses intentional compatibility uses.
`pinot-spi/src/main/java/org/apache/pinot/spi/data/FieldSpec.java`	Suppresses deprecation warning for legacy Jackson subtype support.
`pinot-controller/src/main/java/org/apache/pinot/controller/api/resources/PinotSchemaRestletResource.java`	Adds POST-time rejection for genuinely new `TimeFieldSpec` schemas.
`pinot-controller/src/main/java/org/apache/pinot/controller/validation/DeprecatedFieldSpecChecker.java`	Adds periodic checker and metrics emission for deprecated schema usage.
`pinot-controller/src/main/java/org/apache/pinot/controller/ControllerConf.java`	Adds checker frequency and initial delay configuration.
`pinot-controller/src/main/java/org/apache/pinot/controller/BaseControllerStarter.java`	Registers the new periodic checker in controller startup.
`pinot-common/src/main/java/org/apache/pinot/common/metrics/ControllerGauge.java`	Adds controller gauges for deprecated time-field detection.
`pinot-controller/src/test/java/org/apache/pinot/controller/api/PinotSchemaRestletResourceTest.java`	Covers rejection and allowed legacy schema paths.
`pinot-controller/src/test/java/org/apache/pinot/controller/validation/DeprecatedFieldSpecCheckerTest.java`	Covers checker metrics, hybrid dedupe, null schema handling, and cleanup.
`pinot-controller/src/test/java/org/apache/pinot/controller/helix/ControllerTest.java`	Updates test helper to use force schema creation for legacy fixtures.

xiangfu0 · 2026-05-14T22:15:38Z

+    // controllers does not double-count after a leadership shift.
+    for (String tableNameWithType : tableNamesWithType) {
+      _controllerMetrics.removeTableGauge(tableNameWithType, ControllerGauge.TABLE_USES_DEPRECATED_TIME_FIELD_SPEC);
+    }


Good catch — fixed in 222fa93. nonLeaderCleanup now detects the "all previously-led tables are being cleaned up" case and resets the global gauge to 0; added a regression test testGlobalGaugeResetWhenAllLeadershipLost.

Addresses Copilot review feedback on PR apache#18502. ControllerPeriodicTask.runTask skips processTables (and hence postprocess) when the current controller leads no tables, so the global gauge tablesWithDeprecatedTimeFieldSpec would otherwise stay stuck at its previous non-zero value. nonLeaderCleanup now detects the "every previously-led table is being cleaned up" case and resets the gauge to 0. Add a regression test. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>

…terStatelessTest Adding DeprecatedFieldSpecChecker increased the registered periodic task count from 13 to 14; the stateless starter test asserts the exact count. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>

codecov-commenter · 2026-05-14T23:48:35Z

Codecov Report

❌ Patch coverage is 90.19608% with 5 lines in your changes missing coverage. Please review.
✅ Project coverage is 63.69%. Comparing base (4863cdc) to head (de770a9).
⚠️ Report is 37 commits behind head on master.

Files with missing lines	Patch %	Lines
...va/org/apache/pinot/controller/ControllerConf.java	66.66%	1 Missing and 1 partial ⚠️
...apache/pinot/controller/BaseControllerStarter.java	66.66%	1 Missing ⚠️
...ller/api/resources/PinotSchemaRestletResource.java	88.88%	0 Missing and 1 partial ⚠️
...troller/validation/DeprecatedFieldSpecChecker.java	96.66%	0 Missing and 1 partial ⚠️

Additional details and impacted files

@@             Coverage Diff              @@
##             master   #18502      +/-   ##
============================================
+ Coverage     63.65%   63.69%   +0.04%     
+ Complexity     1735     1684      -51     
============================================
  Files          3254     3267      +13     
  Lines        199446   199948     +502     
  Branches      30977    31056      +79     
============================================
+ Hits         126953   127359     +406     
- Misses        62356    62435      +79     
- Partials      10137    10154      +17

Flag	Coverage Δ
custom-integration1	`100.00% <ø> (ø)`
integration	`100.00% <ø> (ø)`
integration1	`100.00% <ø> (ø)`
integration2	`0.00% <ø> (ø)`
java-21	`63.69% <90.19%> (+0.04%)`	⬆️
temurin	`63.69% <90.19%> (+0.04%)`	⬆️
unittests	`63.69% <90.19%> (+0.04%)`	⬆️
unittests1	`55.81% <100.00%> (+0.10%)`	⬆️
unittests2	`34.98% <90.19%> (+0.01%)`	⬆️

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

☔ View full report in Codecov by Sentry.
📢 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.

…y fixtures TlsIntegrationTest provides its own addSchema override that bypasses the shared ControllerTest helper, so it also needs force=true to load the shared legacy test schema (still TimeFieldSpec-based) under the new controller-side rejection. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>

xiangfu0

Found one high-signal issue; see inline comment.

xiangfu0 · 2026-05-15T12:10:50Z

+    }
+    // If every previously-led table is being cleaned up, ControllerPeriodicTask.runTask did NOT call processTables
+    // (and hence postprocess) this cycle, so the global gauge would otherwise stay stuck at its old value.
+    if (tableNamesWithType.size() == _prevLeaderOfTables.size()) {


ControllerPeriodicTask.runTask() calls processTables() before nonLeaderCleanup(). If leadership fully rotates in one cycle (all old tables lost, new tables gained), this size check still matches and resets TABLES_WITH_DEPRECATED_TIME_FIELD_SPEC to 0 after postprocess() already set the count for the new leaders. That leaves the global gauge wrong until the next run. This reset needs to be gated on the current leader set actually being empty, or moved into a code path that can see the current leaders.

xiangfu0 and others added 2 commits May 14, 2026 14:12

xiangfu0 requested review from J-HowHuang, Jackie-Jiang and Copilot May 14, 2026 21:41

Copilot started reviewing on behalf of xiangfu0 May 14, 2026 21:42 View session

xiangfu0 added deprecation Marks deprecated APIs, configs, or features schema Related to table schema definitions or changes labels May 14, 2026

Copilot AI reviewed May 14, 2026

View reviewed changes

xiangfu0 added the backward-incompat Introduces a backward-incompatible API or behavior change label May 14, 2026

xiangfu0 commented May 15, 2026

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Deprecate TimeFieldSpec; block new TimeFieldSpec schema creation and add a cluster-health checker#18502

Deprecate TimeFieldSpec; block new TimeFieldSpec schema creation and add a cluster-health checker#18502
xiangfu0 wants to merge 5 commits into
apache:masterfrom
xiangfu0:claude/agitated-johnson-99e4b1

xiangfu0 commented May 14, 2026

Uh oh!

Copilot AI left a comment

Uh oh!

xiangfu0 May 14, 2026

Uh oh!

codecov-commenter commented May 14, 2026 •

edited

Loading

Uh oh!

xiangfu0 left a comment

Uh oh!

xiangfu0 May 15, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Conversation

xiangfu0 commented May 14, 2026

Summary

Configs added

Rolling-upgrade note

Test plan

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

xiangfu0 May 14, 2026

Choose a reason for hiding this comment

Uh oh!

codecov-commenter commented May 14, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

xiangfu0 left a comment

Choose a reason for hiding this comment

Uh oh!

xiangfu0 May 15, 2026

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

codecov-commenter commented May 14, 2026 •

edited

Loading