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 65095: Made it possible to update an operation without mutating resources.
Date Fri, 12 Jan 2018 10:58:04 GMT


> On Jan. 11, 2018, 10 p.m., Greg Mann wrote:
> > src/master/master.hpp
> > Lines 881-885 (original), 881-888 (patched)
> > <https://reviews.apache.org/r/65095/diff/1/?file=1939029#file1939029line881>
> >
> >     I'm thinking that `convertResources` might be a more intuitive name for the
boolean parameter here? In all cases, resources associated with the operation are recovered.
In only some cases, we must first convert the consumed resources to converted resources so
that we can then recover them.
> >     
> >     So, the variable behavior seems to be whether or not we are converting resources
before we recover them. WDYT?
> >     
> >     
> >     I think the comment here could be tweaked a bit to better reflect this as well.
Perhaps:
> >     
> >     "Transitions the operation and, if the operation becomes terminal, recovers
the resources associate with the operation. If `convertResources` is false, then no conversion
from consumed to converted resources is applied in the allocator prior to resource recovery."

Thank you for the suggestion, I changed the parameter name to `convertResources`.

I also update the comment slightly,

    // Transitions the operation, and updates and recovers resources if
    // the operation becomes terminal. If `convertResources` is `false`
    // only the consumed resources of terminal operations are recovered,
    // but no resources are converted.
    
I'd prefer a tone along these lines, so we do not just repeat the implementation incompletely,
but instead reflect intention. The parameter `convertResources` e.g., does not just control
resources updates in the allocator, but also in the master's agent state.


- Benjamin


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


On Jan. 12, 2018, 11:53 a.m., Benjamin Bannier wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65095/
> -----------------------------------------------------------
> 
> (Updated Jan. 12, 2018, 11:53 a.m.)
> 
> 
> Review request for mesos, Gaston Kleiman, Greg Mann, and Jan Schlicht.
> 
> 
> Bugs: MESOS-8422
>     https://issues.apache.org/jira/browse/MESOS-8422
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> In certain situations it can make sense to update the state of an
> operation without also wanting to update resources. In this patch we
> modify the master's `updateOperation` function to take an additional
> parameter signifying whether resources should be updated, or whether
> we only care about updating the operation and tracking of used
> resources.
> 
> We will use this functionality in a subsequent patch to perform more
> contained updates to agent state when processing `UpdateSlaveMessages`
> which contain both resources and operations (and where any terminal
> operations were already applied to the agent's resources).
> 
> 
> Diffs
> -----
> 
>   src/master/master.hpp cdfd06ceb2a8eafa60d0af382b628b165e4ddcb5 
>   src/master/master.cpp 8921964a3ed74538b8c831a2f827d07875c1a5ab 
> 
> 
> Diff: https://reviews.apache.org/r/65095/diff/2/
> 
> 
> Testing
> -------
> 
> `make check`
> 
> 
> Thanks,
> 
> Benjamin Bannier
> 
>


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