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 64561: Removed resource categories in UpdateSlaveMessage.
Date Wed, 13 Dec 2017 12:20:54 GMT

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


Fix it, then Ship it!




This implements the same functionality as https://reviews.apache.org/r/64445/. Let's just
update this one and drop the other.


src/master/master.hpp
Line 502 (original), 502 (patched)
<https://reviews.apache.org/r/64561/#comment272277>

    Let's not do this in this patch as it is an unrelated change. Instead we should clean
up all handler at once.



src/master/master.cpp
Line 7221 (original), 7221 (patched)
<https://reviews.apache.org/r/64561/#comment272278>

    Ditto.



src/master/master.cpp
Line 7249 (original), 7249 (patched)
<https://reviews.apache.org/r/64561/#comment272279>

    If the agent did not specify *the* 'update_overs...



src/messages/messages.proto
Line 734 (original), 734 (patched)
<https://reviews.apache.org/r/64561/#comment272280>

    It looks like we introduced resource categories in `a88469bfe8` which is currently still
only contained in `master` and was not part of a release.
    
    We can recycle field number 5.



src/slave/slave.hpp
Line 561 (original), 561 (patched)
<https://reviews.apache.org/r/64561/#comment272267>

    Not yours, but could you `s/resoruce/resource/`?



src/slave/slave.cpp
Line 6992 (original)
<https://reviews.apache.org/r/64561/#comment272281>

    One motivation for such helper functions can be that it becomes clearer than `UpdateSlaveMessage`
should not be constructed manually due to possible complexity. By removing one of them this
is not as apparent anymore.
    
    I'd suggest to keep this helper and revert changes at the callers so an up-to-date `oversubscribedResources`
is found.


- Benjamin Bannier


On Dec. 13, 2017, 1:56 a.m., Jie Yu wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/64561/
> -----------------------------------------------------------
> 
> (Updated Dec. 13, 2017, 1:56 a.m.)
> 
> 
> Review request for mesos, Benjamin Bannier and Jan Schlicht.
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> Given that now we use `UpdateSlaveMessage` to send resource provider
> information directly, having resource categories in the message is
> unnecessary and misleading.
> 
> Instead, this patch introduced a single optional boolean to indicate if
> oversubscribed resources need to be updated or not.
> 
> 
> Diffs
> -----
> 
>   src/master/master.hpp 232cc3758f240db626c4fdaf852163fa48af4dd7 
>   src/master/master.cpp efe8b8f1704b314e6e6a4d5632718cab2854e38f 
>   src/messages/messages.proto e680cd5e4d5a93c3c77309f327844f55fbb239a1 
>   src/slave/slave.hpp 7c40fc71b49057fea0cfd85290931fbd0f6a9d62 
>   src/slave/slave.cpp 5869e73ca1c14c99e580da9d7375181da2073ec5 
>   src/tests/oversubscription_tests.cpp 3f57ce105e24e9f9cd681d8d984dbe242aa51f75 
> 
> 
> Diff: https://reviews.apache.org/r/64561/diff/1/
> 
> 
> Testing
> -------
> 
> make check
> 
> 
> Thanks,
> 
> Jie Yu
> 
>


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