JacksonYao287 commented on PR #3384: URL: https://github.com/apache/ozone/pull/3384#issuecomment-1124510693
@sodonnel thanks for the comment. > We would also avoid excessive changes to the LegacyManager for now until we have a better idea about how the new ReplicationManager looks. since Legacy Replication Manager has no scheduling logic now, the only work it does is to check the container health, so it should be changed to defaultContainerHealthChecker later. since we have EC now , we should support moving EC container for container balancer , so move scheduler should be extracted out from Legacy replication manager to a standalone class to handle move option for all kinds of container. so IMHO, the new ReplicationManager should include the following part: 1 a main class - RepliactionManager.java, which will only handle scheduling related issues. 2 move scheduler, which will handle move options for container balancer. we already have a move scheduler interface and an implementation for ratis contaier now. we could also add a ECMoveScheduler if necessary. 3 health checker. we can add a containerHealthChecker interface , which has a function like ``` ContainerHealthWithCommands checkContainerState(ContainerInfo containerInfo, Set<ContainerReplica> replicas); ``` and then we can change Legacy Replication Manager to an implementation of that interface to check ratis and standalone container, and then add a ECContainerHealtherChecker for ec container. we could add more healthChecker implementation for containers of new types in the future if necessary. 4 since inflightActions are shared among different moveSchedulers and containerHealthCheckers, and some external class will also access and get the infomation of inflightActions, then i think it is better to have a inflightActionManager to manager all the inflightActions. 5 other functions, like sending command which is got from `ContainerHealthWithCommands`, could be wrapped into a standalone function or class, and can be called in Replication Manager after calling `checkContainerState` to send the command included in `ContainerHealthWithCommands` by the way, since refactor will bring a lot of changes , we can consider Replication Manager as a whole and use the current unit test and integration test to verify its correctness, and keep the logic which it exposed to external the same as before. after we finish refactoring , tests for different class can be added one by one what do you think? or do you have other better ideas about this? we can also discuss this in slack -- 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]
