mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Benjamin Mahler <bmah...@apache.org>
Subject Re: Review Request 52231: Fixed driver based schedulers to ACK updates from HTTP executors.
Date Sat, 24 Sep 2016 01:27:30 GMT

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


Fix it, then Ship it!





src/slave/slave.cpp (lines 3622 - 3628)
<https://reviews.apache.org/r/52231/#comment218176>

    How about checking that they're both set and they match the slave's id?
    
    ```
      if (!update.has_slave_id() || update.slave_id() != info.id()) {
        // log warning, increment metric, return  
      }
    
      if (!update.status().has_slave_id() || update.status().slave_id() != info.id()) {
        // log warning, increment metric, return
      }
    ```
    
    As it stands it seems this check will allow the wrong slave id or unset slave ids?



src/tests/command_executor_tests.cpp (lines 276 - 283)
<https://reviews.apache.org/r/52231/#comment218177>

    Is this part necessary?


- Benjamin Mahler


On Sept. 24, 2016, 1:07 a.m., Anand Mazumdar wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52231/
> -----------------------------------------------------------
> 
> (Updated Sept. 24, 2016, 1:07 a.m.)
> 
> 
> Review request for mesos and Benjamin Mahler.
> 
> 
> Bugs: MESOS-6245
>     https://issues.apache.org/jira/browse/MESOS-6245
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> Previously, driver based schedulers were not able to acknowledge
> status updates from HTTP based executors if they had explicit
> acknowledgements enabled. This was due to the fact that we
> were not populating `TaskStatus.slave_id` correctly if not
> set. It would also be great to validate `TaskStatus.slave_id`
> set by the executor and let them know if the value is incorrect.
> 
> 
> Diffs
> -----
> 
>   src/common/protobuf_utils.cpp 5db4be4bdc7b9a3a2a66a17f8a9ac74c8d3dfbf6 
>   src/slave/slave.cpp 11e9c8af87aa5153f72f2a20cc578fe3d729b153 
>   src/tests/command_executor_tests.cpp 07e5eb4d7c2ace2b6714fbe02f29d41663152556 
> 
> Diff: https://reviews.apache.org/r/52231/diff/
> 
> 
> Testing
> -------
> 
> make check
> 
> 
> Thanks,
> 
> Anand Mazumdar
> 
>


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