mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Jie Yu <yujie....@gmail.com>
Subject Re: Review Request 64561: Removed resource categories in UpdateSlaveMessage.
Date Wed, 13 Dec 2017 16:34:55 GMT


> On Dec. 13, 2017, 12:20 p.m., Benjamin Bannier wrote:
> > src/slave/slave.cpp
> > Line 6992 (original)
> > <https://reviews.apache.org/r/64561/diff/1/?file=1914886#file1914886line6998>
> >
> >     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.

I changed that because merging `update_oversubscribed_resources` field with one set to true
and one set to false is weird. There is actually a bug previously that in generateResourceProviderMessage,
we don't set `update_oversubscribed_resources`, with leads to an update to the oversubscribed
resources when people are using resource provider.

I'll stick to the existing code. I'll think about a cleanup to generate those messages without
looking into member fields (instead, making them a pure helper).


- Jie


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


On Dec. 13, 2017, 12: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, 12: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