> 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
>
>
