I have no strong preference either way. On Mon, Sep 26, 2016 at 11:25 AM, Huang Kai <[email protected]> wrote:
> 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 > >
