delete all related minion task metadata when deleting a table #9339
Conversation
Codecov Report
@@ Coverage Diff @@
## master #9339 +/- ##
============================================
- Coverage 69.80% 61.29% -8.51%
+ Complexity 4777 4553 -224
============================================
Files 1875 1870 -5
Lines 99860 99821 -39
Branches 15194 15192 -2
============================================
- Hits 69706 61190 -8516
- Misses 25231 33991 +8760
+ Partials 4923 4640 -283
Flags with carried forward coverage won't be shown. Click here to find out more.
📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
...ntroller/src/main/java/org/apache/pinot/controller/helix/core/PinotHelixResourceManager.java
Outdated
Show resolved
Hide resolved
...ntroller/src/main/java/org/apache/pinot/controller/helix/core/PinotHelixResourceManager.java
Outdated
Show resolved
Hide resolved
|
@jackjlli thanks for reviewing the code. I made some more changes to clean up some redundant. Please help to review the code again when you get some time. Thanks! |
| // 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)) { |
There was a problem hiding this 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.
There was a problem hiding this comment.
Hmmm, I tested locally using integration test, if we are trying to remove a non-existent path, it returns true.
Could you please let me know your test setup so I can try it myself? Thanks!
There was a problem hiding this comment.
Oh nvm, saw the source code in helix-core jar. I feel the better approach would be to traverse all the existing task types instead of going over all the children nodes (there could be thousands of tables in a cluster), but for now I'm fine with this. You can add a TODO here.
There was a problem hiding this comment.
I have another version implemented locally 😄 , which scans jars and find all minion task types, similar to what https://github.com/apache/pinot/blob/master/pinot-controller/src/main/java/org/apache/pinot/controller/helix/core/minion/generator/TaskGeneratorRegistry.java does.
I chose the current one (which reads from ZK) because I want to use ZK as the source of truth, so we will not miss any ZNode.
But you point out a very good point that there might be thousands of tables. I can add a TODO and we can decide later.
There was a problem hiding this comment.
Let me know your thought, we can switch to the other approach or we can merge the current approach and get back to it later.
(BTW, could you please merge it if we want to go with the current approach as for now because I cannot merge the code 😄 )
There was a problem hiding this comment.
We can get back to it later. I'll merge this one. Thanks for making the change!
pinot-common/src/test/java/org/apache/pinot/common/minion/MinionTaskMetadataUtilsTest.java
Show resolved
Hide resolved
...ntroller/src/main/java/org/apache/pinot/controller/helix/core/PinotHelixResourceManager.java
Outdated
Show resolved
Hide resolved
...ntroller/src/main/java/org/apache/pinot/controller/helix/core/PinotHelixResourceManager.java
Outdated
Show resolved
Hide resolved
Delete the ZNode
(1) MINION_TASK_METADATA/${tableName} and its descendants
(2) MINION_TASK_METADATA//${tableName}
when deleting a table, so that
(1) we don't need to explicitly delete each MINION_TASK_METADATA/${tableName}/${taskType} and MINION_TASK_METADATA/${taskType}/${tableName} separately.
(2) the code can also delete customized minion task metadata automatically