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: [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]
