> On Sept. 8, 2016, 9:13 p.m., Joshua Cohen wrote:
> > src/main/python/apache/aurora/executor/aurora_executor.py, lines 204-211
> > <https://reviews.apache.org/r/51742/diff/1/?file=1494643#file1494643line204>
> >
> > I see a couple of problems with this method:
> >
> > 1. The `status_result` argument passed in by `StatusManager#run` is not
> > a `TaskState`, it's a
> > [StatusResult](https://github.com/apache/aurora/blob/master/src/main/python/apache/aurora/executor/common/status_checker.py#L23-L50)
> > which has a `status` property whose value is a `TaskState`. So I believe
> > your first check will always fail in a real world scenario?
> > 2. If we receive multiple invocations of this callback for healthy
> > tasks, the second callback will result in the healthy task being killed
> > (since `self.task_running` will be `True`, the initial condition will
> > evaluate to `False` and we'll land in the shutdown branch).
> >
> > These two issues combined make me wary of shipping these changes
> > without the corresponding changes further down the stack, as it makes it
> > hard to evaluate their actual effect. I appreciate trying to break the
> > review up into manageable chunks, but in this case I think it's having the
> > opposite effect of causing reviewers to have to envision the contents of
> > future reviews to understand the impact of the changes in this review. On
> > top of that, the doubling up of sending the `TASK_RUNNING` update while the
> > code is in the in-between state also makes me nervous.
> >
> > We also definitely need test coverage for this callback.
>
> Kai Huang wrote:
> Thanks for the valid feedback. I'll work on them.