jackjlli commented on code in PR #9339:
URL: https://github.com/apache/pinot/pull/9339#discussion_r965223041
##########
pinot-common/src/test/java/org/apache/pinot/common/minion/MinionTaskMetadataUtilsTest.java:
##########
@@ -91,6 +91,17 @@ public void testDeleteTaskMetadata() {
MinionTaskMetadataUtils.deleteTaskMetadata(propertyStore, TASK_TYPE,
TABLE_NAME_WITH_TYPE);
assertFalse(propertyStore.exists(OLD_MINION_METADATA_PATH, ACCESS_OPTION));
assertFalse(propertyStore.exists(NEW_MINION_METADATA_PATH, ACCESS_OPTION));
+
+ // 1. ZNode MINION_TASK_METADATA/TestTable_OFFLINE and its descendants
will be removed
+ // 2. ZNode MINION_TASK_METADATA/<any task type>/TestTable_OFFLINE will
also be removed
+ propertyStore = new FakePropertyStore();
+ propertyStore.set(OLD_MINION_METADATA_PATH,
OLD_TASK_METADATA.toZNRecord(), EXPECTED_VERSION, ACCESS_OPTION);
+ propertyStore.set(NEW_MINION_METADATA_PATH,
NEW_TASK_METADATA.toZNRecord(), EXPECTED_VERSION, ACCESS_OPTION);
Review Comment:
Please add one more ZNode that the path is a table name that doesn't exist.
##########
pinot-common/src/main/java/org/apache/pinot/common/minion/MinionTaskMetadataUtils.java:
##########
@@ -79,10 +80,38 @@ public static void
deleteTaskMetadata(HelixPropertyStore<ZNRecord> propertyStore
}
}
+ /**
+ * Deletes the minion task metadata ZNRecord for the given tableName, from
both the new path
+ * MINION_TASK_METADATA/${tableNameWthType} and the old path
+ * MINION_TASK_METADATA/<any task type>/${tableNameWthType}
+ */
+ public static void deleteTaskMetadata(HelixPropertyStore<ZNRecord>
propertyStore, String tableNameWithType) {
+ // delete the minion task metadata ZNRecord
MINION_TASK_METADATA/${tableNameWthType}
+ String path =
ZKMetadataProvider.constructPropertyStorePathForMinionTaskMetadata(tableNameWithType);
+ if (!propertyStore.remove(path, AccessOption.PERSISTENT)) {
+ throw new ZkException("Failed to delete task metadata for table: " +
tableNameWithType);
+ }
+ // delete the minion task metadata ZNRecord MINION_TASK_METADATA/<any task
type>/${tableNameWthType}
+ List<String> childNames =
+
propertyStore.getChildNames(ZKMetadataProvider.getPropertyStorePathForMinionTaskMetadataPrefix(),
+ AccessOption.PERSISTENT);
+ if (childNames != null && !childNames.isEmpty()) {
+ for (String child : childNames) {
+ // Even though some child names are not task types (e.g., in the new
metadata path, the child name
+ // is a table name), it does not harm to try to delete the
non-existent constructed path.
+ String oldPath =
+
ZKMetadataProvider.constructPropertyStorePathForMinionTaskMetadataDeprecated(child,
tableNameWithType);
+ if (!propertyStore.remove(oldPath, AccessOption.PERSISTENT)) {
Review Comment:
If the path doesn't exist, `propertyStore.remove()` will return false. In
this case, an exception will be thrown and the for loop will stop, which
contradicts to the comment several lines above though.
##########
pinot-controller/src/main/java/org/apache/pinot/controller/helix/core/PinotHelixResourceManager.java:
##########
@@ -1922,13 +1920,8 @@ public void deleteRealtimeTable(String tableName,
@Nullable String retentionPeri
LOGGER.info("Deleting table {}: Removed segment lineage",
realtimeTableName);
// Remove task related metadata
- MinionTaskMetadataUtils.deleteTaskMetadata(_propertyStore,
MinionConstants.MergeRollupTask.TASK_TYPE,
- realtimeTableName);
- LOGGER.info("Deleting table {}: Removed merge rollup task metadata",
realtimeTableName);
-
- MinionTaskMetadataUtils.deleteTaskMetadata(_propertyStore,
MinionConstants.RealtimeToOfflineSegmentsTask.TASK_TYPE,
- realtimeTableName);
- LOGGER.info("Deleting table {}: Removed merge realtime to offline
metadata", realtimeTableName);
+ MinionTaskMetadataUtils.deleteTaskMetadata(_propertyStore,
realtimeTableName);
+ LOGGER.info("Deleting table {}: Removed all task metadata",
realtimeTableName);
Review Comment:
Same here.
##########
pinot-controller/src/main/java/org/apache/pinot/controller/helix/core/PinotHelixResourceManager.java:
##########
@@ -1859,9 +1858,8 @@ public void deleteOfflineTable(String tableName,
@Nullable String retentionPerio
LOGGER.info("Deleting table {}: Removed segment lineage",
offlineTableName);
// Remove task related metadata
- MinionTaskMetadataUtils.deleteTaskMetadata(_propertyStore,
MinionConstants.MergeRollupTask.TASK_TYPE,
- offlineTableName);
- LOGGER.info("Deleting table {}: Removed merge rollup task metadata",
offlineTableName);
+ MinionTaskMetadataUtils.deleteTaskMetadata(_propertyStore,
offlineTableName);
+ LOGGER.info("Deleting table {}: Removed all task metadata",
offlineTableName);
Review Comment:
nit: `sed 's/Removed all task metadata/Removed all minion task metadata/'`
--
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]
