You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@pinot.apache.org by GitBox <gi...@apache.org> on 2022/06/23 22:59:23 UTC
[GitHub] [pinot] Jackie-Jiang commented on a diff in pull request #8959: Update minion task metadata ZNode path
Jackie-Jiang commented on code in PR #8959:
URL: https://github.com/apache/pinot/pull/8959#discussion_r905567978
##########
pinot-common/src/main/java/org/apache/pinot/common/minion/MinionTaskMetadataUtils.java:
##########
@@ -36,12 +36,25 @@ private MinionTaskMetadataUtils() {
}
/**
- * Fetches the ZNRecord for the given minion task and tableName, from MINION_TASK_METADATA/taskName/tableNameWthType
+ * Fetches the ZNRecord for the given minion task and tableName. Fetch from the old path
Review Comment:
I think we should try the new path first, then fall back to the old one if the new one does not exist?
##########
pinot-common/src/main/java/org/apache/pinot/common/metadata/ZKMetadataProvider.java:
##########
@@ -130,7 +130,13 @@ public static String constructPropertyStorePathForSegmentLineage(String tableNam
return StringUtil.join("/", PROPERTYSTORE_SEGMENT_LINEAGE, tableNameWithType);
}
- public static String constructPropertyStorePathForMinionTaskMetadata(String taskType, String tableNameWithType) {
+ public static String constructPropertyStorePathForMinionTaskMetadataNewFormat(
Review Comment:
Please check the code style setting. Seems it is not using the Pinot Style
##########
pinot-common/src/main/java/org/apache/pinot/common/metadata/ZKMetadataProvider.java:
##########
@@ -130,7 +130,13 @@ public static String constructPropertyStorePathForSegmentLineage(String tableNam
return StringUtil.join("/", PROPERTYSTORE_SEGMENT_LINEAGE, tableNameWithType);
}
- public static String constructPropertyStorePathForMinionTaskMetadata(String taskType, String tableNameWithType) {
+ public static String constructPropertyStorePathForMinionTaskMetadataNewFormat(
Review Comment:
Suggest making this `constructPropertyStorePathForMinionTaskMetadata` and changing the current one to `constructPropertyStorePathForMinionTaskMetadataDeprecated` and annotate it as deprecated
##########
pinot-common/src/main/java/org/apache/pinot/common/minion/MinionTaskMetadataUtils.java:
##########
@@ -51,27 +64,46 @@ public static ZNRecord fetchTaskMetadata(HelixPropertyStore<ZNRecord> propertySt
}
/**
- * Deletes the ZNRecord for the given minion task and tableName, from MINION_TASK_METADATA/taskName/tableNameWthType
+ * Deletes the ZNRecord for the given minion task and tableName, from both the new path
+ * MINION_TASK_METADATA/${tableNameWthType}/${taskType} and the old path
+ * MINION_TASK_METADATA/${taskType}/${tableNameWthType}.
*/
public static void deleteTaskMetadata(HelixPropertyStore<ZNRecord> propertyStore, String taskType,
String tableNameWithType) {
- String path = ZKMetadataProvider.constructPropertyStorePathForMinionTaskMetadata(taskType, tableNameWithType);
- if (!propertyStore.remove(path, AccessOption.PERSISTENT)) {
+ String newPath = ZKMetadataProvider.constructPropertyStorePathForMinionTaskMetadataNewFormat(
+ taskType, tableNameWithType);
+ String oldPath = ZKMetadataProvider.constructPropertyStorePathForMinionTaskMetadata(
+ taskType, tableNameWithType);
+ if (!propertyStore.remove(newPath, AccessOption.PERSISTENT)
Review Comment:
We want to remove from both if both of them exist. Current implementation will short-circuit the second part
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org