yahoNanJing commented on code in PR #728: URL: https://github.com/apache/arrow-ballista/pull/728#discussion_r1151056031
########## ballista/scheduler/src/state/executor_manager.rs: ########## Review Comment: Agree. How about reusing `InMemoryClusterState` for the `KeyValueState`? There are two main reasons for removing the cache part in `ExecutorManager`: - It seems redundant for `InMemoryClusterState` - It's easily to miss some part for ensuring the correctness of the cached things. Previously I thought it may be unnecessary to cache the cluster state when using state storage. If it's necessary, I thing it's better to introduce the `InMemoryClusterState` for the `KeyValueState` directly. Then for the cache part, we can focus on `InMemoryClusterState`. To achieve this, I will raise a few followup commits to this PR. -- 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]
