mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Ben Mahler <benjamin.mah...@gmail.com>
Subject Re: Review Request 43489: KillTask introduces TASK_KILLING state.
Date Sat, 20 Feb 2016 16:30:45 GMT

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



It would be helpful to reference in the testing done section that you've also added tests
in subsequent patches, so that it's clear to the reviewer without having to look through the
entire chain. I left some comments but I'll make the adjustments since they're minor.


src/docker/executor.cpp (lines 209 - 212)
<https://reviews.apache.org/r/43489/#comment181422>

    Ditto here, I can't see why this is only done inside killTask, looks like a bug!



src/launcher/executor.cpp (line 117)
<https://reviews.apache.org/r/43489/#comment181418>

    To be consistent, let's set frameworkInfo right below where we set driver.



src/launcher/executor.cpp (lines 460 - 469)
<https://reviews.apache.org/r/43489/#comment181420>

    Let's do this inside shutdown per my comment at the top.
    
    It's also not clear to me why the health check process is only killed inside killTask,
seems that we'll leak the health check process if we only are sent shutdown! Looks like a
bug :(



src/launcher/executor.cpp (lines 460 - 461)
<https://reviews.apache.org/r/43489/#comment181423>

    Let's avoid copying every capability we loop over (take a const & instead), since
Capability may store additional data in the future and we don't need to mutate a copy here.



src/launcher/executor.cpp (line 461)
<https://reviews.apache.org/r/43489/#comment181419>

    Please add a CHECK_SOME to ensure that frameworkInfo is Some before you de-reference.
    
    Also, you can use the -> operator to clean this up:
    
    ```
        foreach (FrameworkInfo::Capability capability,
                 frameworkInfo->capabilities()) {
    ```
    
    A space is needed before the brace on the foreach: s/){/) {/



src/launcher/executor.cpp (line 464)
<https://reviews.apache.org/r/43489/#comment181421>

    Please use CopyFrom for copying protobufs, unless you need the merge functionality (you
don't need merging functionality here).


- Ben Mahler


On Feb. 16, 2016, 10:23 a.m., Abhishek Dasgupta wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/43489/
> -----------------------------------------------------------
> 
> (Updated Feb. 16, 2016, 10:23 a.m.)
> 
> 
> Review request for mesos, Ben Mahler and Qian Zhang.
> 
> 
> Bugs: MESOS-4547
>     https://issues.apache.org/jira/browse/MESOS-4547
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> KillTask introduces TASK_KILLING state.
> 
> 
> Diffs
> -----
> 
>   src/docker/executor.cpp 654a41db843a85d953880d5145bc95ada9ed2920 
>   src/launcher/executor.cpp c27e0792e4b9bcec0829a46be4232c013d965cf9 
> 
> Diff: https://reviews.apache.org/r/43489/diff/
> 
> 
> Testing
> -------
> 
> make check
> 
> 
> Thanks,
> 
> Abhishek Dasgupta
> 
>


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