mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Greg Mann <g...@mesosphere.io>
Subject Re: Review Request 66144: Enforced task launch order on the agent.
Date Sat, 31 Mar 2018 00:23:55 GMT

-----------------------------------------------------------
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
> 
>


Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message