mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Benjamin Bannier <benjamin.bann...@mesosphere.io>
Subject Re: Review Request 64010: Added additional parameters to Allocator::updateSlave().
Date Fri, 01 Dec 2017 12:24:45 GMT

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




include/mesos/allocator/allocator.hpp
Lines 211-212 (patched)
<https://reviews.apache.org/r/64010/#comment270628>

    This doesn't look what was in the original design where a framework could receive an update
for agent updates and remove offer filters itself (we already independently plan to extend
the scheduler interfaces to allow reviving a single agent for offer operation feedback).
    
    I feel having a master remove unexpired offer filters by itself violates responsibilities.
It would seem much more straight-forward to reason about lifecycles would the framework which
created the filter also be the sole on responsible for cleaning it up before expiry. The originally
designed agent update channel to the framework would permit this, and I hope this is what
we still plan to implement.
    
    This makes me wonder whether we should we mark this parameter as transitional if we cannot
get rid of it completely now. We are working on a public interface here so this is not ideal
either.



include/mesos/allocator/allocator.hpp
Line 215 (original), 219-220 (patched)
<https://reviews.apache.org/r/64010/#comment270629>

    I feel it might make sense to remove the default arguments here as they are error-prone
and could lead to surprising behavior, see http://www.gotw.ca/gotw/005.htm.


- Benjamin Bannier


On Nov. 29, 2017, 7:17 p.m., Benno Evers wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/64010/
> -----------------------------------------------------------
> 
> (Updated Nov. 29, 2017, 7:17 p.m.)
> 
> 
> Review request for mesos and Vinod Kone.
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> The current `SlaveInfo` of a slave is now passed as an additional
> parameter to `updateSlave()`, to account for the possibility of
> it changing during an agent restart.
> 
> While the existing HierarchicalDRFAllocator only cares about the domain
> and hostname fields, the full SlaveInfo is passed since custom
> allocators might base their scheduling decisions on other fields, for
> example attributes.
> 
> Additionally, this also provides callers with an option to reset
> existing offer filters for a given slave id when the update is
> applied.
> 
> 
> Diffs
> -----
> 
>   include/mesos/allocator/allocator.hpp ae122003487ca8956573e993cd3993aa8cc286f1 
>   src/master/allocator/mesos/allocator.hpp 8fa4fdeec4ec64bcd49fc442c230d8684a11cfd9 
>   src/master/allocator/mesos/hierarchical.hpp 2c4832b29842330fa57756cd3d4202f265a820f3

>   src/master/allocator/mesos/hierarchical.cpp 5ce9ceaa3a5f84a1e076d45448863c418531cc2b

>   src/tests/allocator.hpp 6a84f1beb86dceb5a5e9bf4615c13a216f3d0905 
>   src/tests/hierarchical_allocator_tests.cpp f0f95ba4f667bf8ea54e985d8cde913a4170d8ff

> 
> 
> Diff: https://reviews.apache.org/r/64010/diff/4/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Benno Evers
> 
>


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