This is an automated email from the ASF dual-hosted git repository.
nehapawar pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/incubator-pinot.git
The following commit(s) were added to refs/heads/master by this push:
new 9929dad Adding more table config validation (#6073)
9929dad is described below
commit 9929dad2e804f525b8ace925e859e07c6fdee9da
Author: icefury71 <[email protected]>
AuthorDate: Fri Oct 2 09:41:55 2020 -0700
Adding more table config validation (#6073)
Adding more table config validation (retention, pushType and tenant)
Fixing bug where retention manager does not work for real-time tables if
the pushType is missing.
---
.../api/resources/PinotTableRestletResource.java | 2 +-
.../helix/core/PinotHelixResourceManager.java | 28 ++++++++-----
.../helix/core/retention/RetentionManager.java | 5 ++-
.../apache/pinot/core/util/TableConfigUtils.java | 48 +++++++++++++++++++++-
.../pinot/core/util/TableConfigUtilsTest.java | 25 +++++++++++
5 files changed, 93 insertions(+), 15 deletions(-)
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 7060d00..c5da9e1 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
@@ -151,7 +151,7 @@ public class PinotTableRestletResource {
public String recommendConfig(String inputStr) {
try {
return RecommenderDriver.run(inputStr);
- }catch (Exception e){
+ } catch (Exception e) {
throw new ControllerApplicationException(LOGGER, e.getMessage(),
Response.Status.BAD_REQUEST, e);
}
}
diff --git
a/pinot-controller/src/main/java/org/apache/pinot/controller/helix/core/PinotHelixResourceManager.java
b/pinot-controller/src/main/java/org/apache/pinot/controller/helix/core/PinotHelixResourceManager.java
index af8c3be..0cc0a34 100644
---
a/pinot-controller/src/main/java/org/apache/pinot/controller/helix/core/PinotHelixResourceManager.java
+++
b/pinot-controller/src/main/java/org/apache/pinot/controller/helix/core/PinotHelixResourceManager.java
@@ -1095,16 +1095,7 @@ public class PinotHelixResourceManager {
*/
public void addTable(TableConfig tableConfig)
throws IOException {
- TenantConfig tenantConfig = tableConfig.getTenantConfig();
- String brokerTag = tenantConfig.getBroker();
- String serverTag = tenantConfig.getServer();
- if (brokerTag == null || serverTag == null) {
- String newBrokerTag = brokerTag == null ?
TagNameUtils.DEFAULT_TENANT_NAME : brokerTag;
- String newServerTag = serverTag == null ?
TagNameUtils.DEFAULT_TENANT_NAME : serverTag;
- tableConfig.setTenantConfig(new TenantConfig(newBrokerTag, newServerTag,
tenantConfig.getTagOverrideConfig()));
- }
validateTableTenantConfig(tableConfig);
-
String tableNameWithType = tableConfig.getTableName();
SegmentsValidationAndRetentionConfig segmentsConfig =
tableConfig.getValidationConfig();
@@ -1191,12 +1182,27 @@ public class PinotHelixResourceManager {
}
/**
- * Validates the tenant config for the table
+ * Validates the tenant config for the table. In case of a single tenant
cluster,
+ * if the server and broker tenants are not specified in the config, they're
+ * auto-populated with the default tenant name. In case of a multi-tenant
cluster,
+ * these parameters must be specified in the table config.
*/
@VisibleForTesting
void validateTableTenantConfig(TableConfig tableConfig) {
- String tableNameWithType = tableConfig.getTableName();
TenantConfig tenantConfig = tableConfig.getTenantConfig();
+ String tableNameWithType = tableConfig.getTableName();
+ String brokerTag = tenantConfig.getBroker();
+ String serverTag = tenantConfig.getServer();
+ if (brokerTag == null || serverTag == null) {
+ if (!_isSingleTenantCluster) {
+ throw new InvalidTableConfigException(
+ "server and broker tenants must be specified for multi-tenant
cluster for table: " + tableNameWithType);
+ }
+
+ String newBrokerTag = brokerTag == null ?
TagNameUtils.DEFAULT_TENANT_NAME : brokerTag;
+ String newServerTag = serverTag == null ?
TagNameUtils.DEFAULT_TENANT_NAME : serverTag;
+ tableConfig.setTenantConfig(new TenantConfig(newBrokerTag, newServerTag,
tenantConfig.getTagOverrideConfig()));
+ }
// Check if tenant exists before creating the table
Set<String> tagsToCheck = new TreeSet<>();
diff --git
a/pinot-controller/src/main/java/org/apache/pinot/controller/helix/core/retention/RetentionManager.java
b/pinot-controller/src/main/java/org/apache/pinot/controller/helix/core/retention/RetentionManager.java
index c074af8..2793087 100644
---
a/pinot-controller/src/main/java/org/apache/pinot/controller/helix/core/retention/RetentionManager.java
+++
b/pinot-controller/src/main/java/org/apache/pinot/controller/helix/core/retention/RetentionManager.java
@@ -44,6 +44,7 @@ import
org.apache.pinot.controller.helix.core.retention.strategy.RetentionStrate
import
org.apache.pinot.controller.helix.core.retention.strategy.TimeRetentionStrategy;
import org.apache.pinot.spi.config.table.SegmentsValidationAndRetentionConfig;
import org.apache.pinot.spi.config.table.TableConfig;
+import org.apache.pinot.spi.config.table.TableType;
import org.apache.pinot.spi.utils.builder.TableNameBuilder;
import org.apache.pinot.spi.utils.retry.RetryPolicies;
import org.apache.pinot.spi.utils.retry.RetryPolicy;
@@ -96,9 +97,11 @@ public class RetentionManager extends
ControllerPeriodicTask<Void> {
LOGGER.error("Failed to get table config for table: {}",
tableNameWithType);
return;
}
+
+ // For offline tables, ensure that the segmentPushType is APPEND.
SegmentsValidationAndRetentionConfig validationConfig =
tableConfig.getValidationConfig();
String segmentPushType = validationConfig.getSegmentPushType();
- if (!"APPEND".equalsIgnoreCase(segmentPushType)) {
+ if (tableConfig.getTableType() == TableType.OFFLINE &&
!"APPEND".equalsIgnoreCase(segmentPushType)) {
LOGGER.info("Segment push type is not APPEND for table: {}, skip",
tableNameWithType);
return;
}
diff --git
a/pinot-core/src/main/java/org/apache/pinot/core/util/TableConfigUtils.java
b/pinot-core/src/main/java/org/apache/pinot/core/util/TableConfigUtils.java
index caa32d5..bc23bb9 100644
--- a/pinot-core/src/main/java/org/apache/pinot/core/util/TableConfigUtils.java
+++ b/pinot-core/src/main/java/org/apache/pinot/core/util/TableConfigUtils.java
@@ -25,6 +25,7 @@ import java.util.HashSet;
import java.util.List;
import java.util.Map;
import java.util.Set;
+import java.util.concurrent.TimeUnit;
import javax.annotation.Nullable;
import org.apache.pinot.common.tier.TierFactory;
import org.apache.pinot.common.utils.CommonConstants;
@@ -39,6 +40,7 @@ import
org.apache.pinot.spi.config.table.SegmentsValidationAndRetentionConfig;
import org.apache.pinot.spi.config.table.StarTreeIndexConfig;
import org.apache.pinot.spi.config.table.TableConfig;
import org.apache.pinot.spi.config.table.TableType;
+import org.apache.pinot.spi.config.table.TenantConfig;
import org.apache.pinot.spi.config.table.TierConfig;
import org.apache.pinot.spi.config.table.ingestion.FilterConfig;
import org.apache.pinot.spi.config.table.ingestion.TransformConfig;
@@ -62,6 +64,7 @@ public final class TableConfigUtils {
* 2. IngestionConfig
* 3. TierConfigs
* 4. Indexing config
+ * 5. Field Config List
*
* TODO: Add more validations for each section (e.g. validate conditions are
met for aggregateMetrics)
*/
@@ -79,7 +82,7 @@ public final class TableConfigUtils {
/**
* Validates the table name with the following rules:
* <ul>
- * <li>Table name shouldn't contain dot in it</li>
+ * <li>Table name shouldn't contain dot or space in it</li>
* </ul>
*/
public static void validateTableName(TableConfig tableConfig) {
@@ -90,13 +93,52 @@ public final class TableConfigUtils {
}
/**
+ * Validates retention config. Checks for following things:
+ * - Valid segmentPushType
+ * - Valid retentionTimeUnit
+ */
+ public static void validateRetentionConfig(TableConfig tableConfig) {
+ SegmentsValidationAndRetentionConfig segmentsConfig =
tableConfig.getValidationConfig();
+ String tableName = tableConfig.getTableName();
+
+ if (segmentsConfig == null) {
+ throw new IllegalStateException(
+ String.format("Table: %s, \"segmentsConfig\" field is missing in
table config", tableName));
+ }
+
+ String segmentPushType = segmentsConfig.getSegmentPushType();
+ // segmentPushType is not needed for Realtime table
+ if (tableConfig.getTableType() == TableType.OFFLINE) {
+ if (segmentPushType == null) {
+ throw new IllegalStateException(String.format("Table: %s, null push
type", tableName));
+ }
+
+ if (!segmentPushType.equalsIgnoreCase("REFRESH") &&
!segmentPushType.equalsIgnoreCase("APPEND")) {
+ throw new IllegalStateException(String.format("Table: %s, invalid push
type: %s", tableName, segmentPushType));
+ }
+ }
+
+ // Retention may not be specified. Ignore validation in that case.
+ String timeUnitString = segmentsConfig.getRetentionTimeUnit();
+ if (timeUnitString == null || timeUnitString.isEmpty()) {
+ return;
+ }
+ try {
+ TimeUnit.valueOf(timeUnitString.toUpperCase());
+ } catch (Exception e) {
+ throw new IllegalStateException(String.format("Table: %s, invalid time
unit: %s", tableName, timeUnitString));
+ }
+ }
+
+ /**
* Validates the following in the validationConfig of the table
* 1. For REALTIME table
* - checks for non-null timeColumnName
* - checks for valid field spec for timeColumnName in schema
+ * - Validates retention config
*
* 2. For OFFLINE table
- * - checks for valid field spec for timeColumnName in schema, if
timeColumnName and schema re non-null
+ * - checks for valid field spec for timeColumnName in schema, if
timeColumnName and schema are non-null
*
* 3. Checks peerDownloadSchema
*/
@@ -122,6 +164,8 @@ public final class TableConfigUtils {
+ "' for peerSegmentDownloadScheme. Must be one of http or https");
}
}
+
+ validateRetentionConfig(tableConfig);
}
/**
diff --git
a/pinot-core/src/test/java/org/apache/pinot/core/util/TableConfigUtilsTest.java
b/pinot-core/src/test/java/org/apache/pinot/core/util/TableConfigUtilsTest.java
index de2493b..73a12e4 100644
---
a/pinot-core/src/test/java/org/apache/pinot/core/util/TableConfigUtilsTest.java
+++
b/pinot-core/src/test/java/org/apache/pinot/core/util/TableConfigUtilsTest.java
@@ -24,6 +24,7 @@ import java.util.Collections;
import java.util.List;
import java.util.HashMap;
import java.util.Map;
+import java.util.Set;
import org.apache.pinot.common.tier.TierFactory;
import org.apache.pinot.spi.config.table.ColumnPartitionConfig;
import org.apache.pinot.spi.config.table.FieldConfig;
@@ -40,6 +41,7 @@ import org.apache.pinot.spi.data.Schema;
import org.apache.pinot.spi.utils.builder.TableConfigBuilder;
import org.testng.Assert;
import org.testng.annotations.Test;
+import org.testng.collections.Sets;
/**
@@ -635,4 +637,27 @@ public class TableConfigUtilsTest {
// expected
}
}
+
+ @Test
+ public void testValidateRetentionConfig() {
+ Schema schema =
+ new
Schema.SchemaBuilder().setSchemaName(TABLE_NAME).addSingleValueDimension("myCol",
FieldSpec.DataType.STRING)
+ .build();
+ TableConfig tableConfig =
+ new
TableConfigBuilder(TableType.OFFLINE).setTableName(TABLE_NAME).setRetentionTimeUnit("hours")
+ .setRetentionTimeValue("24").build();
+ try {
+ TableConfigUtils.validate(tableConfig, schema);
+ } catch (Exception e) {
+ Assert.fail("Should not fail for valid retention time unit value");
+ }
+
+ tableConfig = new
TableConfigBuilder(TableType.OFFLINE).setTableName(TABLE_NAME).setRetentionTimeUnit("abc").build();
+ try {
+ TableConfigUtils.validate(tableConfig, schema);
+ Assert.fail("Should fail for invalid retention time unit value");
+ } catch (Exception e) {
+ // expected
+ }
+ }
}
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
