mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Anindya Sinha <anindya_si...@apple.com>
Subject Re: Review Request 57817: Suppress offers for frameworks on registration.
Date Sat, 15 Apr 2017 06:40:44 GMT


> On April 13, 2017, 12:21 a.m., Vinod Kone wrote:
> > src/master/allocator/mesos/allocator.hpp
> > Line 70 (original), 70-71 (patched)
> > <https://reviews.apache.org/r/57817/diff/3/?file=1684648#file1684648line70>
> >
> >     This is making me think whether we want framework to subscribe with "inactive"
field instead "suppress".
> >     
> >     The reason being, "suppressOffers" was originally added instead of "deactivateFramework"
because the former was intended to have some filters. One such filter has been recently added:
you can suppress offers for a specific role instead of all roles. Do we want to give the same
flexibility for suppression during subscription? 
> >     
> >     For example, here it's not clear to me what is the difference between "active"
and "offersSuppressed" booleans as far as the allocator is concerned.
> >     
> >     Thoughts?
> 
> Anindya Sinha wrote:
>     I agree that the 2 APIs are very similar and I agree we can also achieve the same
by an `inactive` (instead of `suppress_offers`) field of `SUBSCRIBE` override the definition
of an active framework which is currently just based on framework state. However, it seemed
cleaner to me to *not* update the definition of an active framework based on a field in the
`SUBSCRIBE` call. Basically, `framework->state` is *modifiable* characteristic of a framework,
whereas `suppress_offers` is an initial setting to register that framework.
>     
>     I think functionally it would achieve the same so I am fine with using an `inactive`
field in `SUBSCRIBE` redefine the definition of an active framework.
>     
>     Let me know what you think and I can adapt accordingly.
> 
> Vinod Kone wrote:
>     Thinking a bit more, "inactive" field in the SUBSCRIBE message might not be intuitive
for framework authors to figure out how to make it "active". We don't have a REACTIVATE call.
So maybe "supress_offers" is more intuitive because they would intuitively know to call REVIVE
to un-suppress.
>     
>     Given that, how about "Suppress suppress" field inside "Subcribe" instead of "boolean
suppress_offers" to be more future proof? Ideally, we would have a way to atomically send
"SUBSCRIBE" and "SUPPRESS" calls together in one calls but not sure how to easily achieve
that.

That sounds good. Updated `SUBSCRIBE` to contain `Suppress suppress` (for being future proof),
and made changes accordingly in all the reviews in this chain.


- Anindya


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


On April 11, 2017, 11:10 p.m., Anindya Sinha wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/57817/
> -----------------------------------------------------------
> 
> (Updated April 11, 2017, 11:10 p.m.)
> 
> 
> Review request for mesos, James Peach, Vinod Kone, and Jiang Yan Xu.
> 
> 
> Bugs: MESOS-7015
>     https://issues.apache.org/jira/browse/MESOS-7015
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> If requested in SUBSCRIBE api call, offers are suppressed on
> framework registration.
> 
> 
> Diffs
> -----
> 
>   include/mesos/allocator/allocator.hpp 6eda1b8619269c1501a935045b18b1deaf845b33 
>   src/master/allocator/mesos/allocator.hpp 57b54b86c43c7731e64d422d285c4b8ca7e27a60 
>   src/master/allocator/mesos/hierarchical.hpp f84b0574ce9a392c9528c87b04b01dbb2053cff7

>   src/master/allocator/mesos/hierarchical.cpp 051f749dd5921a322ca930a042c31814616d38f9

>   src/master/master.hpp d537933d0b467a6f9996951c601b31338bb9d034 
>   src/master/master.cpp 0f4c64c6b102ef201779a331c96b5d78a98281ad 
>   src/tests/allocator.hpp 6b71c574e0e4facd1795ef50ee0869c03b87833d 
>   src/tests/hierarchical_allocator_tests.cpp 33e7b455f8664858eb4f03727b076a10c80cd6e0

>   src/tests/master_allocator_tests.cpp 119e318f8a01d50e8dae5c30cf5fa6a017c3c625 
>   src/tests/persistent_volume_endpoints_tests.cpp 1237d081d781948975f66a8240ae9bdb5e819a3b

>   src/tests/reservation_tests.cpp 4504831d77c1bfcf5f2ddf6d28cd45dea2c421ad 
>   src/tests/resource_offers_tests.cpp f0bca1d9e03013ce35215b0ffa6b50b38972dc0c 
>   src/tests/slave_recovery_tests.cpp 53f33a2b0411c8158326074ce043c7b1dbeef5b4 
> 
> 
> Diff: https://reviews.apache.org/r/57817/diff/4/
> 
> 
> Testing
> -------
> 
> All tests passed.
> 
> 
> Thanks,
> 
> Anindya Sinha
> 
>


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