You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@pinot.apache.org by "Copilot (via GitHub)" <gi...@apache.org> on 2026/05/04 23:04:08 UTC
Re: [PR] Annotation-driven deprecated table config validation on create + update [pinot]
Copilot commented on code in PR #18411:
URL: https://github.com/apache/pinot/pull/18411#discussion_r3185041827
##########
pinot-controller/src/main/java/org/apache/pinot/controller/api/resources/PinotTableRestletResource.java:
##########
@@ -346,6 +350,11 @@ public CopyTableResponse copyTable(
ObjectNode realtimeTableConfigNode = (ObjectNode) tableConfigNode.get(TableType.REALTIME.name());
tweakRealtimeTableConfig(realtimeTableConfigNode, copyTablePayload);
+ try {
+ DeprecatedTableConfigValidationUtils.validateNoDeprecatedConfigs(realtimeTableConfigNode);
+ } catch (IllegalStateException e) {
Review Comment:
`validateNoDeprecatedConfigs()` throws `IllegalArgumentException` (via `validateOnCreate()`), but this code only catches `IllegalStateException`. As a result, deprecated-config validation failures during table copy will fall through to the generic `catch (Exception)` and be returned as `500 INTERNAL_SERVER_ERROR` instead of `400 BAD_REQUEST`.
Catch `IllegalArgumentException` here (or change the validator to throw the documented exception type consistently) so copyTable returns 400 for deprecated config keys.
##########
pinot-tools/src/main/resources/examples/stream/fineFoodReviews_part_0/fineFoodReviews_part_0_realtime_table_config.json:
##########
@@ -2,14 +2,18 @@
"tableName": "fineFoodReviews_part_0",
"tableType": "REALTIME",
"segmentsConfig": {
- "segmentPushType": "APPEND",
"timeColumnName": "ts",
"retentionTimeUnit": "DAYS",
"retentionTimeValue": "5",
"replication": "1"
},
"tenants": {
},
+ "segmentAssignmentConfigMap": {
+ "REALTIME": {
+ "segmentAssignmentStrategy": "BalanceNumSegmentAssignmentStrategy"
+ }
+ },
Review Comment:
`segmentAssignmentConfigMap` keys should be assignment types like `COMPLETED`/`CONSUMING` (for REALTIME) rather than `REALTIME`. With `REALTIME`, the controller won’t find the entry when selecting a segment assignment strategy.
Update the key to `COMPLETED` (or whichever assignment type this config is intended for).
##########
pinot-tools/src/main/resources/examples/stream/fineFoodReviews_part_1/fineFoodReviews_part_1_realtime_table_config.json:
##########
@@ -2,14 +2,18 @@
"tableName": "fineFoodReviews_part_1",
"tableType": "REALTIME",
"segmentsConfig": {
- "segmentPushType": "APPEND",
"timeColumnName": "ts",
"retentionTimeUnit": "DAYS",
"retentionTimeValue": "5",
"replication": "1"
},
"tenants": {
},
+ "segmentAssignmentConfigMap": {
+ "REALTIME": {
+ "segmentAssignmentStrategy": "BalanceNumSegmentAssignmentStrategy"
+ }
+ },
Review Comment:
`segmentAssignmentConfigMap` is not keyed by table type. For REALTIME tables, use an assignment type such as `COMPLETED` (segment assignment config is applied to completed segments) so the entry is actually read by the controller.
With the current `REALTIME` key, this config will be ignored.
##########
pinot-controller/src/main/java/org/apache/pinot/controller/api/resources/DeprecatedTableConfigValidationUtils.java:
##########
@@ -0,0 +1,445 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.pinot.controller.api.resources;
+
+import com.fasterxml.jackson.annotation.JsonProperty;
+import com.fasterxml.jackson.databind.JsonNode;
+import java.lang.reflect.Method;
+import java.lang.reflect.Modifier;
+import java.lang.reflect.ParameterizedType;
+import java.lang.reflect.Type;
+import java.util.ArrayList;
+import java.util.Collection;
+import java.util.Collections;
+import java.util.HashSet;
+import java.util.List;
+import java.util.Map;
+import java.util.Objects;
+import java.util.Set;
+import javax.annotation.Nullable;
+import org.apache.pinot.common.version.PinotVersion;
+import org.apache.pinot.spi.config.BaseJsonConfig;
+import org.apache.pinot.spi.config.DeprecatedConfig;
+import org.apache.pinot.spi.config.table.TableConfig;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+
+/**
+ * Validates raw table-config JSON for deprecated properties on create and update paths.
+ *
+ * <p>Rules are derived at class-load time from {@link DeprecatedConfig} annotations on getters reachable from
+ * {@link TableConfig}. A violation's severity ({@link Severity#WARNING} vs {@link Severity#ERROR}) is decided by
+ * comparing the annotation's {@code since} version against the running Pinot version (see
+ * {@link #classifySeverity}): properties deprecated in the current major.minor release are warnings; properties
+ * deprecated in any earlier release are errors. When the running version cannot be determined, all violations are
+ * reported as errors so the safe default is to reject.
+ *
+ * <p>The validator operates on raw JSON rather than the deserialized {@link TableConfig} so it can detect
+ * explicitly provided deprecated keys even when they carry default or false-y values that Jackson would otherwise
+ * elide.</p>
+ */
+public final class DeprecatedTableConfigValidationUtils {
+ private DeprecatedTableConfigValidationUtils() {
+ }
+
+ private static final Logger LOGGER = LoggerFactory.getLogger(DeprecatedTableConfigValidationUtils.class);
+ private static final String WILDCARD = "*";
+ // CURRENT_MAJOR_MINOR must be initialized before RULES because discoverRules() -> classifySeverity() reads it.
+ private static final String CURRENT_MAJOR_MINOR = currentMajorMinor();
+ private static final List<DeprecatedConfigRule> RULES = discoverRules();
+
+ public enum Severity {
+ WARNING, ERROR
+ }
+
+ /**
+ * Result of validating a table-config JSON, exposing errors and warnings separately so callers can decide whether
+ * to reject the request or surface non-fatal warnings to the user.
+ */
+ public static final class Result {
+ private final List<String> _errors;
+ private final List<String> _warnings;
+
+ private Result(List<String> errors, List<String> warnings) {
+ _errors = List.copyOf(errors);
+ _warnings = List.copyOf(warnings);
+ }
+
+ public List<String> getErrors() {
+ return _errors;
+ }
+
+ public List<String> getWarnings() {
+ return _warnings;
+ }
+
+ public boolean hasErrors() {
+ return !_errors.isEmpty();
+ }
+
+ public boolean hasWarnings() {
+ return !_warnings.isEmpty();
+ }
+ }
+
+ /**
+ * Validates a table-config JSON document. When {@code oldTableConfigJson} is non-null, only paths whose value
+ * newly appears in {@code newTableConfigJson} or whose value differs from the corresponding path in
+ * {@code oldTableConfigJson} are reported, so re-submitting an unchanged legacy field on an update is a no-op.
+ * When {@code oldTableConfigJson} is null (creation path), every present deprecated path is reported.
+ *
+ * @param newTableConfigJson the incoming table config to validate; must not be {@code null}
+ * @param oldTableConfigJson the currently-stored table config to diff against, or {@code null} for create paths
+ * @param rootPathPrefix optional prefix for emitted paths (e.g. {@code "realtime"} when validating a sub-section)
+ */
+ public static Result validate(JsonNode newTableConfigJson, @Nullable JsonNode oldTableConfigJson,
+ @Nullable String rootPathPrefix) {
+ Objects.requireNonNull(newTableConfigJson, "newTableConfigJson");
+ List<String> errors = new ArrayList<>();
+ List<String> warnings = new ArrayList<>();
+ String prefix = rootPathPrefix == null ? "" : rootPathPrefix;
+ for (DeprecatedConfigRule rule : RULES) {
+ rule.collect(newTableConfigJson, oldTableConfigJson, prefix, errors, warnings);
+ }
+ return new Result(errors, warnings);
+ }
+
+ /**
+ * Validates a freshly-submitted table config (no prior stored value). On any error the method throws
+ * {@link IllegalStateException}; warnings are returned so the caller can surface them in the response.
+ */
+ public static List<String> validateOnCreate(JsonNode newTableConfigJson, @Nullable String rootPathPrefix) {
+ Result result = validate(newTableConfigJson, null, rootPathPrefix);
+ if (result.hasErrors()) {
+ throw new IllegalArgumentException("Deprecated table config properties are not allowed on table creation: "
+ + String.join("; ", result.getErrors()));
+ }
Review Comment:
The Javadoc for `validateOnCreate()` says it throws `IllegalStateException`, but the implementation throws `IllegalArgumentException` on violations. This mismatch leaks into callers (e.g., some catch `IllegalStateException` and will miss the actual exception type).
Align the documented and actual exception types (either update the Javadoc or throw the documented type) and keep it consistent across `validateOnCreate()` / `validateOnUpdate()`.
##########
pinot-tools/src/main/resources/examples/stream/dailySales/dailySales_realtime_table_config.json:
##########
@@ -2,14 +2,18 @@
"tableName": "dailySales",
"tableType": "REALTIME",
"segmentsConfig": {
- "segmentPushType": "APPEND",
"timeColumnName": "daysSinceEpoch",
"retentionTimeUnit": "DAYS",
"retentionTimeValue": "50000",
"replication": "1"
},
"tenants": {
},
+ "segmentAssignmentConfigMap": {
+ "REALTIME": {
+ "segmentAssignmentStrategy": "BalanceNumSegmentAssignmentStrategy"
+ }
+ },
Review Comment:
`segmentAssignmentConfigMap` should be keyed by assignment type (e.g. `COMPLETED` for REALTIME completed segments), not `REALTIME`. `SegmentAssignmentStrategyFactory` looks up the entry by `assignmentType.toUpperCase()`.
Change the key from `REALTIME` to `COMPLETED` (or the intended assignment type) so the example actually configures segment assignment.
##########
pinot-tools/src/main/resources/examples/stream/fineFoodReviews/fineFoodReviews_realtime_table_config.json:
##########
@@ -2,14 +2,18 @@
"tableName": "fineFoodReviews",
"tableType": "REALTIME",
"segmentsConfig": {
- "segmentPushType": "APPEND",
"timeColumnName": "ts",
"retentionTimeUnit": "DAYS",
"retentionTimeValue": "5",
"replication": "1"
},
"tenants": {
},
+ "segmentAssignmentConfigMap": {
+ "REALTIME": {
+ "segmentAssignmentStrategy": "BalanceNumSegmentAssignmentStrategy"
+ }
+ },
Review Comment:
`segmentAssignmentConfigMap` is keyed by the segment assignment type (e.g. `OFFLINE` / `COMPLETED` / `CONSUMING`), as used by `SegmentAssignmentStrategyFactory` via `segmentAssignmentConfigMap.get(assignmentType.toUpperCase())`. Using `REALTIME` here won’t be picked up and makes the example misleading.
For a REALTIME table, this should typically be `COMPLETED` (segment assignment config is applied to completed segments).
##########
pinot-spi/src/main/java/org/apache/pinot/spi/config/DeprecatedConfig.java:
##########
@@ -0,0 +1,54 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.pinot.spi.config;
+
+import java.lang.annotation.ElementType;
+import java.lang.annotation.Retention;
+import java.lang.annotation.RetentionPolicy;
+import java.lang.annotation.Target;
+
+
+/**
+ * Marks a config getter as a deprecated JSON property in {@code TableConfig} (or any nested config bean).
+ *
+ * <p>Discovered at runtime by {@code DeprecatedTableConfigValidationUtils} to gate creation and update of table
+ * configs. Severity is determined by comparing {@link #since()} against the running Pinot version: properties
+ * deprecated in the current major.minor release are reported as <em>warnings</em>, while properties deprecated in
+ * any earlier release are reported as <em>errors</em>.
+ *
+ * <p>Place this annotation on the Jackson-visible getter (the one that drives JSON property naming) so the
+ * discovery walk picks up the correct serialized name.</p>
+ */
+@Target({ElementType.METHOD, ElementType.FIELD})
Review Comment:
`@DeprecatedConfig` is declared with `@Target({METHOD, FIELD})`, but the discovery logic in `DeprecatedTableConfigValidationUtils` only inspects method annotations (`method.getAnnotation(DeprecatedConfig.class)`) and never checks fields. This can mislead contributors into annotating fields and then wondering why the rule is not enforced.
Either restrict the target to `ElementType.METHOD` (matching the current implementation and PR description), or extend the rule discovery walk to also inspect fields (including `@JsonProperty` renames on fields if you keep FIELD support).
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org