Annotation-driven deprecated table config validation on create + update#18411
Annotation-driven deprecated table config validation on create + update#18411xiangfu0 wants to merge 13 commits into
Conversation
327db64 to
e02ba1c
Compare
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #18411 +/- ##
============================================
- Coverage 64.26% 64.25% -0.01%
Complexity 1126 1126
============================================
Files 3311 3313 +2
Lines 203829 204269 +440
Branches 31722 31826 +104
============================================
+ Hits 130989 131259 +270
- Misses 62326 62448 +122
- Partials 10514 10562 +48
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
noob-se7en
left a comment
There was a problem hiding this comment.
1 major error-handling / 2 medium follow-ups.
2fae407 to
fbc259a
Compare
There was a problem hiding this comment.
Pull request overview
This PR introduces annotation-driven validation to reject explicit usage of deprecated TableConfig JSON keys on both table/config creation and updates, while preserving backward compatibility for legacy values already stored in ZK by diffing against the raw stored ZNRecord JSON.
Changes:
- Add
@DeprecatedConfig(replacement, since)annotations on deprecated SPI config getters and reflectively discover deprecated JSON paths for validation. - Enforce deprecated-config validation on controller create/update/validate endpoints (with version-aware warning vs error severity) and surface warnings via
deprecationWarnings. - Update builders, tests, and example table-config JSON to use modern fields (ingestion configs,
indexTypes,jsonIndexConfigs, etc.), plus add raw-ZK JSON reconstruction utilities for update diffing.
Reviewed changes
Copilot reviewed 50 out of 50 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| pinot-spi/src/main/java/org/apache/pinot/spi/config/DeprecatedConfig.java | Adds the new deprecation annotation used as the single source of truth for deprecated JSON keys. |
| pinot-controller/src/main/java/org/apache/pinot/controller/api/resources/DeprecatedTableConfigValidationUtils.java | Implements reflective rule discovery + create/update-time deprecated-key validation with version-aware severity. |
| pinot-common/src/main/java/org/apache/pinot/common/utils/config/TableConfigSerDeUtils.java | Adds toRawJsonNode(ZNRecord) to reconstruct raw stored JSON for byte-faithful update diffing. |
| pinot-common/src/main/java/org/apache/pinot/common/metadata/ZKMetadataProvider.java | Adds getTableConfigZNRecord() helper to fetch raw table config ZNRecord. |
| pinot-controller/src/main/java/org/apache/pinot/controller/api/resources/PinotTableRestletResource.java | Enforces deprecated-config validation on table create/update/validate and returns warnings in responses. |
| pinot-controller/src/main/java/org/apache/pinot/controller/api/resources/TableConfigsRestletResource.java | Enforces deprecated-config validation for TableConfigs create/update/validate and returns warnings in responses. |
| pinot-controller/src/main/java/org/apache/pinot/controller/api/resources/ConfigSuccessResponse.java | Adds optional deprecationWarnings field to success responses. |
| pinot-spi/src/main/java/org/apache/pinot/spi/utils/builder/TableConfigBuilder.java | Converts deprecated builder setters (segment push + stream configs) into modern ingestion config fields and omits deprecated serialized keys. |
| pinot-spi/src/test/java/org/apache/pinot/spi/utils/builder/TableConfigBuilderTest.java | Tests conversion/omission behavior of deprecated fields in builder output JSON. |
| pinot-spi/src/main/java/org/apache/pinot/spi/config/table/SegmentsValidationAndRetentionConfig.java | Annotates deprecated segment push + other deprecated fields with @DeprecatedConfig. |
| pinot-spi/src/main/java/org/apache/pinot/spi/config/table/IndexingConfig.java | Annotates deprecated indexing fields (jsonIndexColumns, streamConfigs, etc.) with @DeprecatedConfig. |
| pinot-spi/src/main/java/org/apache/pinot/spi/config/table/FieldConfig.java | Marks legacy indexType as deprecated config key for validation while preserving deserialization via @JsonCreator. |
| pinot-spi/src/main/java/org/apache/pinot/spi/config/table/RoutingConfig.java | Annotates deprecated routingTableBuilderName with @DeprecatedConfig. |
| pinot-spi/src/main/java/org/apache/pinot/spi/config/table/UpsertConfig.java | Annotates deprecated upsert booleans with @DeprecatedConfig and NON_DEFAULT inclusion. |
| pinot-spi/src/main/java/org/apache/pinot/spi/config/table/DedupConfig.java | Annotates deprecated dedup booleans with @DeprecatedConfig and NON_DEFAULT inclusion. |
| pinot-spi/src/main/java/org/apache/pinot/spi/config/table/assignment/InstanceReplicaGroupPartitionConfig.java | Annotates deprecated nested minimizeDataMovement with @DeprecatedConfig. |
| pinot-common/src/test/java/org/apache/pinot/common/utils/config/TableConfigSerDeUtilsTest.java | Adds tests ensuring toRawJsonNode() preserves keys stripped by bean round-trips. |
| pinot-controller/src/test/java/org/apache/pinot/controller/api/resources/DeprecatedTableConfigValidationUtilsTest.java | Adds unit tests for rule discovery, version severity, create vs update diff behavior. |
| pinot-controller/src/test/java/org/apache/pinot/controller/api/PinotTableRestletResourceTest.java | Adds REST-level tests for rejecting deprecated keys on create and on update when newly introduced. |
| pinot-controller/src/test/java/org/apache/pinot/controller/api/TableConfigsRestletResourceTest.java | Adds TableConfigs REST test ensuring deprecated keys are rejected on create. |
| pinot-controller/src/test/java/org/apache/pinot/controller/helix/core/rebalance/TableRebalancerClusterStatelessTest.java | Updates test table config creation to use modern ingestion config handling. |
| pinot-core/src/test/java/org/apache/pinot/core/data/manager/realtime/RealtimeSegmentDataManagerTest.java | Migrates tests to use ingestion-config helpers instead of deprecated streamConfigs. |
| pinot-integration-tests/src/test/java/org/apache/pinot/integration/tests/BasePauselessRealtimeIngestionTest.java | Migrates ingestion setup away from deprecated streamConfigs. |
| pinot-integration-tests/src/test/java/org/apache/pinot/integration/tests/logicaltable/LogicalTableWithTwoRealtimeTableIntegrationTest.java | Migrates stream config access to ingestion-config utilities. |
| pinot-integration-tests/src/test/java/org/apache/pinot/integration/tests/realtime/ingestion/KafkaIncreaseDecreasePartitionsIntegrationTest.java | Refactors test to rely on base-class topic/table wiring rather than manual creation. |
| pinot-segment-local/src/test/java/org/apache/pinot/segment/local/utils/TableConfigUtilsTest.java | Adjusts legacy conversion test to inject deprecated keys directly (since builder now produces modern ingestion config). |
| pinot-tools/src/main/java/org/apache/pinot/tools/BootstrapTableTool.java | Adds a null-guard around batch config maps iteration. |
| pinot-tools/src/main/resources/conf/sample_offline_table_config.json | Updates sample config to modern ingestion fields / removes deprecated keys. |
| pinot-tools/src/main/resources/conf/sample_realtime_table_config.json | Updates sample config to modern ingestion fields / removes deprecated keys. |
| pinot-tools/src/main/resources/examples/batch/airlineStats/airlineStats_offline_table_config.json | Removes deprecated segment push keys; adds modern ingestion config fields. |
| pinot-tools/src/main/resources/examples/batch/baseballStats/baseballStats_offline_table_config.json | Removes deprecated segment push keys; adds modern ingestion config fields. |
| pinot-tools/src/main/resources/examples/minions/batch/baseballStats/baseballStats_offline_table_config.json | Removes deprecated segment push keys; adds modern ingestion config fields. |
| pinot-tools/src/main/resources/examples/batch/clickstreamFunnel/clickstreamFunnel_offline_table_config.json | Removes deprecated keys and adds modern ingestion config fields. |
| pinot-tools/src/main/resources/examples/batch/dimBaseballTeams/dimBaseballTeams_offline_table_config.json | Removes deprecated segment push keys; adds modern ingestion config fields. |
| pinot-tools/src/main/resources/examples/batch/fineFoodReviews/fineFoodReviews_offline_table_config.json | Migrates field configs to indexTypes and adds modern ingestion config fields. |
| pinot-tools/src/main/resources/examples/batch/githubEvents/githubEvents_offline_table_config.json | Migrates jsonIndexColumns -> jsonIndexConfigs and adds modern ingestion config fields. |
| pinot-tools/src/main/resources/examples/batch/githubComplexTypeEvents/githubComplexTypeEvents_offline_table_config.json | Removes deprecated segment push keys; adds modern ingestion config fields. |
| pinot-tools/src/main/resources/examples/batch/starbucksStores/starbucksStores_offline_table_config.json | Migrates H3 field config to indexTypes and adds modern ingestion config fields. |
| pinot-tools/src/main/resources/examples/batch/testUnnest/testUnnest_offline_table_config.json | Removes deprecated fields and adds modern ingestion config fields. |
| pinot-tools/src/main/resources/examples/stream/dailySales/dailySales_realtime_table_config.json | Removes deprecated keys and migrates to modern ingestion config fields. |
| pinot-tools/src/main/resources/examples/stream/fineFoodReviews/fineFoodReviews_realtime_table_config.json | Removes deprecated keys and migrates to modern ingestion config fields. |
| pinot-tools/src/main/resources/examples/stream/fineFoodReviews_part_0/fineFoodReviews_part_0_realtime_table_config.json | Removes deprecated keys and migrates to modern ingestion config fields. |
| pinot-tools/src/main/resources/examples/stream/fineFoodReviews_part_1/fineFoodReviews_part_1_realtime_table_config.json | Removes deprecated keys and migrates to modern ingestion config fields. |
| pinot-tools/src/main/resources/examples/stream/meetupRsvpJson/meetupRsvpJson_realtime_table_config.json | Migrates jsonIndexColumns -> jsonIndexConfigs. |
| pinot-tools/src/main/resources/examples/stream/upsertJsonMeetupRsvp/upsertJsonMeetupRsvp_realtime_table_config.json | Migrates jsonIndexColumns -> jsonIndexConfigs. |
| pinot-tools/src/main/resources/examples/stream/upsertMeetupRsvp/upsertMeetupRsvp_realtime_table_config.json | Migrates upsert deprecated fields to modern enums; migrates indexType -> indexTypes. |
| pinot-tools/src/main/resources/examples/stream/upsertPartialMeetupRsvp/upsertPartialMeetupRsvp_realtime_table_config.json | Migrates indexType -> indexTypes. |
| pinot-integration-tests/src/test/resources/chaos-monkey-create-table.json | Removes deprecated segment push keys; adds modern ingestion config fields. |
| pinot-integration-tests/src/test/resources/dimDayOfWeek_config.json | Removes deprecated segment push keys; adds modern ingestion config fields. |
| pinot-clients/pinot-java-client/src/main/java/org/apache/pinot/client/admin/README.md | Documents modern create payload fields and common migrations away from deprecated keys. |
Comments suppressed due to low confidence (1)
pinot-spi/src/main/java/org/apache/pinot/spi/utils/builder/TableConfigBuilder.java:238
setSegmentPushType()/setSegmentPushFrequency()are documented as deprecated in their Javadoc, but they are not annotated with@Deprecated(unlike other deprecated builder APIs in this class, e.g.setLLC).
Annotate these methods with @Deprecated so callers get compiler warnings and IDE tooling consistently flags their use.
/**
* @deprecated Use {@code segmentIngestionType} from {@link IngestionConfig#getBatchIngestionConfig()}
*/
public TableConfigBuilder setSegmentPushType(String segmentPushType) {
if (REFRESH_SEGMENT_PUSH_TYPE.equalsIgnoreCase(segmentPushType)) {
_segmentPushType = REFRESH_SEGMENT_PUSH_TYPE;
} else {
_segmentPushType = "APPEND";
}
return this;
}
/**
* @deprecated Use {@code segmentIngestionFrequency} from {@link IngestionConfig#getBatchIngestionConfig()}
*/
public TableConfigBuilder setSegmentPushFrequency(String segmentPushFrequency) {
_segmentPushFrequency = segmentPushFrequency;
return this;
}
5870292 to
3cb2910
Compare
xiangfu0
left a comment
There was a problem hiding this comment.
Found two high-signal deprecation-validation gaps; see inline comments.
3558fc1 to
1173e98
Compare
xiangfu0
left a comment
There was a problem hiding this comment.
Found one high-signal enforcement gap; see inline comment.
a4a4b3f to
e252bed
Compare
xiangfu0
left a comment
There was a problem hiding this comment.
Two validate endpoints still echo the full raw config body on parse failures. Because ControllerApplicationException logs 4xx messages, that leaks stream credentials and other secrets into controller logs; inline details below.
| tableConfigsJson = JsonUtils.stringToJsonNode(tableConfigsStr); | ||
| } catch (IOException e) { | ||
| throw new ControllerApplicationException(LOGGER, | ||
| String.format("Invalid TableConfigs json string: %s. Reason: %s", tableConfigsStr, e.getMessage()), |
There was a problem hiding this comment.
This still reflects the entire tableConfigsStr into both the 400 response and the controller log line (ControllerApplicationException logs 4xx messages). TableConfigs payloads can carry stream credentials, so a malformed /tableConfigs/validate request now leaks secrets into logs. Please match the addConfig() scrub here and keep only e.getMessage().
There was a problem hiding this comment.
Fixed in 05def21: switched to "Invalid TableConfigs json string. " + e.getMessage() to match addConfig. Also reordered /tableConfigs/validate so the permission check runs BEFORE the ZK-reading validateNoDeprecatedConfigs path, mirroring PinotTableRestletResource.checkTableConfig and preventing an unauthenticated caller from probing table existence.
| JsonUtils.stringToObjectAndUnrecognizedProperties(tableConfigStr, TableConfig.class); | ||
| tableConfigJson = JsonUtils.stringToJsonNode(tableConfigStr); | ||
| } catch (IOException e) { | ||
| String msg = String.format("Invalid table config json string: %s. Reason: %s", tableConfigStr, e.getMessage()); |
There was a problem hiding this comment.
Same leak on /tables/validate: parse failures log the full raw table config via ControllerApplicationException, including any stream config credentials in the request body. The new PR already scrubbed similar error paths elsewhere, so this validate endpoint needs the same treatment.
There was a problem hiding this comment.
Fixed in 05def21: scrubbed the raw tableConfigStr from both the 400 response and the controller log. The catch now uses concatenation ("Invalid table config json string. " + e.getMessage()), matching the addTable/copyTable scrub pattern already in this file.
388e103 to
6934f43
Compare
86e488a to
f4289c6
Compare
Adds @DeprecatedConfig annotation on deprecated TableConfig getters (pinot-spi) plus a reflective controller-side validator that surfaces deprecated keys via a new deprecationWarnings field on create / update / validate / tune / copy response shapes. Soft-launch policy ------------------ Severity classification is gated by DeprecatedTableConfigValidationUtils.SOFT_LAUNCH_WARNING_ONLY = true: every parseable @DeprecatedConfig.since classifies as WARNING regardless of running Pinot version. Only an unparseable since (annotation bug) classifies as ERROR. The constant's Javadoc enumerates the promotion pre-conditions: version-checked CAS on every update path, a concurrency regression test, and a test seam for the older-than-current → ERROR branch. Each update-path call site is tagged with TODO(SOFT_LAUNCH_WARNING_ONLY) so the promotion PR can grep them. Wire-format additions --------------------- - ConfigSuccessResponse: new deprecationWarnings field with @JsonInclude(NON_EMPTY) so older clients see the unchanged shape, plus @JsonPropertyOrder pinning so the serialized order is stable across Jackson versions. - CopyTableResponse: new deprecationWarnings field (same shape and NON_EMPTY policy) so the copy-table flow no longer silently swallows deprecation reports. - /tableConfigs/validate and /tableConfigs/tune share the same TableConfigsValidationResult holder so both surface deprecation warnings. - Raw-body scrubbing on JSON parse-failure error messages (was leaking stream credentials into 4xx response bodies and controller logs). Security / ordering ------------------- - /tableConfigs (addConfig) and /tableConfigs/validate now run the AccessControl check BEFORE the ZK read used for the deprecation diff, preventing unauthenticated callers from probing stored config contents via the diff response shape. - Update endpoints check table existence before computing the diff, so a PUT to a missing table reports 404 rather than a misleading deprecation 400. Behavioural notes for downstream consumers ------------------------------------------ - TableConfigBuilder._segmentPushType keeps its historical "APPEND" default for programmatic-caller backward compatibility. The new validator surfaces the legacy field as a deprecation warning when present, signalling the modern ingestionConfig.batchIngestionConfig.segmentIngestionType replacement. - TableConfigBuilder._createInvertedIndexDuringSegmentGeneration moves from boolean false to Boolean null so the bean's default is only written when the caller explicitly sets it. Bean discovery walk ------------------- - Reflective rule discovery walks every BaseJsonConfig-derived bean reachable from TableConfig; runs exactly once at first reference via the Rules lazy-holder pattern; depth-capped at 32 segments to fail loudly on a future SPI cycle. - Duplicate JSON-property name detection in the walk fails loud at class-load if two getters on the same bean map to the same name. - Array-wildcard diff matches old vs new elements by `name` identity (FieldConfig, TierConfig) when both sides have a textual `name`, falls back to positional only when neither side is name-keyed, and treats asymmetric shapes as newly-introduced (no spurious positional matches). Regression guards ----------------- - testEveryAnnotatedGetterHasJavaZeroDefault: every @DeprecatedConfig getter's owning bean default-instance value must register as default under isJacksonDefault; beans needing the reflective-ctor fallback must be explicitly allowlisted. - testEveryAnnotatedGetterAlsoCarriesJdkDeprecated: dual-annotation invariant; an @DeprecatedConfig without @deprecated fails the build. - testRulesListIsCachedAcrossCalls: pins the discovery walk as one-shot. - testEveryNestedConfigBeanExtendsBaseJsonConfig: walks the TableConfig graph and flags any nested type that isn't a BaseJsonConfig descendant, preventing the validator from silently skipping new SPI bean classes. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Add testEveryRuleHasParseableSince to assert that every discovered DeprecatedConfigRule classifies as WARNING under the soft-launch policy. Under SOFT_LAUNCH_WARNING_ONLY=true the only way to get a non-WARNING severity is an unparseable since string, so a single assertion locks the "every since must be MAJOR.MINOR[.PATCH]" invariant at build time. Without this guard, an empty or malformed since on any annotation would classify as ERROR. Today that surfaces as a soft warning, but the moment the soft-launch flag flips, every PUT/POST that touches that path becomes a 400. Catching it in JUnit means a since-typo can never reach production. Expose DeprecatedConfigRule.since() as a package-private accessor so the test message can quote the offending raw value. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Drop scope creep that crept in alongside the deprecation work: - Revert the speculative segmentAssignmentConfigMap blocks added to 11 example/sample JSONs. The string "BalanceNumSegmentAssignmentStrategy" did not match any canonical AssignmentStrategy constant; it worked only by falling through SegmentAssignmentStrategyFactory's permissive default. The block was not part of any deprecated-key migration. - Revert KafkaIncreaseDecreasePartitionsIntegrationTest. Under the soft-launch policy the original test passes without modification — the refactor was unrelated to the validator change. Harden the wire contract on ConfigSuccessResponse: - Annotate the canonical constructor with @JsonCreator + @JsonProperty so Jackson deserializes without depending on the -parameters flag. - Add round-trip tests: full-shape deserialize plus the new-client / old-controller compatibility direction (no deprecationWarnings field). Make PinotTableRestletResourceTest order-insensitive on deprecationWarnings. The discovery walk relies on Class.getMethods(), whose order is not specified, so asserting the exact JSON string was fragile. Helpers now parse the response and compare the warning SET. Add a SUNSET TARGET section to SOFT_LAUNCH_WARNING_ONLY's Javadoc so the flip is anchored to a release (≥1.7.0) and the validator is not left as a permanent logging shim. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
- TableConfigSerDeUtils.toRawJsonNode: guard null simpleField values. ZNRecord.getSimpleFields() permits null values; the previous code NPE'd inside looksLikeJsonContainer on the update-path diff read. Add a targeted unit test that pushes a null value directly into the simpleFields map and asserts the diff sees NullNode. - CopyTableResponse: mark status and msg @nullable in the @JsonCreator signature. The class is @JsonInclude(NON_NULL) so both fields have always been omittable; the constructor's non-null typing was misleading SDK / OpenAPI generators. Also drop the new setDeprecationWarnings setter — deprecation warnings are immutable after the response is built. - DeprecatedTableConfigValidationUtilsTest: lock SOFT_LAUNCH_WARNING_ONLY at true so the flag cannot flip without an intentional edit to this test. The test message reminds the next author of the four promotion pre-conditions (CAS, concurrency test, version seam, ERROR integration test). Also replace inline FQCNs with imports per CLAUDE.md. - Realtime sample JSONs: drop speculative batchIngestionConfig blocks. The segmentPushType -> batchIngestionConfig migration is meaningful for OFFLINE tables only; REALTIME tables don't push segments in the OFFLINE sense. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
- DeprecatedConfig.since: spell out in the SPI Javadoc that unparseable values classify as ERROR and therefore become 400 BAD_REQUEST under the soft-launch policy. Names the in-tree controller test that locks the invariant so external annotators know where the contract lives. - DeprecatedTableConfigValidationUtils.isJsonAccessor: document why @JsonIgnore getters are intentionally traversed. The validator detects deprecated JSON input keys; @JsonIgnore prevents the bean from re-emitting the key on output but does not prevent the @JsonCreator / setters from accepting it on input. FieldConfig.indexType is the canonical example. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The deprecation-validator pre-reads the stored ZNRecord to compute the diff, then later issues an update write. Previously these were two separate ZK operations with no version check binding them, so a concurrent writer that landed between the read and the write could slip a deprecated key past the diff. Documented as the #1 promotion pre-condition in SOFT_LAUNCH_WARNING_ONLY's Javadoc; now landed. Plumbing: - ZKMetadataProvider.getTableConfigZNRecord(propertyStore, name, stat): new overload that populates the supplied Stat so callers can capture the znode version. - PinotHelixResourceManager.updateTableConfig(tableConfig, expectedVersion, force): new overload that forwards expectedVersion to setExistingTableConfig for the CAS write. - PinotTableRestletResource.updateTableConfig: thread the stat.getVersion through to the write call. If the stored config didn't exist (the hasTable check above would have already 404'd, but for defense in depth we still pass -1 to skip the version check in that case). - TableConfigsRestletResource.validateNoDeprecatedConfigs: return a DeprecationValidationResult holder carrying per-sub-type versions alongside the warnings, so the offline/realtime update calls can each issue their own CAS write. The Javadoc for SOFT_LAUNCH_WARNING_ONLY now lists pre-condition #1 as done; the three remaining (concurrency regression test, classifySeverity version-injection seam, ERROR-path integration test) are still required before the flip. Also: switch the test reflective-ctor String placeholder from "_test" to "" so a future failure surfaces as a real default mismatch rather than a confusing "got `_test`, expected default" message. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
CAS conflict is now a typed exception (TableConfigVersionConflictException in pinot-common) so the REST resources can return HTTP 409 CONFLICT with retry guidance rather than burning a generic 500 with an opaque message. The legacy updateTableConfig(tableConfig) / updateTableConfig(tc, force) overloads use expectedVersion=-1 so they cannot trigger the CAS branch; they wrap any unexpected throw as IllegalStateException to preserve the pre-existing two-checked-exception signature for back-compat with existing callers (PinotDdlRestletResource, ControllerRequestClient, RealtimeOffsetAutoResetKafkaHandler). For TableConfigs PUT, the 409 message explicitly notes the partial- success semantics — TableConfigs writes offline and realtime sequentially, so the offline sub-config may have committed before the realtime CAS lost. The client is told to re-read both and retry. Adds testDiscoveryWalkDoesNotThrowAtClassLoad to lock the build-time invariant that the validator's static lazy holder loads cleanly. A duplicate @JsonProperty annotation across two getters on the same bean, an unparseable since="…", or a bean cycle exceeding MAX_WALK_DEPTH would otherwise surface only at deploy time as an ExceptionInInitializerError that permanently breaks every controller table endpoint. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Lands two of the four promotion pre-conditions in this PR (the other two — version-injection seam for classifySeverity, and the REST integration test — are now done or no longer applicable): Concurrency regression test (testUpdateTableConfigCasConflictThrowsConflictException) in PinotHelixResourceManagerStatelessTest: exercises the CAS read→write window end-to-end. T1 reads znode version v; T2 commits an update at v+1; T1 attempts a write at v and must receive TableConfigVersionConflictException (not a generic 5xx). Asserts T2's commit survives. ERROR-path REST integration test (testErrorSeverityRuleRejectsCreateAs400) in PinotTableRestletResourceTest: under SOFT_LAUNCH_WARNING_ONLY no in-tree annotation can produce an ERROR severity. Inject a synthetic ERROR rule via the new setRulesOverrideForTesting test seam, POST a table that triggers the rule, and assert the REST surface returns 400. Restores the override in finally. The test seam (setRulesOverrideForTesting / clearRulesOverrideForTesting / makeRuleForTesting) lets tests substitute the rule list end-to-end without flipping SOFT_LAUNCH_WARNING_ONLY. DeprecatedConfigRule is now public so cross-package tests can construct synthetic rules; the constructor remains package-private so production code cannot fabricate rules outside the reflective discovery walk. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
setExistingTableConfig now performs the version check before the backward-compatibility validation. Previously the back-compat read ran against a potentially newer znode than the caller's expectedVersion, which could surface a misleading 400 TableConfigBackwardIncompatibleException when the right answer is 409 TableConfigVersionConflictException. The short-circuit reads a Stat-bearing snapshot, compares, and throws immediately if the versions differ. The CAS write below remains the source of truth for the atomic write. Add @VisibleForTesting (com.google.common.annotations) to the three rule-override test seams so static analysis surfaces any inadvertent production callers. The methods stay public (cross-package tests in controller.api need them) but are now self-documenting as test-only. Document the non-atomic offline-then-realtime semantics on the /tableConfigs/{tableName} PUT endpoint Javadoc: clients receiving 409 MUST re-read both sub-configs and retry the full transaction; the offline write may have already landed before realtime CAS failed. Add a cross-reference comment in DeprecatedTableConfigValidationUtilsTest mapping each of the four SOFT_LAUNCH_WARNING_ONLY promotion pre-conditions to its in-tree coverage. This makes the promotion-PR checklist discoverable from a single test file. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…arsal Three changes addressing reviewer-flagged MAJORs: 1. Move DeprecatedTableConfigValidationUtils (and its test) from pinot-controller/api/resources to pinot-common/utils/config. The class does not depend on any controller-specific type — only pinot-spi annotations and Jackson — so it can live in pinot-common alongside TableConfigSerDeUtils. This unblocks future reuse from pinot-broker / pinot-server / ops tooling without pulling in pinot-controller. Pure relocation; package-private members are preserved (the test moves with the class to keep access to them). 2. Add DeprecatedTableConfigValidationUtils#warmUp(): public method that eagerly triggers the Rules.ALL lazy holder so the first REST request after controller startup doesn't pay the reflective discovery cost. Called from BaseControllerStarter#setUpPinotController and logs the discovered rule count. 3. Add testErrorSeverityRuleAllowsUnchangedLegacyValueOnUpdate: post-flip rehearsal that uses the existing @VisibleForTesting rule-override seam to simulate SOFT_LAUNCH_WARNING_ONLY=false semantics. Verifies the two non-obvious post-flip cases: - PUT re-submitting an UNCHANGED legacy value on a table that already holds it must succeed (diff sees no change, ERROR severity does not fire). This is the operability guarantee the promotion PR depends on — without it, every PUT to every existing table holding a legacy field would 400 the moment the flag flips. - PUT INTRODUCING a value change at the same deprecated path must be rejected as 400. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
- setExistingTableConfig: in CAS mode (expectedVersion >= 0), REUSE
the byte-snapshot fetched by the pre-flight version check for the
backward-compatibility validation that follows. Previously the two
reads were independent — back-compat validation could run against
a fresher znode than the one whose version the caller checked,
producing a misleading 400 BAD_REQUEST in the race window. Now a
single TableConfig instance drives both decisions in the CAS path.
Legacy non-CAS callers (-1 expectedVersion) keep the original
getTableConfig() path.
- TableConfigsRestletResource: extend the documented atomicity caveat
to enumerate the schema-then-tables non-rollback semantics, not
just the offline-then-realtime case. Clients receiving 409 must
re-read schema AND both sub-configs and merge against observed
state.
- DeprecationValidationResult: drop the unused warnings() accessor;
callers all read the field directly so the getter was dead code.
- TableConfigSerDeUtils.toRawJsonNode: log WARN when a stored
simpleField looks like a JSON container (starts with '{' or '[')
but fails to parse. The fallback to a text-node representation is
preserved so the table stays usable, but operators now have a
signal when a corrupt ZK record could be producing spurious
"newly introduced" deprecation warnings on update.
- ConfigSuccessResponse: add @JsonIgnoreProperties(ignoreUnknown=true)
so strict-parsing older clients (FAIL_ON_UNKNOWN_PROPERTIES=true)
tolerate future controller-emitted fields. Locked by new
testStrictParserToleratesUnknownFutureField in
ConfigSuccessResponseTest.
- DeprecatedTableConfigValidationUtils.findMatchingOldElement: when
the stored array carries DUPLICATE `name` keys (Pinot does not
enforce uniqueness at the JSON layer), log WARN with the duplicate
name and count so the corruption is visible. Behaviour-preserving:
first-match wins, matching the historical contract.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
- DeprecatedConfig annotation Javadoc: rewrite to describe current
soft-launch semantics (every parseable since classifies as warning,
only unparseable since fires the throw branch) and clearly mark the
post-flip semantics as future-tense.
- PinotTableRestletResource.updateTableConfig: handle the
hasTable-then-delete race. When the stored ZNRecord disappears
between the existence check and the diff read, return 404 with an
explicit message rather than letting an NPE inside validateOnUpdate
bubble out as an opaque 5xx.
- PinotHelixResourceManager.setExistingTableConfig: correct the
comment that claimed the pre-flight version check "closes" the
race window. The atomicity boundary is the propertyStore.set CAS,
not the pre-flight read; the early throw is an optimisation that
gives a precise 409 in the obvious-stale-read case.
- TableConfigBuilder: restore the named constant
APPEND_SEGMENT_PUSH_TYPE and reference it from both the field
default and the setSegmentPushType("APPEND") branch, eliminating
duplicated string literals (C7.14).
- PinotTableRestletResourceTest: belt-and-braces cleanup in the
@AfterMethod always clears the deprecation-validator rule override
so a failing ERROR-path test cannot poison subsequent tests.
- TableConfigSerDeUtilsTest: add three round-trip tests covering
malformed JSON container fallback, primitive-looking text values
(123, true, null) that must stay as text nodes, and the existing
null-value branch. Locks the load-bearing diff-driving contract
for toRawJsonNode.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…re shape Reviewer flagged that broadening the checked-exception throws clause on PinotHelixResourceManager.setExistingTableConfig(TableConfig, int [, boolean]) is a source-incompat for external callers (forks, plugins) compiled against the prior signature. Convert TableConfigVersionConflictException from `extends Exception` to `extends RuntimeException` so the binary contract of the pre-existing two-arg / three-arg public methods is preserved. The REST resources still catch the specific subclass to translate to HTTP 409; the "callers SHOULD return 409" recommendation is unchanged, just no longer forced by the compiler. Cleanup that flows from the change: - Strip the now-unnecessary `throws TableConfigVersionConflictException` from setExistingTableConfig(...,int) / (...,int,boolean) and updateTableConfig(...,int,boolean) public signatures. - Drop the catch-and-rewrap blocks in the legacy setExistingTableConfig(TableConfig) and updateTableConfig(TableConfig, boolean) overloads. They no longer need to convert a checked exception that the underlying methods cannot throw with expectedVersion=-1. Concurrent-delete race (reviewer MAJOR #4): in CAS mode, when the pre-flight read returns null because a concurrent thread deleted the table, throw TableConfigVersionConflictException with a clear "was deleted concurrently" message instead of silently proceeding (which could re-create the table at expectedVersion — Lazarus pattern — or emit a misleading "modified by a concurrent writer" message). Wire-shape rolling-upgrade docs (reviewer MAJORs #2 and #3): expand the pinot-java-client README to enumerate the response-shape changes - FieldConfig.indexType elided via @JsonIgnore (read indexTypes instead), and several boolean getters gain @JsonInclude(NON_DEFAULT) so the field disappears when at the type default. New @JsonIgnoreProperties(ignoreUnknown=true) on ConfigSuccessResponse covers strict-parsing old clients. Recommends FAIL_ON_UNKNOWN_PROPERTIES=false for strict parsers OR upgrade in lockstep with the controller. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
b02182b to
a7825e6
Compare
Summary
Reject explicit use of deprecated table-config keys on both create and update, driven by a single source of truth on the SPI getters instead of a hand-maintained rule list.
@DeprecatedConfig(replacement, since)annotation inpinot-spi. Placed on the deprecated getter;sinceis the Pinot release the field was first marked@Deprecated.DeprecatedTableConfigValidationUtilsdiscovers rules at class-load via a Jackson-aware reflection walk overTableConfig(recursing into nestedBaseJsonConfigtypes,Collection<X>,Map<?,V>, honoring@JsonPropertyrenames). The hand-maintained list is gone.since.major.minorequals the runningPinotVersion.major.minoris reported as a warning (one-release grace period). Older rules are errors that block the request. Unknown current version → safe default of error./tables/{name}, PUT/tableConfigs/{name}, validate POSTs against existing tables) diff the incoming JSON against the byte-faithful stored ZNRecord JSON (new helperTableConfigSerDeUtils.toRawJsonNode+ZKMetadataProvider.getTableConfigZNRecord) — not againstexistingConfig.toJsonNode(), which would silently strip@JsonIgnore-d /@JsonInclude(NON_DEFAULT)deprecated keys and turn every legacy PUT into a false positive. Re-submitting an unchanged legacy value is a no-op; only newly introduced or value-changed deprecated paths fire.deprecationWarnings: List<String>field onConfigSuccessResponseand the validate endpoint JSON. Errors continue to throw400.Deprecated table-config keys covered
Sorted from earliest deprecation to latest. On the current
1.6.0-SNAPSHOTrelease line, everything older than1.6is an error;1.6.0deprecations are warnings (one-release grace period).routing.routingTableBuilderNamerouting.segmentPrunerTypesandrouting.instanceSelectorTypetableIndexConfig.streamConfigsingestionConfig.streamIngestionConfig.streamConfigMapssegmentsConfig.segmentPushFrequencyingestionConfig.batchIngestionConfig.segmentIngestionFrequencysegmentsConfig.segmentPushTypeingestionConfig.batchIngestionConfig.segmentIngestionTypefieldConfigList[*].indexTypefieldConfigList[].indexTypestableIndexConfig.jsonIndexColumnstableIndexConfig.jsonIndexConfigssegmentsConfig.replicasPerPartitionsegmentsConfig.replicationinstanceAssignmentConfigMap[*].replicaGroupPartitionConfig.minimizeDataMovementsegmentsConfig.replicaGroupStrategyConfigsegmentAssignmentConfigMapsegmentsConfig.minimizeDataMovementinstanceAssignmentConfigMapupsertConfig.enableSnapshotupsertConfig.snapshotupsertConfig.enablePreloadupsertConfig.preloadupsertConfig.allowPartialUpsertConsumptionDuringCommitingestionConfig.streamIngestionConfig.parallelSegmentConsumptionPolicydedupConfig.enablePreloaddedupConfig.preloaddedupConfig.allowDedupConsumptionDuringCommitingestionConfig.streamIngestionConfig.parallelSegmentConsumptionPolicytableIndexConfig.createInvertedIndexDuringSegmentGenerationsincewas determined per field by walkinggit log/git tag --containsagainst the upstream apache/pinot history to find the first release tag that ships the@Deprecatedannotation (or the original@deprecatedJavadoc when that came first).Behavior
400; warnings → server WARN log +deprecationWarningsfield.TableConfigBuilder.build()now converts the deprecated_segmentPushType/_segmentPushFrequencysetters into moderningestionConfig.batchIngestionConfig.segmentIngestionType/Frequency, so existing tests and tools that use the builder produce create payloads that pass validation.Testing
./mvnw -pl pinot-spi,pinot-common,pinot-controller -am -Dtest='DeprecatedTableConfigValidationUtilsTest+TableConfigSerDeUtilsTest+TableConfigBuilderTest+PinotTableRestletResourceTest#testRejectsDeprecatedConfigOnCreateAndOnUpdateWhenNewlyIntroduced+PinotTableRestletResourceTest#testUpdateAllowsUnchangedLegacyDeprecatedConfig+TableConfigsRestletResourceTest' test./mvnw -pl pinot-spi,pinot-common,pinot-controller spotless:apply checkstyle:check license:checkCoverage includes: annotation discovery walk against
TableConfig, diff filtering on update, version-based severity classification, raw-JSON preservation across@JsonIgnore/@JsonInclude(NON_DEFAULT)getters, and round-trip rejection/acceptance through the controller REST endpoints.