This is an automated email from the ASF dual-hosted git repository.
kharekartik pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/pinot.git
The following commit(s) were added to refs/heads/master by this push:
new 88f523afb22 Refactor table config validations for create and update
table to TableConfigValidationUtils (#17679)
88f523afb22 is described below
commit 88f523afb225aebdecd74c532378bb8571c45635
Author: Krishan Goyal <[email protected]>
AuthorDate: Thu Feb 12 12:26:32 2026 +0530
Refactor table config validations for create and update table to
TableConfigValidationUtils (#17679)
* Refactor table config validations for create and update table to
TableConfigValidationUtils
* Move translatePhysicalTableNamesWithDB to LogicalTableConfigUtils
* cstyle fixes
* javadoc update
* Minor improvements
---
.../common/utils/LogicalTableConfigUtils.java | 22 +++++
.../api/resources/PinotLogicalTableResource.java | 27 +------
.../api/resources/PinotTableRestletResource.java | 55 +------------
.../api/resources/TableConfigValidationUtils.java | 94 ++++++++++++++++++++++
.../controller/helix/ControllerRequestClient.java | 24 ++++++
.../segment/local/utils/TableConfigUtils.java | 2 +-
6 files changed, 148 insertions(+), 76 deletions(-)
diff --git
a/pinot-common/src/main/java/org/apache/pinot/common/utils/LogicalTableConfigUtils.java
b/pinot-common/src/main/java/org/apache/pinot/common/utils/LogicalTableConfigUtils.java
index 9e1bbc3d8f0..99a593845e5 100644
---
a/pinot-common/src/main/java/org/apache/pinot/common/utils/LogicalTableConfigUtils.java
+++
b/pinot-common/src/main/java/org/apache/pinot/common/utils/LogicalTableConfigUtils.java
@@ -25,6 +25,8 @@ import java.util.HashSet;
import java.util.Map;
import java.util.Set;
import java.util.function.Predicate;
+import java.util.stream.Collectors;
+import javax.ws.rs.core.HttpHeaders;
import org.apache.commons.lang3.StringUtils;
import org.apache.helix.store.zk.ZkHelixPropertyStore;
import org.apache.helix.zookeeper.datamodel.ZNRecord;
@@ -284,4 +286,24 @@ public class LogicalTableConfigUtils {
return physicalTableNames.contains(tableName);
}
}
+
+ public static void translatePhysicalTableNamesWithDB(LogicalTableConfig
logicalTableConfig, HttpHeaders headers) {
+ // Translate physical table names to include the database name
+ Map<String, PhysicalTableConfig> physicalTableConfigMap =
logicalTableConfig.getPhysicalTableConfigMap().entrySet()
+ .stream()
+ .map(entry ->
Map.entry(DatabaseUtils.translateTableName(entry.getKey(), headers),
entry.getValue()))
+ .collect(Collectors.toMap(Map.Entry::getKey, Map.Entry::getValue));
+
+ logicalTableConfig.setPhysicalTableConfigMap(physicalTableConfigMap);
+
+ // Translate refOfflineTableName and refRealtimeTableName to include the
database name
+ String refOfflineTableName = logicalTableConfig.getRefOfflineTableName();
+ if (refOfflineTableName != null) {
+
logicalTableConfig.setRefOfflineTableName(DatabaseUtils.translateTableName(refOfflineTableName,
headers));
+ }
+ String refRealtimeTableName = logicalTableConfig.getRefRealtimeTableName();
+ if (refRealtimeTableName != null) {
+
logicalTableConfig.setRefRealtimeTableName(DatabaseUtils.translateTableName(refRealtimeTableName,
headers));
+ }
+ }
}
diff --git
a/pinot-controller/src/main/java/org/apache/pinot/controller/api/resources/PinotLogicalTableResource.java
b/pinot-controller/src/main/java/org/apache/pinot/controller/api/resources/PinotLogicalTableResource.java
index 778d9b3724a..3cd4416307e 100644
---
a/pinot-controller/src/main/java/org/apache/pinot/controller/api/resources/PinotLogicalTableResource.java
+++
b/pinot-controller/src/main/java/org/apache/pinot/controller/api/resources/PinotLogicalTableResource.java
@@ -30,7 +30,6 @@ import io.swagger.annotations.SecurityDefinition;
import io.swagger.annotations.SwaggerDefinition;
import java.util.List;
import java.util.Map;
-import java.util.stream.Collectors;
import javax.inject.Inject;
import javax.ws.rs.Consumes;
import javax.ws.rs.DELETE;
@@ -47,6 +46,7 @@ import javax.ws.rs.core.Response;
import org.apache.commons.lang3.tuple.Pair;
import org.apache.pinot.common.exception.TableNotFoundException;
import org.apache.pinot.common.utils.DatabaseUtils;
+import org.apache.pinot.common.utils.LogicalTableConfigUtils;
import org.apache.pinot.controller.api.access.AccessControlFactory;
import org.apache.pinot.controller.api.access.AccessType;
import org.apache.pinot.controller.api.access.Authenticate;
@@ -58,7 +58,6 @@ import org.apache.pinot.core.auth.Authorize;
import org.apache.pinot.core.auth.ManualAuthorization;
import org.apache.pinot.core.auth.TargetType;
import org.apache.pinot.spi.data.LogicalTableConfig;
-import org.apache.pinot.spi.data.PhysicalTableConfig;
import org.apache.pinot.spi.utils.JsonUtils;
import org.glassfish.grizzly.http.server.Request;
import org.slf4j.Logger;
@@ -143,31 +142,11 @@ public class PinotLogicalTableResource {
ResourceUtils.checkPermissionAndAccess(tableName, request, httpHeaders,
AccessType.CREATE,
Actions.Table.CREATE_TABLE, _accessControlFactory, LOGGER);
- translatePhysicalTableNamesWithDB(logicalTableConfig, httpHeaders);
+
LogicalTableConfigUtils.translatePhysicalTableNamesWithDB(logicalTableConfig,
httpHeaders);
SuccessResponse successResponse = addLogicalTable(logicalTableConfig);
return new ConfigSuccessResponse(successResponse.getStatus(),
logicalTableConfigAndUnrecognizedProps.getRight());
}
- private void translatePhysicalTableNamesWithDB(LogicalTableConfig
logicalTableConfig, HttpHeaders headers) {
- // Translate physical table names to include the database name
- Map<String, PhysicalTableConfig> physicalTableConfigMap =
logicalTableConfig.getPhysicalTableConfigMap().entrySet()
- .stream()
- .map(entry ->
Map.entry(DatabaseUtils.translateTableName(entry.getKey(), headers),
entry.getValue()))
- .collect(Collectors.toMap(Map.Entry::getKey, Map.Entry::getValue));
-
- logicalTableConfig.setPhysicalTableConfigMap(physicalTableConfigMap);
-
- // Translate refOfflineTableName and refRealtimeTableName to include the
database name
- String refOfflineTableName = logicalTableConfig.getRefOfflineTableName();
- if (refOfflineTableName != null) {
-
logicalTableConfig.setRefOfflineTableName(DatabaseUtils.translateTableName(refOfflineTableName,
headers));
- }
- String refRealtimeTableName = logicalTableConfig.getRefRealtimeTableName();
- if (refRealtimeTableName != null) {
-
logicalTableConfig.setRefRealtimeTableName(DatabaseUtils.translateTableName(refRealtimeTableName,
headers));
- }
- }
-
@PUT
@Produces(MediaType.APPLICATION_JSON)
@Consumes(MediaType.APPLICATION_JSON)
@@ -193,7 +172,7 @@ public class PinotLogicalTableResource {
tableName = DatabaseUtils.translateTableName(tableName, headers);
logicalTableConfig.setTableName(tableName);
- translatePhysicalTableNamesWithDB(logicalTableConfig, headers);
+
LogicalTableConfigUtils.translatePhysicalTableNamesWithDB(logicalTableConfig,
headers);
SuccessResponse successResponse = updateLogicalTable(logicalTableConfig);
return new ConfigSuccessResponse(successResponse.getStatus(),
logicalTableConfigAndUnrecognizedProps.getRight());
}
diff --git
a/pinot-controller/src/main/java/org/apache/pinot/controller/api/resources/PinotTableRestletResource.java
b/pinot-controller/src/main/java/org/apache/pinot/controller/api/resources/PinotTableRestletResource.java
index 74df24014dd..9d934c4d184 100644
---
a/pinot-controller/src/main/java/org/apache/pinot/controller/api/resources/PinotTableRestletResource.java
+++
b/pinot-controller/src/main/java/org/apache/pinot/controller/api/resources/PinotTableRestletResource.java
@@ -248,24 +248,14 @@ public class PinotTableRestletResource {
TableConfigTunerUtils.applyTunerConfigs(_pinotHelixResourceManager,
tableConfig, schema, Collections.emptyMap());
- // TableConfigUtils.validate(...) is used across table create/update.
- TableConfigUtils.validate(tableConfig, schema, typesToSkip);
- TableConfigUtils.validateTableName(tableConfig);
+ TableConfigValidationUtils.validateTableConfig(
+ tableConfig, schema, typesToSkip, _pinotHelixResourceManager,
_controllerConf, _pinotTaskManager);
} catch (TableAlreadyExistsException e) {
throw new ControllerApplicationException(LOGGER, e.getMessage(),
Response.Status.CONFLICT, e);
} catch (Exception e) {
throw new ControllerApplicationException(LOGGER, e.getMessage(),
Response.Status.BAD_REQUEST, e);
}
try {
- try {
- TableConfigUtils.ensureMinReplicas(tableConfig,
_controllerConf.getDefaultTableMinReplicas());
- TableConfigUtils.ensureStorageQuotaConstraints(tableConfig,
_controllerConf.getDimTableMaxSize());
-
checkHybridTableConfig(TableNameBuilder.extractRawTableName(tableNameWithType),
tableConfig);
- TaskConfigUtils.validateTaskConfigs(tableConfig, schema,
_pinotTaskManager, typesToSkip);
- validateInstanceAssignment(tableConfig);
- } catch (Exception e) {
- throw new InvalidTableConfigException(e);
- }
if (!ignoreActiveTasks) {
tableTasksValidation(tableConfig, _pinotHelixTaskResourceManager);
}
@@ -760,7 +750,8 @@ public class PinotTableRestletResource {
schema = _pinotHelixResourceManager.getTableSchema(tableNameWithType);
Preconditions.checkState(schema != null, "Failed to find schema for
table: %s", tableNameWithType);
- TableConfigUtils.validate(tableConfig, schema, typesToSkip);
+ TableConfigValidationUtils.validateTableConfig(
+ tableConfig, schema, typesToSkip, _pinotHelixResourceManager,
_controllerConf, _pinotTaskManager);
} catch (Exception e) {
String msg = String.format("Invalid table config: %s with error: %s",
tableName, e.getMessage());
throw new ControllerApplicationException(LOGGER, msg,
Response.Status.BAD_REQUEST, e);
@@ -771,16 +762,6 @@ public class PinotTableRestletResource {
throw new ControllerApplicationException(LOGGER, "Table " +
tableNameWithType + " does not exist",
Response.Status.NOT_FOUND);
}
-
- try {
- TableConfigUtils.ensureMinReplicas(tableConfig,
_controllerConf.getDefaultTableMinReplicas());
- TableConfigUtils.ensureStorageQuotaConstraints(tableConfig,
_controllerConf.getDimTableMaxSize());
-
checkHybridTableConfig(TableNameBuilder.extractRawTableName(tableNameWithType),
tableConfig);
- TaskConfigUtils.validateTaskConfigs(tableConfig, schema,
_pinotTaskManager, typesToSkip);
- validateInstanceAssignment(tableConfig);
- } catch (Exception e) {
- throw new InvalidTableConfigException(e);
- }
_pinotHelixResourceManager.updateTableConfig(tableConfig);
} catch (InvalidTableConfigException e) {
String errStr = String.format("Failed to update configuration for %s due
to: %s", tableName, e.getMessage());
@@ -1177,20 +1158,6 @@ public class PinotTableRestletResource {
return TableNameBuilder.forType(tableType).tableNameWithType(tableName);
}
- private void checkHybridTableConfig(String rawTableName, TableConfig
tableConfig) {
- if (tableConfig.getTableType() == TableType.REALTIME) {
- if (_pinotHelixResourceManager.hasOfflineTable(rawTableName)) {
- TableConfigUtils.verifyHybridTableConfigs(rawTableName,
- _pinotHelixResourceManager.getOfflineTableConfig(rawTableName),
tableConfig);
- }
- } else {
- if (_pinotHelixResourceManager.hasRealtimeTable(rawTableName)) {
- TableConfigUtils.verifyHybridTableConfigs(rawTableName, tableConfig,
- _pinotHelixResourceManager.getRealtimeTableConfig(rawTableName));
- }
- }
- }
-
@GET
@Path("/tables/{tableName}/status")
@Authorize(targetType = TargetType.TABLE, paramName = "tableName", action =
Actions.Table.GET_METADATA)
@@ -1546,18 +1513,4 @@ public class PinotTableRestletResource {
return timeBoundaryMs;
}
-
- /**
- * Try to calculate the instance partitions for the given table config.
Throws exception if it fails.
- */
- private void validateInstanceAssignment(TableConfig tableConfig) {
- TableRebalancer tableRebalancer = new
TableRebalancer(_pinotHelixResourceManager.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());
- }
- }
}
diff --git
a/pinot-controller/src/main/java/org/apache/pinot/controller/api/resources/TableConfigValidationUtils.java
b/pinot-controller/src/main/java/org/apache/pinot/controller/api/resources/TableConfigValidationUtils.java
new file mode 100644
index 00000000000..1b3d1c0cfe7
--- /dev/null
+++
b/pinot-controller/src/main/java/org/apache/pinot/controller/api/resources/TableConfigValidationUtils.java
@@ -0,0 +1,94 @@
+/**
+ * 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.
+ *
+ * <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.
+ *
+ * @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(), e);
+ }
+ }
+}
diff --git
a/pinot-controller/src/main/java/org/apache/pinot/controller/helix/ControllerRequestClient.java
b/pinot-controller/src/main/java/org/apache/pinot/controller/helix/ControllerRequestClient.java
index 0b2ce9fd3c7..1c78ea13061 100644
---
a/pinot-controller/src/main/java/org/apache/pinot/controller/helix/ControllerRequestClient.java
+++
b/pinot-controller/src/main/java/org/apache/pinot/controller/helix/ControllerRequestClient.java
@@ -209,6 +209,30 @@ public class ControllerRequestClient {
}
}
+ 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);
+ }
+ }
+
public TableConfig getTableConfig(String tableName, TableType tableType)
throws IOException {
try {
diff --git
a/pinot-segment-local/src/main/java/org/apache/pinot/segment/local/utils/TableConfigUtils.java
b/pinot-segment-local/src/main/java/org/apache/pinot/segment/local/utils/TableConfigUtils.java
index f7e89f6660c..834516ef417 100644
---
a/pinot-segment-local/src/main/java/org/apache/pinot/segment/local/utils/TableConfigUtils.java
+++
b/pinot-segment-local/src/main/java/org/apache/pinot/segment/local/utils/TableConfigUtils.java
@@ -165,7 +165,7 @@ public final class TableConfigUtils {
* 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());
Set<ValidationType> skipTypes = parseTypesToSkipString(typesToSkip);
// Sanitize the table config before validation
sanitize(tableConfig);
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
