> On 八月 23, 2016, 8:45 a.m., Guangya Liu wrote: > > src/master/master.cpp, line 3362 > > <https://reviews.apache.org/r/51317/diff/1/?file=1481628#file1481628line3362> > > > > The current logic is that the task with duplicate ID will be ignored > > and will not be launched, so which task will trigger and where do we send > > out the TASK_ERROR message? > > > > I saw that the TASK_ERROR message will only be sent out if the task is > > killed in `pendingTasks` and invalid. > > Benjamin Mahler wrote: > Hm.. I'm not sure I follow your comment. Maybe this will clarify things: > > There are two reasons a task will not be contained in `pendingTasks` when > we arrive in `_accept()`: > > 1. The task was killed while in `pendingTasks`. > 2. The task had a duplicate TaskID with another task in `pendingTasks`. > > So if a task is missing in `pendingTasks` we still have to validate it to > catch (2). This means we could have sent TASK_ERROR after TASK_KILLED if the > task was killed while in `pendingTasks` but is invalid or unauthorized. > > There are a few ways to fix this, but they're all a bit clunky so I > decided on just documenting this a bit better for now.
Yes, I think that we are on the same page, and the only comment is shall we update the comment a bit for `(and TASK_ERROR will be sent).`, probably update this as `(and TASK_ERROR will be sent after TASK_KILLED if the task was killed while in pendingTasks but is invalid or unauthorized).` ? > On 八月 23, 2016, 8:45 a.m., Guangya Liu wrote: > > src/master/master.cpp, line 3366 > > <https://reviews.apache.org/r/51317/diff/1/?file=1481628#file1481628line3366> > > > > I saw that this only occur if the task authorization failed for now, > > shall we highlight this in the comments here? > > Benjamin Mahler wrote: > Hm.. it can occur if authorization fails or if validation fails, why did > you mention only authorization failure? > > We could update the code to avoid sending TASK_ERROR REASON_UNAUTHORIZED > when the task is not in the `pendingTasks` map, since we know that it is > either killed or invalid, but this seemed not worth it since it only half-way > fixes the issue. Yes, but in `_accept`, we only send out `TASK_ERROR` when authorization failed but did not handle the case if `validation failed`, what about put `validation failed` in `TODO`? > On 八月 23, 2016, 8:45 a.m., Guangya Liu wrote: > > src/master/master.cpp, lines 3765-3767 > > <https://reviews.apache.org/r/51317/diff/1/?file=1481628#file1481628line3765> > > > > What about put more detail around line 3807 for `invalid` task case by > > adding a `TODO`? > > > > ``` > > // TODO(bmahler): Validate the task. We may send TASK_ERROR > > // after a TASK_KILLED if a task was killed (removed from > > // `pendingTasks`) *and* the task is invalid here. > > ``` > > Benjamin Mahler wrote: > Hm.. this looks like a duplicate of the TODO here? Are you suggesting > moving it down? Since it covers both authorization and validation I opted to > keep it up higher. I meant split the comments to two: one for `authorization failed` here and the other for `validation failed` in #3807, but I do not have strong intention on this as long as we can comment this clearly. - Guangya ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/51317/#review146492 ----------------------------------------------------------- On 八月 23, 2016, 5:11 a.m., Benjamin Mahler wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/51317/ > ----------------------------------------------------------- > > (Updated 八月 23, 2016, 5:11 a.m.) > > > Review request for mesos and Vinod Kone. > > > Repository: mesos > > > Description > ------- > > The use of `pendingTasks` cannot distinguish between a duplicate > TaskID and a task that has been killed while pending. This means > that if an invalid or unauthorized task is killed while pending, > TASK_KILLED is sent, and once in Master::_accept, we will also > send TASK_ERROR. > > > Diffs > ----- > > src/master/master.cpp d94a8510c4cee9c010706f79caf27ef4a10b41a8 > > Diff: https://reviews.apache.org/r/51317/diff/ > > > Testing > ------- > > N/A > > > Thanks, > > Benjamin Mahler > >
