mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Jiang Yan Xu <...@jxu.me>
Subject Re: Review Request 61473: Do not kill non partition aware tasks.
Date Thu, 10 Aug 2017 21:00:16 GMT

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/61473/#review182605
-----------------------------------------------------------



Some of the comments below were made before I started to feel that we are probably doing too
many conversions to justify storing these tasks in TASK_UNREACHABLE. Perhaps we can just store
them in `Framework.unreachableTasks` but in TASK_LOST state?

It's possible that we can add another map `BoundedHashMap<TaskID, process::Owned<Task>>
unreachableNonPartitionAwareTasks;` for these tasks but it's clunky in the sense that you
have to clarify that `unreachableTasks` is only for partition aware tasks but in fact all
of these tasks belong to the same framework which is either parition aware or not, however
with the possibility of changing capability... so it's probably easier to describe things
if we just put all of them in `unreachableTasks` and simply say that "if the framework is
not partition-aware, the tasks stored in `unreachableTasks` may be in `TASK_LOST`".

If we do that, then some of the comments below don't apply any more but I am keep them just
for posterity (some styling issues etc).


src/master/http.cpp
Lines 342 (patched)
<https://reviews.apache.org/r/61473/#comment258567>

    One empty line above.



src/master/http.cpp
Lines 3203-3206 (original), 3215-3227 (patched)
<https://reviews.apache.org/r/61473/#comment258574>

    Should we only loop through the once? And in order to try not to duplicate similar lines,
consider the following?
    
    ```
    foreachvalue (const Owned<Task>& _task, framework->unreachableTasks) {
      Owned<Task> task = _task;
    
      if (framework->capabilities.partitionAware) {
        task = ...
      }
      
      frameworkTaskSummaries[frameworkId].count(*task.get());
      slaveTaskSummaries[task->slave_id()].count(*task.get());
    }
    ```



src/master/http.cpp
Lines 4017-4027 (original), 4038-4063 (patched)
<https://reviews.apache.org/r/61473/#comment258575>

    Similar to above, don't duplicate lines.



src/master/http.cpp
Lines 4167-4175 (original), 4203-4221 (patched)
<https://reviews.apache.org/r/61473/#comment258576>

    Ditto.



src/master/master.hpp
Lines 2403-2408 (patched)
<https://reviews.apache.org/r/61473/#comment258578>

    Can we hold off creating a helper for this? IMO this helper is doing too little and not
so much of an "abstraction", i.e., what the method does it not perfectly captured by the name
of the mehod. Inlining 1) making a copy & 2) setting the state is not too much redudancy
than calling this method.



src/master/master.hpp
Lines 2769-2771 (original), 2773-2775 (patched)
<https://reviews.apache.org/r/61473/#comment258600>

    Fix the comment.



src/master/master.cpp
Lines 6398-6414 (original), 6389-6405 (patched)
<https://reviews.apache.org/r/61473/#comment258580>

    Fix the comments.



src/master/master.cpp
Lines 6422 (patched)
<https://reviews.apache.org/r/61473/#comment258579>

    `erase` can handle the `!contains` case.



src/master/master.cpp
Lines 6477-6480 (original), 6464-6467 (patched)
<https://reviews.apache.org/r/61473/#comment258584>

    Fix the comment.



src/master/master.cpp
Line 7137 (original), 7112 (patched)
<https://reviews.apache.org/r/61473/#comment258586>

    If our code stops making such distinction, I don't think the comment should continue to
make such distinction.



src/master/master.cpp
Lines 7132-7137 (patched)
<https://reviews.apache.org/r/61473/#comment258590>

    Do this only when we actually send an update i.e., `framework->connected()`?



src/master/master.cpp
Lines 8928-8931 (original), 8907-8910 (patched)
<https://reviews.apache.org/r/61473/#comment258599>

    So for this update event we are going to send TASK_UNREACHABLE instead TASK_LOST, which
is unintended?


- Jiang Yan Xu


On Aug. 10, 2017, 9:07 a.m., Megha Sharma wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/61473/
> -----------------------------------------------------------
> 
> (Updated Aug. 10, 2017, 9:07 a.m.)
> 
> 
> Review request for mesos, Vinod Kone and Jiang Yan Xu.
> 
> 
> Bugs: MESOS-7215
>     https://issues.apache.org/jira/browse/MESOS-7215
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> Master will not kill the tasks for non-Partition aware frameworks
> when an unreachable agent re-registers with the master.
> Master used to send a ShutdownFrameworkMessages to the agent
> to kill the tasks from non partition aware frameworks including the
> ones that are still registered which was problematic because the offer
> from this agent could still go to the same framework which could then
> launch new tasks. The agent would then receive tasks of the same
> framework and ignore them because it thinks the framework is shutting
> down. The framework is not shutting down of course, so from the master
> and the scheduler’s perspective the task is pending in STAGING forever
> until the next agent reregistration, which could happen much later.
> This commit fixes the problem by not shutting down the non-partition
> aware frameworks on such an agent.
> 
> 
> Diffs
> -----
> 
>   src/master/http.cpp 959091c8ec03b6ac7bcb5d21b04d2f7d5aff7d54 
>   src/master/master.hpp b802fd153a10f6012cea381f153c28cc78cae995 
>   src/master/master.cpp 7f38a5e21884546d4b4c866ca5918db779af8f99 
>   src/tests/partition_tests.cpp 62a84f797201ccd18b71490949e3130d2b9c3668 
> 
> 
> Diff: https://reviews.apache.org/r/61473/diff/3/
> 
> 
> Testing
> -------
> 
> make check
> 
> 
> Thanks,
> 
> Megha Sharma
> 
>


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