mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Gastón Kleiman <gas...@mesosphere.io>
Subject Re: Review Request 70412: Made the operation feedback example framwork cleanup old reservations.
Date Tue, 09 Apr 2019 23:39:11 GMT


> On April 8, 2019, 10:26 a.m., Benno Evers wrote:
> > src/examples/operation_feedback_framework.cpp
> > Lines 329 (patched)
> > <https://reviews.apache.org/r/70412/diff/1/?file=2137761#file2137761line333>
> >
> >     Is this always true even in the presence of `RESERVATION_REFINEMENT`?
> 
> Gastón Kleiman wrote:
>     This framework only pushes one reservation, so I guess that there should only be
one element for reservations made by it?
>     
>     The check would fail if the example framework shares a role with another framework
that is using reservation refinement. Is that a case that we want to contemplate for a simple
example framework?
>     
>     If so, the framework should probably check the labels of all the reservations.
> 
> Greg Mann wrote:
>     Yea we would only hit a failure at this CHECK if another framework refined the reservations
made by this framework. I'm fine with making the assumption that that will not occur, what
do you guys think?
> 
> Benno Evers wrote:
>     Hm, the original question was more along the lines of "Does the `Resources::reserved()`
guarantee to always return resources with a single reservation even when using reservation
refinement?".
>     
>     If it does not, then I'm not sure we should be `CHECK()`ing here: A check failure
should usually indicate some bug in the local code that led to an inconsistent state where
it is impossible to recover. However, here it looks like we're `CHECK()`ing on external input,
and it does not even have to be malformed to trigger a check failure.
>     
>     I'm fine with making the assumption that this does not happen, but the problem is
how to communicate that assumption. I fear that if we distill the discussion here into a comment
and put it above the check it might be too distracting, especially since the target audience
are anyways people who might not be too familiar with the intricacies of Mesos.
>     
>     Maybe one solution would be to remove the `RESERVATION_REFINEMENT` capability below
and then also remove the `CHECK()` here completely, leaving the framework to crash with a
protobuf error if our assumption was wrong.

I tried removing the `RESERVATION_REFINEMENT` capability, but then the `v1::Resources()` constructor
crashes because it expects the resources proto to be in the post-refinement format. I could
upgrade the resources, but that also adds unnecessary complexity.

I ended up just removing the `CHECK()`. Note however that the framework has some other `CHECK()`
statements on external input, e.g., https://github.com/apache/mesos/blob/475b5e14085eef88edd868d331bd08b7ba5443bf/src/examples/operation_feedback_framework.cpp#L665
— we might want to get rid of those statements too.


- Gastón


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


On April 5, 2019, 4:35 p.m., Gastón Kleiman wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/70412/
> -----------------------------------------------------------
> 
> (Updated April 5, 2019, 4:35 p.m.)
> 
> 
> Review request for mesos, Benno Evers and Greg Mann.
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> This patch adds a flag to the operation feedback example framework that
> makes it unreserve resources that were reserved by another instance of
> the framework.
> 
> NOTE: The new flag should only be enabled if the example framework is
> the only framework making reservations for a given role. Otherwise it
> will unreserve reservations made by another framework.
> 
> 
> Diffs
> -----
> 
>   src/examples/operation_feedback_framework.cpp 2480c340c4ccb4246098c35e0315093f3eb44e81

> 
> 
> Diff: https://reviews.apache.org/r/70412/diff/1/
> 
> 
> Testing
> -------
> 
> Manual testing.
> 
> 
> Thanks,
> 
> Gastón Kleiman
> 
>


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