Jackie-Jiang commented on code in PR #11574:
URL: https://github.com/apache/pinot/pull/11574#discussion_r1349319401
##########
pinot-controller/src/main/java/org/apache/pinot/controller/BaseControllerStarter.java:
##########
@@ -549,6 +556,83 @@ protected void configure() {
_serviceStatusCallbackList.add(generateServiceStatusCallback(_helixParticipantManager));
}
+ /**
+ * This method is used to fix table/schema names.
+ * TODO: in the next release, maybe 2.0.0, we can remove this method.
Meanwhile we can delete the orphan schemas
+ * that has been existed longer than a certain time period.
+ *
+ */
+ @VisibleForTesting
+ public void fixSchemaNameInTableConfig() {
+ AtomicInteger fixedTableCount = new AtomicInteger();
+ AtomicInteger failedToFixTableCount = new AtomicInteger();
+ AtomicInteger misConfiguredTableCount = new AtomicInteger();
+ List<String> allTables = _helixResourceManager.getAllTables();
+
+ allTables.forEach(table -> {
+ TableConfig tableConfig = _helixResourceManager.getTableConfig(table);
+ if ((tableConfig == null) || (tableConfig.getValidationConfig() ==
null)) {
+ LOGGER.warn("Failed to find table config for table: {}", table);
+ failedToFixTableCount.getAndIncrement();
Review Comment:
This is normal when table is deleted. Suggest not counting it as failed
##########
pinot-controller/src/main/java/org/apache/pinot/controller/BaseControllerStarter.java:
##########
@@ -549,6 +556,83 @@ protected void configure() {
_serviceStatusCallbackList.add(generateServiceStatusCallback(_helixParticipantManager));
}
+ /**
+ * This method is used to fix table/schema names.
+ * TODO: in the next release, maybe 2.0.0, we can remove this method.
Meanwhile we can delete the orphan schemas
+ * that has been existed longer than a certain time period.
+ *
+ */
+ @VisibleForTesting
+ public void fixSchemaNameInTableConfig() {
+ AtomicInteger fixedTableCount = new AtomicInteger();
+ AtomicInteger failedToFixTableCount = new AtomicInteger();
+ AtomicInteger misConfiguredTableCount = new AtomicInteger();
+ List<String> allTables = _helixResourceManager.getAllTables();
+
+ allTables.forEach(table -> {
+ TableConfig tableConfig = _helixResourceManager.getTableConfig(table);
+ if ((tableConfig == null) || (tableConfig.getValidationConfig() ==
null)) {
+ LOGGER.warn("Failed to find table config for table: {}", table);
+ failedToFixTableCount.getAndIncrement();
+ return;
+ }
+ String rawTableName =
TableNameBuilder.extractRawTableName(tableConfig.getTableName());
+ String existSchemaName =
tableConfig.getValidationConfig().getSchemaName();
+ if (existSchemaName == null) {
+ // Although the table config is valid, we still need to ensure the
schema exists
+ if (_helixResourceManager.getSchema(rawTableName) == null) {
+ LOGGER.warn("Failed to find schema for table: {}", rawTableName);
+ failedToFixTableCount.getAndIncrement();
+ return;
+ }
+ // Table config is already in good status
+ return;
+ }
+ if (_helixResourceManager.getSchema(rawTableName) != null) {
+ // If a schema named `rawTableName` already exists, then likely this
is a misconfiguration.
+ // Reset schema name in table config to null to let the table point to
the existing schema.
+ LOGGER.warn("Schema: {} already exists, fix the schema name in table
config from {} to null", rawTableName,
+ existSchemaName);
+ misConfiguredTableCount.getAndIncrement();
+ } else {
+ // Copy the schema current table referring to to `rawTableName` if it
does not exist
+ Schema schema = _helixResourceManager.getSchema(existSchemaName);
+ if (schema == null) {
+ LOGGER.warn("Failed to find schema for schema name: {}",
existSchemaName);
+ failedToFixTableCount.getAndIncrement();
+ return;
+ }
+ schema.setSchemaName(rawTableName);
+ try {
+ _helixResourceManager.addSchema(schema, false, false);
+ LOGGER.info("Copied schema: {} to {}", existSchemaName,
rawTableName);
+ } catch (Exception e) {
+ LOGGER.error("Failed to copy schema: {} to {}", existSchemaName,
rawTableName, e);
+ failedToFixTableCount.getAndIncrement();
Review Comment:
Make it more specific - `failedToCopySchemaCount`
##########
pinot-controller/src/main/java/org/apache/pinot/controller/ControllerConf.java:
##########
@@ -764,7 +764,8 @@ public void
setMinionInstancesCleanupTaskInitialDelaySeconds(int initialDelaySec
@Deprecated
public int
getMinionInstancesCleanupTaskMinOfflineTimeBeforeDeletionInSeconds() {
return Optional.ofNullable(
-
getProperty(ControllerPeriodicTasksConf.MINION_INSTANCES_CLEANUP_TASK_MIN_OFFLINE_TIME_BEFORE_DELETION_PERIOD))
+ getProperty(
Review Comment:
(minor) Revert this
##########
pinot-common/src/main/java/org/apache/pinot/common/metrics/ControllerGauge.java:
##########
@@ -132,7 +132,16 @@ public enum ControllerGauge implements
AbstractMetrics.Gauge {
MAX_RECORDS_LAG("maxRecordsLag", false),
// Consumption availability lag in ms at a partition level
- MAX_RECORD_AVAILABILITY_LAG_MS("maxRecordAvailabilityLagMs", false);
+ MAX_RECORD_AVAILABILITY_LAG_MS("maxRecordAvailabilityLagMs", false),
+
+ // Number of table schema got fixed
+ FIXED_SCHEMA_TABLE_COUNT("FixedSchemaTableCount", true),
Review Comment:
Do we have these configs covered in the prometheus yaml?
##########
pinot-controller/src/main/java/org/apache/pinot/controller/BaseControllerStarter.java:
##########
@@ -549,6 +556,83 @@ protected void configure() {
_serviceStatusCallbackList.add(generateServiceStatusCallback(_helixParticipantManager));
}
+ /**
+ * This method is used to fix table/schema names.
+ * TODO: in the next release, maybe 2.0.0, we can remove this method.
Meanwhile we can delete the orphan schemas
+ * that has been existed longer than a certain time period.
+ *
+ */
+ @VisibleForTesting
+ public void fixSchemaNameInTableConfig() {
+ AtomicInteger fixedTableCount = new AtomicInteger();
+ AtomicInteger failedToFixTableCount = new AtomicInteger();
+ AtomicInteger misConfiguredTableCount = new AtomicInteger();
+ List<String> allTables = _helixResourceManager.getAllTables();
+
+ allTables.forEach(table -> {
+ TableConfig tableConfig = _helixResourceManager.getTableConfig(table);
+ if ((tableConfig == null) || (tableConfig.getValidationConfig() ==
null)) {
+ LOGGER.warn("Failed to find table config for table: {}", table);
+ failedToFixTableCount.getAndIncrement();
+ return;
+ }
+ String rawTableName =
TableNameBuilder.extractRawTableName(tableConfig.getTableName());
+ String existSchemaName =
tableConfig.getValidationConfig().getSchemaName();
+ if (existSchemaName == null) {
+ // Although the table config is valid, we still need to ensure the
schema exists
+ if (_helixResourceManager.getSchema(rawTableName) == null) {
+ LOGGER.warn("Failed to find schema for table: {}", rawTableName);
+ failedToFixTableCount.getAndIncrement();
+ return;
+ }
+ // Table config is already in good status
+ return;
+ }
+ if (_helixResourceManager.getSchema(rawTableName) != null) {
+ // If a schema named `rawTableName` already exists, then likely this
is a misconfiguration.
+ // Reset schema name in table config to null to let the table point to
the existing schema.
+ LOGGER.warn("Schema: {} already exists, fix the schema name in table
config from {} to null", rawTableName,
+ existSchemaName);
+ misConfiguredTableCount.getAndIncrement();
Review Comment:
I wouldn't count it as mis-configuration if the schema name is the same as
raw table name. Maybe modify line 581 to `if (existSchemaName == null ||
existSchemaName.equals(rawTableName)`
##########
pinot-controller/src/main/java/org/apache/pinot/controller/BaseControllerStarter.java:
##########
@@ -549,6 +556,83 @@ protected void configure() {
_serviceStatusCallbackList.add(generateServiceStatusCallback(_helixParticipantManager));
}
+ /**
+ * This method is used to fix table/schema names.
+ * TODO: in the next release, maybe 2.0.0, we can remove this method.
Meanwhile we can delete the orphan schemas
+ * that has been existed longer than a certain time period.
+ *
+ */
+ @VisibleForTesting
+ public void fixSchemaNameInTableConfig() {
+ AtomicInteger fixedTableCount = new AtomicInteger();
+ AtomicInteger failedToFixTableCount = new AtomicInteger();
+ AtomicInteger misConfiguredTableCount = new AtomicInteger();
+ List<String> allTables = _helixResourceManager.getAllTables();
+
+ allTables.forEach(table -> {
+ TableConfig tableConfig = _helixResourceManager.getTableConfig(table);
+ if ((tableConfig == null) || (tableConfig.getValidationConfig() ==
null)) {
+ LOGGER.warn("Failed to find table config for table: {}", table);
+ failedToFixTableCount.getAndIncrement();
+ return;
+ }
+ String rawTableName =
TableNameBuilder.extractRawTableName(tableConfig.getTableName());
+ String existSchemaName =
tableConfig.getValidationConfig().getSchemaName();
+ if (existSchemaName == null) {
+ // Although the table config is valid, we still need to ensure the
schema exists
+ if (_helixResourceManager.getSchema(rawTableName) == null) {
+ LOGGER.warn("Failed to find schema for table: {}", rawTableName);
+ failedToFixTableCount.getAndIncrement();
Review Comment:
Let's call it `tableWithoutSchemaCount`
##########
pinot-controller/src/main/java/org/apache/pinot/controller/BaseControllerStarter.java:
##########
@@ -549,6 +556,83 @@ protected void configure() {
_serviceStatusCallbackList.add(generateServiceStatusCallback(_helixParticipantManager));
}
+ /**
+ * This method is used to fix table/schema names.
+ * TODO: in the next release, maybe 2.0.0, we can remove this method.
Meanwhile we can delete the orphan schemas
+ * that has been existed longer than a certain time period.
+ *
+ */
+ @VisibleForTesting
+ public void fixSchemaNameInTableConfig() {
+ AtomicInteger fixedTableCount = new AtomicInteger();
+ AtomicInteger failedToFixTableCount = new AtomicInteger();
+ AtomicInteger misConfiguredTableCount = new AtomicInteger();
+ List<String> allTables = _helixResourceManager.getAllTables();
+
+ allTables.forEach(table -> {
+ TableConfig tableConfig = _helixResourceManager.getTableConfig(table);
+ if ((tableConfig == null) || (tableConfig.getValidationConfig() ==
null)) {
+ LOGGER.warn("Failed to find table config for table: {}", table);
+ failedToFixTableCount.getAndIncrement();
+ return;
+ }
+ String rawTableName =
TableNameBuilder.extractRawTableName(tableConfig.getTableName());
+ String existSchemaName =
tableConfig.getValidationConfig().getSchemaName();
+ if (existSchemaName == null) {
+ // Although the table config is valid, we still need to ensure the
schema exists
+ if (_helixResourceManager.getSchema(rawTableName) == null) {
+ LOGGER.warn("Failed to find schema for table: {}", rawTableName);
+ failedToFixTableCount.getAndIncrement();
+ return;
+ }
+ // Table config is already in good status
+ return;
+ }
+ if (_helixResourceManager.getSchema(rawTableName) != null) {
+ // If a schema named `rawTableName` already exists, then likely this
is a misconfiguration.
+ // Reset schema name in table config to null to let the table point to
the existing schema.
+ LOGGER.warn("Schema: {} already exists, fix the schema name in table
config from {} to null", rawTableName,
+ existSchemaName);
+ misConfiguredTableCount.getAndIncrement();
+ } else {
+ // Copy the schema current table referring to to `rawTableName` if it
does not exist
+ Schema schema = _helixResourceManager.getSchema(existSchemaName);
+ if (schema == null) {
+ LOGGER.warn("Failed to find schema for schema name: {}",
existSchemaName);
+ failedToFixTableCount.getAndIncrement();
+ return;
+ }
+ schema.setSchemaName(rawTableName);
+ try {
+ _helixResourceManager.addSchema(schema, false, false);
+ LOGGER.info("Copied schema: {} to {}", existSchemaName,
rawTableName);
+ } catch (Exception e) {
+ LOGGER.error("Failed to copy schema: {} to {}", existSchemaName,
rawTableName, e);
+ failedToFixTableCount.getAndIncrement();
+ return;
+ }
+ }
+ // Update table config to remove schema name
+ tableConfig.getValidationConfig().setSchemaName(null);
+ try {
+ _helixResourceManager.updateTableConfig(tableConfig);
Review Comment:
We need to do version check and update. Currently it might override another
table config update.
##########
pinot-controller/src/main/java/org/apache/pinot/controller/BaseControllerStarter.java:
##########
@@ -549,6 +556,83 @@ protected void configure() {
_serviceStatusCallbackList.add(generateServiceStatusCallback(_helixParticipantManager));
}
+ /**
+ * This method is used to fix table/schema names.
+ * TODO: in the next release, maybe 2.0.0, we can remove this method.
Meanwhile we can delete the orphan schemas
+ * that has been existed longer than a certain time period.
+ *
+ */
+ @VisibleForTesting
+ public void fixSchemaNameInTableConfig() {
+ AtomicInteger fixedTableCount = new AtomicInteger();
+ AtomicInteger failedToFixTableCount = new AtomicInteger();
+ AtomicInteger misConfiguredTableCount = new AtomicInteger();
+ List<String> allTables = _helixResourceManager.getAllTables();
+
+ allTables.forEach(table -> {
+ TableConfig tableConfig = _helixResourceManager.getTableConfig(table);
+ if ((tableConfig == null) || (tableConfig.getValidationConfig() ==
null)) {
+ LOGGER.warn("Failed to find table config for table: {}", table);
+ failedToFixTableCount.getAndIncrement();
+ return;
+ }
+ String rawTableName =
TableNameBuilder.extractRawTableName(tableConfig.getTableName());
+ String existSchemaName =
tableConfig.getValidationConfig().getSchemaName();
+ if (existSchemaName == null) {
+ // Although the table config is valid, we still need to ensure the
schema exists
+ if (_helixResourceManager.getSchema(rawTableName) == null) {
+ LOGGER.warn("Failed to find schema for table: {}", rawTableName);
+ failedToFixTableCount.getAndIncrement();
+ return;
+ }
+ // Table config is already in good status
+ return;
+ }
+ if (_helixResourceManager.getSchema(rawTableName) != null) {
+ // If a schema named `rawTableName` already exists, then likely this
is a misconfiguration.
+ // Reset schema name in table config to null to let the table point to
the existing schema.
+ LOGGER.warn("Schema: {} already exists, fix the schema name in table
config from {} to null", rawTableName,
+ existSchemaName);
+ misConfiguredTableCount.getAndIncrement();
+ } else {
+ // Copy the schema current table referring to to `rawTableName` if it
does not exist
+ Schema schema = _helixResourceManager.getSchema(existSchemaName);
+ if (schema == null) {
+ LOGGER.warn("Failed to find schema for schema name: {}",
existSchemaName);
+ failedToFixTableCount.getAndIncrement();
Review Comment:
If we reach here, it means the schema name is not the same as raw table
name, and the schema does not exist. We can probably count it as both
`tableWithoutSchame` and `misConfiguredTable`
--
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]
