Jackie-Jiang commented on code in PR #11782:
URL: https://github.com/apache/pinot/pull/11782#discussion_r1357530963
##########
pinot-common/src/main/java/org/apache/pinot/common/metadata/ZKMetadataProvider.java:
##########
@@ -73,12 +74,61 @@ public static void
setUserConfig(ZkHelixPropertyStore<ZNRecord> propertyStore, S
propertyStore.set(constructPropertyStorePathForUserConfig(username),
znRecord, AccessOption.PERSISTENT);
}
+ @Deprecated
public static void setTableConfig(ZkHelixPropertyStore<ZNRecord>
propertyStore, String tableNameWithType,
ZNRecord znRecord) {
propertyStore.set(constructPropertyStorePathForResourceConfig(tableNameWithType),
znRecord,
AccessOption.PERSISTENT);
}
+ /**
+ * Create table config, fail if existed.
+ *
+ * @return true if creation is successful.
+ */
+ public static boolean createTableConfig(ZkHelixPropertyStore<ZNRecord>
propertyStore, String tableNameWithType,
+ TableConfig tableConfig) {
+ String tableConfigPath =
constructPropertyStorePathForResourceConfig(tableNameWithType);
+ ZNRecord tableConfigZNRecord;
+ try {
+ tableConfigZNRecord = TableConfigUtils.toZNRecord(tableConfig);
+ } catch (Exception e) {
+ LOGGER.error("Caught exception constructing ZNRecord from table config
for table: {}", tableNameWithType, e);
+ return false;
+ }
+ return propertyStore.create(tableConfigPath, tableConfigZNRecord,
AccessOption.PERSISTENT);
+ }
+
+ /**
+ * Full override table config.
+ *
+ * @return true if update is successful.
+ */
+ public static boolean setTableConfig(ZkHelixPropertyStore<ZNRecord>
propertyStore, String tableNameWithType,
+ TableConfig tableConfig) {
+ return setTableConfig(propertyStore, tableNameWithType, tableConfig, -1);
+ }
+
+ /**
+ * Update table config with an expected version. This is to avoid race
condition for table config update issued by
+ * multiple clients, especially when update configs in a programmatic way.
+ * The typical usage is to read table config, apply some changes, then
update it.
+ *
+ * @return true if update is successful.
+ */
+ public static boolean setTableConfig(ZkHelixPropertyStore<ZNRecord>
propertyStore, String tableNameWithType,
+ TableConfig tableConfig, int expectVersion) {
Review Comment:
```suggestion
TableConfig tableConfig, int expectedVersion) {
```
##########
pinot-common/src/main/java/org/apache/pinot/common/metadata/ZKMetadataProvider.java:
##########
@@ -73,12 +74,61 @@ public static void
setUserConfig(ZkHelixPropertyStore<ZNRecord> propertyStore, S
propertyStore.set(constructPropertyStorePathForUserConfig(username),
znRecord, AccessOption.PERSISTENT);
}
+ @Deprecated
public static void setTableConfig(ZkHelixPropertyStore<ZNRecord>
propertyStore, String tableNameWithType,
ZNRecord znRecord) {
propertyStore.set(constructPropertyStorePathForResourceConfig(tableNameWithType),
znRecord,
AccessOption.PERSISTENT);
}
+ /**
+ * Create table config, fail if existed.
+ *
+ * @return true if creation is successful.
+ */
+ public static boolean createTableConfig(ZkHelixPropertyStore<ZNRecord>
propertyStore, String tableNameWithType,
Review Comment:
Let's remove `tableNameWithType`, which is just
`tableConfig.getTableName()`. Same for the other 2 methods
##########
pinot-controller/src/main/java/org/apache/pinot/controller/helix/core/PinotHelixResourceManager.java:
##########
@@ -1559,33 +1560,32 @@ public void addTable(TableConfig tableConfig)
if (ZKMetadataProvider.getSchema(_propertyStore,
TableNameBuilder.extractRawTableName(tableNameWithType)) == null) {
throw new InvalidTableConfigException("No schema defined for table: " +
tableNameWithType);
}
- if (tableType == TableType.OFFLINE) {
- try {
- // Add table config
- ZKMetadataProvider.setTableConfig(_propertyStore, tableNameWithType,
TableConfigUtils.toZNRecord(tableConfig));
- // Assign instances
- assignInstances(tableConfig, true);
+ Preconditions.checkState(tableType == TableType.OFFLINE || tableType ==
TableType.REALTIME,
+ "Invalid table type: %s", tableType);
+ try {
+ // Add table config
+ if (!ZKMetadataProvider.createTableConfig(_propertyStore,
tableNameWithType, tableConfig)) {
+ if (getTableConfig(tableNameWithType) != null) {
Review Comment:
We already have this check on line 1542, so IMO we can directly throw the
`RuntimeException`
##########
pinot-controller/src/main/java/org/apache/pinot/controller/helix/core/PinotHelixResourceManager.java:
##########
@@ -1559,33 +1560,32 @@ public void addTable(TableConfig tableConfig)
if (ZKMetadataProvider.getSchema(_propertyStore,
TableNameBuilder.extractRawTableName(tableNameWithType)) == null) {
throw new InvalidTableConfigException("No schema defined for table: " +
tableNameWithType);
}
- if (tableType == TableType.OFFLINE) {
- try {
- // Add table config
- ZKMetadataProvider.setTableConfig(_propertyStore, tableNameWithType,
TableConfigUtils.toZNRecord(tableConfig));
- // Assign instances
- assignInstances(tableConfig, true);
+ Preconditions.checkState(tableType == TableType.OFFLINE || tableType ==
TableType.REALTIME,
+ "Invalid table type: %s", tableType);
+ try {
+ // Add table config
+ if (!ZKMetadataProvider.createTableConfig(_propertyStore,
tableNameWithType, tableConfig)) {
+ if (getTableConfig(tableNameWithType) != null) {
+ throw new TableAlreadyExistsException("Table config for " +
tableNameWithType
+ + " already exists. If this is unexpected, try deleting the
table to remove all metadata associated"
+ + " with it.");
+ } else {
+ throw new RuntimeException("Failed to create table config for table:
" + tableNameWithType);
+ }
+ }
+ // Assign instances
+ assignInstances(tableConfig, true);
+ if (tableType == TableType.OFFLINE) {
// Add ideal state
_helixAdmin.addResource(_helixClusterName, tableNameWithType,
idealState);
- } catch (Exception e) {
- LOGGER.error("Caught exception during offline table setup. Cleaning up
table {}", tableNameWithType, e);
- deleteOfflineTable(tableNameWithType);
- throw e;
- }
- } else {
- Preconditions.checkState(tableType == TableType.REALTIME, "Invalid table
type: %s", tableType);
- try {
- // Add table config
- ZKMetadataProvider.setTableConfig(_propertyStore, tableNameWithType,
TableConfigUtils.toZNRecord(tableConfig));
- // Assign instances
- assignInstances(tableConfig, true);
+ } else {
// Add ideal state with the first CONSUMING segment
_pinotLLCRealtimeSegmentManager.setUpNewTable(tableConfig, idealState);
- } catch (Exception e) {
- LOGGER.error("Caught exception during realtime table setup. Cleaning
up table {}", tableNameWithType, e);
- deleteRealtimeTable(tableNameWithType);
- throw e;
}
+ } catch (Exception e) {
+ LOGGER.error("Caught exception during offline table setup. Cleaning up
table {}", tableNameWithType, e);
+ deleteTable(tableNameWithType, tableType, null);
Review Comment:
This is risky. If 2 calls to this API happens at the same time, the just
created table can be deleted here. We should probably move the `try` after the
table config is posted
##########
pinot-controller/src/main/java/org/apache/pinot/controller/helix/core/PinotHelixResourceManager.java:
##########
@@ -1787,16 +1787,17 @@ public void updateUserConfig(UserConfig userConfig)
public void updateTableConfig(TableConfig tableConfig)
throws IOException {
validateTableTenantConfig(tableConfig);
- setExistingTableConfig(tableConfig);
+ setExistingTableConfig(tableConfig, -1);
}
/**
- * Sets the given table config into zookeeper
+ * Sets the given table config into zookeeper with the expected version,
which is the previous tableConfig znRecord
+ * version. If the expected version is -1, the version check is ignored.
*/
- public void setExistingTableConfig(TableConfig tableConfig)
+ public void setExistingTableConfig(TableConfig tableConfig, int
expectedVersion)
Review Comment:
Let's keep the existing one and add one with `expectedVersion`, no need to
change the existing usages
--
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]
