----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/66144/#review200256 -----------------------------------------------------------
src/slave/slave.hpp Lines 1147-1155 (patched) <https://reviews.apache.org/r/66144/#comment280904> Before you mention the lifecycle, could you explain the purpose of these sequences? i.e., ``` // Sequences in this map are used to enforce the order of tasks launched on each executor. // // Note on the lifecycle of the sequence: ... ``` src/slave/slave.cpp Lines 2227-2230 (patched) <https://reviews.apache.org/r/66144/#comment280905> Is it true that "we do not want the discard event to propagate to other tasks"? I might recommend the following: "We use this sequence only to maintain the task launching order. If the sequence is deleted, we do not want the resulting discard event to propagate up the chain, which would prevent the previous `.then()` or `.repair()` callbacks from being invoked. Thus, we use `undiscardable` to protect each `taskLaunch`." src/slave/slave.cpp Lines 2233-2258 (patched) <https://reviews.apache.org/r/66144/#comment280909> I would recommend moving this comment block above the `.onAny( ... )` call, since it is explaining the reason for the `.onAny` call and the conditions on `seqFuture`. Then, it would be good to add a comment above `taskLaunch.onReady( ... )` explaining why we are chaining onto `taskLaunch` here, rather than dispatching directly. Maybe something like: ``` // We only want to execute the following callbacks once the work performed in the `taskLaunch` chain is complete. Thus, we add them onto the `taskLaunch` chain rather than dispatching directly. taskLaunch .onReady( ... ``` src/slave/slave.cpp Lines 2242 (patched) <https://reviews.apache.org/r/66144/#comment280906> s/task's/tasks'/ src/slave/slave.cpp Lines 2251 (patched) <https://reviews.apache.org/r/66144/#comment280907> s/tasks'/task's/ src/slave/slave.cpp Line 2234 (original), 2279 (patched) <https://reviews.apache.org/r/66144/#comment280910> s/expects new/expects a new/ s/task launch/task/ src/slave/slave.cpp Line 2236 (original), 2281 (patched) <https://reviews.apache.org/r/66144/#comment280911> Use backticks instead of single quotes around `ExitedExecutorMessage`. src/slave/slave.cpp Lines 2286 (patched) <https://reviews.apache.org/r/66144/#comment280903> If `framework_ == nullptr` is true, then dereferencing the pointer here will be bad. You could enclose this in a conditional check, `if (framework_ != nullptr)`. - Greg Mann On March 29, 2018, 6:15 p.m., Meng Zhu wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/66144/ > ----------------------------------------------------------- > > (Updated March 29, 2018, 6:15 p.m.) > > > Review request for mesos, Chun-Hung Hsiao and Greg Mann. > > > Bugs: MESOS-8617 and MESOS-8624 > https://issues.apache.org/jira/browse/MESOS-8617 > https://issues.apache.org/jira/browse/MESOS-8624 > > > Repository: mesos > > > Description > ------- > > Up until now, Mesos does not guarantee in-order > task launch on the agent. There are two asynchronous > steps (unschedule GC and task authorization) in the > agent task launch path. Depending on the CPU scheduling > order, a later task launch may finish these two steps earlier > than its predecessors and get to the launch executor stage > earlier, resulting in out-of-order task delivery. > > This patch enforces the task delivery order by sequencing > task launch after the two asynchronous steps, specifically > right before entering `__run()`. > > > Diffs > ----- > > src/slave/slave.hpp 37f0361251524e63d02d251e8a03901812b8affb > src/slave/slave.cpp a4bd4ccd3fc59c3c0e462d9b480f5424b3e52d7a > > > Diff: https://reviews.apache.org/r/66144/diff/6/ > > > Testing > ------- > > make check > > > Thanks, > > Meng Zhu > >
