Copilot commented on code in PR #17679:
URL: https://github.com/apache/pinot/pull/17679#discussion_r2792964850
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/utils/TableConfigUtils.java:
##########
@@ -165,7 +165,7 @@ public static void validate(TableConfig tableConfig, Schema
schema) {
* TODO: Add more validations for each section (e.g. validate conditions are
met for aggregateMetrics)
*/
public static void validate(TableConfig tableConfig, Schema schema,
@Nullable String typesToSkip) {
- Preconditions.checkArgument(schema != null, "Schema should not be null");
+ Preconditions.checkArgument(schema != null, "Schema should not be null for
table: %s", tableConfig.getTableName());
Review Comment:
`tableConfig.getTableName()` is dereferenced inside the `schema != null`
precondition message, which can throw an NPE if `tableConfig` is null (or if
`getTableName()` returns null) before the intended argument validation failure.
Consider adding a separate precondition for `tableConfig` earlier, or avoid
dereferencing `tableConfig` in the message (e.g., use a safe fallback like
`String.valueOf(tableConfig)` or guard the table name lookup).
##########
pinot-controller/src/main/java/org/apache/pinot/controller/helix/ControllerRequestClient.java:
##########
@@ -209,6 +209,30 @@ public void deleteLogicalTable(String logicalTableName)
}
}
+ public String getLogicalTable(String logicalTableName)
+ throws IOException {
+ try {
+ SimpleHttpResponse response = HttpClient.wrapAndThrowHttpException(
+ _httpClient.sendGetRequest(
+ new
URI(_controllerRequestURLBuilder.forLogicalTableGet(logicalTableName)),
_headers));
+ return response.getResponse();
+ } catch (HttpErrorStatusException | URISyntaxException e) {
+ throw new IOException(e);
+ }
+ }
+
+ public List<String> getLogicalTableNames()
+ throws IOException {
+ try {
+ SimpleHttpResponse response = HttpClient.wrapAndThrowHttpException(
+ _httpClient.sendGetRequest(
+ new URI(_controllerRequestURLBuilder.forLogicalTableNamesGet()),
_headers));
+ return JsonUtils.stringToObject(response.getResponse(), List.class);
+ } catch (HttpErrorStatusException | URISyntaxException e) {
+ throw new IOException(e);
+ }
+ }
Review Comment:
`JsonUtils.stringToObject(..., List.class)` returns a raw `List` and relies
on an unchecked conversion to `List<String>`. To make this type-safe (and avoid
runtime surprises if the payload changes), deserialize with an explicit generic
type (e.g., Jackson `TypeReference<List<String>>` equivalent used by
`JsonUtils`, if available).
##########
pinot-controller/src/main/java/org/apache/pinot/controller/api/resources/PinotLogicalTableResource.java:
##########
@@ -148,7 +148,7 @@ public SuccessResponse addLogicalTable(
return new ConfigSuccessResponse(successResponse.getStatus(),
logicalTableConfigAndUnrecognizedProps.getRight());
}
- private void translatePhysicalTableNamesWithDB(LogicalTableConfig
logicalTableConfig, HttpHeaders headers) {
+ public static void translatePhysicalTableNamesWithDB(LogicalTableConfig
logicalTableConfig, HttpHeaders headers) {
Review Comment:
Changing this helper from `private` to `public static` expands the public
API surface of a REST resource class for an internal utility operation. If this
is only meant for reuse within the package/module, prefer package-private
(`static` with no access modifier) or move it into a dedicated utility class
(keeping the REST resource focused on request handling).
```suggestion
static void translatePhysicalTableNamesWithDB(LogicalTableConfig
logicalTableConfig, HttpHeaders headers) {
```
##########
pinot-controller/src/main/java/org/apache/pinot/controller/api/resources/TableConfigValidationUtils.java:
##########
@@ -0,0 +1,113 @@
+/**
+ * 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 javax.annotation.Nullable;
+import org.apache.pinot.controller.ControllerConf;
+import org.apache.pinot.controller.helix.core.PinotHelixResourceManager;
+import org.apache.pinot.controller.helix.core.minion.PinotTaskManager;
+import org.apache.pinot.controller.helix.core.rebalance.TableRebalancer;
+import org.apache.pinot.controller.util.TaskConfigUtils;
+import org.apache.pinot.segment.local.utils.TableConfigUtils;
+import org.apache.pinot.spi.config.table.TableConfig;
+import org.apache.pinot.spi.config.table.TableType;
+import org.apache.pinot.spi.data.Schema;
+import org.apache.pinot.spi.utils.builder.TableNameBuilder;
+
+
+/**
+ * Utility class that encapsulates table config validation logic shared across
+ * {@link PinotTableRestletResource} and StarTree's managed logical table
resource.
+ *
+ * <p>This lives in {@code pinot-controller} (not {@code
pinot-segment-local}'s {@code TableConfigUtils})
+ * because validation requires controller-level dependencies like {@link
PinotHelixResourceManager},
+ * {@link ControllerConf}, {@link PinotTaskManager}, and {@link
TableRebalancer}.</p>
+ */
+public final class TableConfigValidationUtils {
+
+ private TableConfigValidationUtils() {
+ }
+
+ /**
+ * Validates a table config against the given schema and controller
configuration.
+ *
+ * <p>Performs the following validations in order:</p>
+ * <ol>
+ * <li>Core validation ({@link TableConfigUtils#validate})</li>
+ * <li>Table name validation ({@link
TableConfigUtils#validateTableName})</li>
+ * <li>Min replicas enforcement</li>
+ * <li>Storage quota constraints</li>
+ * <li>Hybrid table config check (if both OFFLINE and REALTIME versions
exist)</li>
+ * <li>Task config validation (skipped if {@code taskManager} is null)</li>
Review Comment:
The Javadoc states task config validation is skipped when `taskManager` is
null, but the implementation calls `TaskConfigUtils.validateTaskConfigs(...)`
unconditionally. Either update the Javadoc to reflect the actual behavior, or
add an explicit null-guard before invoking task validation so the contract
matches the documentation.
##########
pinot-controller/src/main/java/org/apache/pinot/controller/api/resources/TableConfigValidationUtils.java:
##########
@@ -0,0 +1,113 @@
+/**
+ * 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 javax.annotation.Nullable;
+import org.apache.pinot.controller.ControllerConf;
+import org.apache.pinot.controller.helix.core.PinotHelixResourceManager;
+import org.apache.pinot.controller.helix.core.minion.PinotTaskManager;
+import org.apache.pinot.controller.helix.core.rebalance.TableRebalancer;
+import org.apache.pinot.controller.util.TaskConfigUtils;
+import org.apache.pinot.segment.local.utils.TableConfigUtils;
+import org.apache.pinot.spi.config.table.TableConfig;
+import org.apache.pinot.spi.config.table.TableType;
+import org.apache.pinot.spi.data.Schema;
+import org.apache.pinot.spi.utils.builder.TableNameBuilder;
+
+
+/**
+ * Utility class that encapsulates table config validation logic shared across
+ * {@link PinotTableRestletResource} and StarTree's managed logical table
resource.
+ *
+ * <p>This lives in {@code pinot-controller} (not {@code
pinot-segment-local}'s {@code TableConfigUtils})
+ * because validation requires controller-level dependencies like {@link
PinotHelixResourceManager},
+ * {@link ControllerConf}, {@link PinotTaskManager}, and {@link
TableRebalancer}.</p>
+ */
+public final class TableConfigValidationUtils {
+
+ private TableConfigValidationUtils() {
+ }
+
+ /**
+ * Validates a table config against the given schema and controller
configuration.
+ *
+ * <p>Performs the following validations in order:</p>
+ * <ol>
+ * <li>Core validation ({@link TableConfigUtils#validate})</li>
+ * <li>Table name validation ({@link
TableConfigUtils#validateTableName})</li>
+ * <li>Min replicas enforcement</li>
+ * <li>Storage quota constraints</li>
+ * <li>Hybrid table config check (if both OFFLINE and REALTIME versions
exist)</li>
+ * <li>Task config validation (skipped if {@code taskManager} is null)</li>
+ * <li>Instance assignment validation</li>
+ * </ol>
+ *
+ * <p><b>NOT included</b> (caller responsibility):</p>
+ * <ul>
+ * <li>Schema retrieval — caller resolves it</li>
+ * <li>Tuner config application — CREATE-only, mutates config</li>
+ * <li>Active tasks check — caller-specific</li>
+ * </ul>
+ *
+ * @param tableConfig the table config to validate
+ * @param schema the schema for the table (must not be null)
+ * @param typesToSkip comma-separated list of validation types to skip
(ALL|TASK|UPSERT), or null
+ * @param resourceManager the Helix resource manager
+ * @param controllerConf the controller configuration
+ * @param taskManager the task manager, or null to skip task validation
+ */
+ public static void validateTableConfig(TableConfig tableConfig, Schema
schema,
+ @Nullable String typesToSkip, PinotHelixResourceManager resourceManager,
+ ControllerConf controllerConf, @Nullable PinotTaskManager taskManager) {
+ TableConfigUtils.validate(tableConfig, schema, typesToSkip);
+ TableConfigUtils.validateTableName(tableConfig);
+ TableConfigUtils.ensureMinReplicas(tableConfig,
controllerConf.getDefaultTableMinReplicas());
+ TableConfigUtils.ensureStorageQuotaConstraints(tableConfig,
controllerConf.getDimTableMaxSize());
+ checkHybridTableConfig(resourceManager, tableConfig);
+ TaskConfigUtils.validateTaskConfigs(tableConfig, schema, taskManager,
typesToSkip);
+ validateInstanceAssignment(resourceManager, tableConfig);
+ }
+
+ private static void checkHybridTableConfig(PinotHelixResourceManager
resourceManager, TableConfig tableConfig) {
+ String rawTableName =
TableNameBuilder.extractRawTableName(tableConfig.getTableName());
+ if (tableConfig.getTableType() == TableType.REALTIME) {
+ if (resourceManager.hasOfflineTable(rawTableName)) {
+ TableConfigUtils.verifyHybridTableConfigs(rawTableName,
+ resourceManager.getOfflineTableConfig(rawTableName), tableConfig);
+ }
+ } else {
+ if (resourceManager.hasRealtimeTable(rawTableName)) {
+ TableConfigUtils.verifyHybridTableConfigs(rawTableName, tableConfig,
+ resourceManager.getRealtimeTableConfig(rawTableName));
+ }
+ }
+ }
+
+ private static void validateInstanceAssignment(PinotHelixResourceManager
resourceManager,
+ TableConfig tableConfig) {
+ TableRebalancer tableRebalancer = new
TableRebalancer(resourceManager.getHelixZkManager());
+ try {
+ tableRebalancer.getInstancePartitionsMap(tableConfig, true, true, true);
+ } catch (Exception e) {
+ throw new RuntimeException(
+ "Failed to calculate instance partitions for table: " +
tableConfig.getTableName() + ", reason: "
+ + e.getMessage());
Review Comment:
This rethrows as a new `RuntimeException` without attaching the original
cause, which drops stack trace context and makes debugging harder. Prefer
including `e` as the cause in the thrown exception (and ideally avoid catching
broad `Exception` if a narrower set of exceptions can be handled).
```suggestion
+ e.getMessage(), e);
```
##########
pinot-controller/src/main/java/org/apache/pinot/controller/api/resources/TableConfigValidationUtils.java:
##########
@@ -0,0 +1,113 @@
+/**
+ * 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 javax.annotation.Nullable;
+import org.apache.pinot.controller.ControllerConf;
+import org.apache.pinot.controller.helix.core.PinotHelixResourceManager;
+import org.apache.pinot.controller.helix.core.minion.PinotTaskManager;
+import org.apache.pinot.controller.helix.core.rebalance.TableRebalancer;
+import org.apache.pinot.controller.util.TaskConfigUtils;
+import org.apache.pinot.segment.local.utils.TableConfigUtils;
+import org.apache.pinot.spi.config.table.TableConfig;
+import org.apache.pinot.spi.config.table.TableType;
+import org.apache.pinot.spi.data.Schema;
+import org.apache.pinot.spi.utils.builder.TableNameBuilder;
+
+
+/**
+ * Utility class that encapsulates table config validation logic shared across
+ * {@link PinotTableRestletResource} and StarTree's managed logical table
resource.
+ *
+ * <p>This lives in {@code pinot-controller} (not {@code
pinot-segment-local}'s {@code TableConfigUtils})
+ * because validation requires controller-level dependencies like {@link
PinotHelixResourceManager},
+ * {@link ControllerConf}, {@link PinotTaskManager}, and {@link
TableRebalancer}.</p>
+ */
+public final class TableConfigValidationUtils {
+
+ private TableConfigValidationUtils() {
+ }
+
+ /**
+ * Validates a table config against the given schema and controller
configuration.
+ *
+ * <p>Performs the following validations in order:</p>
+ * <ol>
+ * <li>Core validation ({@link TableConfigUtils#validate})</li>
+ * <li>Table name validation ({@link
TableConfigUtils#validateTableName})</li>
+ * <li>Min replicas enforcement</li>
+ * <li>Storage quota constraints</li>
+ * <li>Hybrid table config check (if both OFFLINE and REALTIME versions
exist)</li>
+ * <li>Task config validation (skipped if {@code taskManager} is null)</li>
+ * <li>Instance assignment validation</li>
+ * </ol>
+ *
+ * <p><b>NOT included</b> (caller responsibility):</p>
+ * <ul>
+ * <li>Schema retrieval — caller resolves it</li>
+ * <li>Tuner config application — CREATE-only, mutates config</li>
+ * <li>Active tasks check — caller-specific</li>
+ * </ul>
+ *
+ * @param tableConfig the table config to validate
+ * @param schema the schema for the table (must not be null)
+ * @param typesToSkip comma-separated list of validation types to skip
(ALL|TASK|UPSERT), or null
+ * @param resourceManager the Helix resource manager
+ * @param controllerConf the controller configuration
+ * @param taskManager the task manager, or null to skip task validation
+ */
+ public static void validateTableConfig(TableConfig tableConfig, Schema
schema,
+ @Nullable String typesToSkip, PinotHelixResourceManager resourceManager,
+ ControllerConf controllerConf, @Nullable PinotTaskManager taskManager) {
+ TableConfigUtils.validate(tableConfig, schema, typesToSkip);
+ TableConfigUtils.validateTableName(tableConfig);
+ TableConfigUtils.ensureMinReplicas(tableConfig,
controllerConf.getDefaultTableMinReplicas());
+ TableConfigUtils.ensureStorageQuotaConstraints(tableConfig,
controllerConf.getDimTableMaxSize());
+ checkHybridTableConfig(resourceManager, tableConfig);
+ TaskConfigUtils.validateTaskConfigs(tableConfig, schema, taskManager,
typesToSkip);
Review Comment:
The Javadoc states task config validation is skipped when `taskManager` is
null, but the implementation calls `TaskConfigUtils.validateTaskConfigs(...)`
unconditionally. Either update the Javadoc to reflect the actual behavior, or
add an explicit null-guard before invoking task validation so the contract
matches the documentation.
```suggestion
if (taskManager != null) {
TaskConfigUtils.validateTaskConfigs(tableConfig, schema, taskManager,
typesToSkip);
}
```
--
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]
