LiebingYu commented on PR #1477: URL: https://github.com/apache/fluss/pull/1477#issuecomment-3268659598
> Sorry, I do have some concerns about this pull request. I left the comments below: > > 1. Why moving it to background can improve the ZK performance? Doesn't the background execution also require recursively deleting all the children for the partition assignment? Could you share the benchmark result before and after using the background? > 2. I think this changes the semantic of `deletePartitionAssignment` which means all the assignment has been deleted after the method returns. This may introduce some unexpected behavior when using this method. For example: `TableManager#completeDeletePartition` removes the partition from `coordinatorContext` only when the assignement has been deleted in zk. But this method becomes async, and it will remove the partition from `coordinatorContext` before all the assignment is deleted from zk. > 3. We have a lot of `deletingChildrenIfNeeded` in `ZooKeeperClient`, should we also update other methods as well? Otherwise, it introduces in-consistent in the code. This makes future development confusing that which way is suggested to use. 1. Use backgroud cannot improve the ZK performance. But it avoid to block `processDeleteReplicaResponseReceived()`, which may take several seconds to complete. For KV tables with many snapshot nodes, the deletion event takes longer. 2. It does introduce a certain degree of inconsistency, but eventual consistency should be guaranteed. I'm not sure whether this eventual consistency could cause any issues. 3. In `deleteTableAssignment()` and `deletePartitionAssignment()` we better use background mode. Because the parent node may have a lot of children. However, deleting a table is not a common operation, so we might be able to ignore it for now. -- 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]
