Hi folks, I'm currently blocked on the review https://reviews.apache.org/r/51876. I was wondering if you guys can provide some insights into the two proposed approaches on RB and help me proceed.
The problem is that the aurora executor needs to determine if it should send a TASK_RUNNING message based on whether health check is enabled for an assigned task. Initially, I created an is_health_check_enabled(assigned_task) function in task_info.py, and use it in aurora executor. See: https://reviews.apache.org/r/51876/diff/3/. However, Maxim raised a valid point that is_health_check_enabled has some duplication with the set up of health checker in later step. Therefore we should reuse the logic of is_health_check_enabled as much as possible in health_checker. One solution is to create a dedicated function called is_health_check_enabled for an assigned_task, and reuse it when we set up a health checker. The benefit is better abstraction and ease for test. The challenge of implementing it is that this function seems a little bit heavy-weighted, we have to parse an assigned_task, compute the port map, and get health_checker, health_check_config from it as well. One solution I can come up with is to store all the computation result(port_map, health_checker, health_check_config) in a utility class. So that it can be reuse later. But a downside here is that the is_health_check_enabled now serves multiple purposes, and the meaning of this function is not clear. It should only answer one question: is health check enabled on this task? A second solution is to check if health check is enabled for an assigned_task by checking the presence of a health checker instance. A benefit of doing this is that we can set up the necessary health checkers and check if health check is enabled in one pass. In this way, we used the logic as much as possible and eliminate the duplications. See https://reviews.apache.org/r/51876/diff/5/ Could you guys let me know your thought on the two approaches? If no one objects to the second solution, I will modify the executor as https://reviews.apache.org/r/51876/diff/5/ Best, Kai
