Re: Review Request 65095: Made it possible to update an operation without mutating resources.

2018-01-12 Thread Greg Mann 

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


Ship it!





src/master/master.cpp
Line 10318 (original), 10319 (patched)


As we discussed in chat, while this isn't a huge deal at present (since 
updates are being acknowledged immediately by the master), we'll want to change 
this anyway once we have the framework API in place for operation feedback. If 
this helper is going to be called upon every receipt of an 
UpdateOperationStatusMessage, then we won't want to push the statuses from 
update retries onto the master's internal state. Once we're relying on 
frameworks to acknowledge, this could be an issue.

Gaston and I will follow up with a patch to implement some simple 
de-duplication for now, and we can revisit correct de-duplication semantics 
later on, when the operation update workflow becomes more complex.


- Greg Mann


On Jan. 12, 2018, 10: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, 10: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
> 
>

Re: Review Request 65095: Made it possible to update an operation without mutating resources.

2018-01-12 Thread Jan Schlicht 

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


Ship it!




Ship It!

- Jan Schlicht


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

Re: Review Request 65095: Made it possible to update an operation without mutating resources.

2018-01-12 Thread Benjamin Bannier 


> On Jan. 11, 2018, 10 p.m., Greg Mann wrote:
> > src/master/master.hpp
> > Lines 881-885 (original), 881-888 (patched)
> > 
> >
> > 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
> 
>

Re: Review Request 65095: Made it possible to update an operation without mutating resources.

2018-01-12 Thread Benjamin Bannier 

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


Changes
---

Implemented suggestions by Greg.


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 (updated)
-

  src/master/master.hpp cdfd06ceb2a8eafa60d0af382b628b165e4ddcb5 
  src/master/master.cpp 8921964a3ed74538b8c831a2f827d07875c1a5ab 


Diff: https://reviews.apache.org/r/65095/diff/2/

Changes: https://reviews.apache.org/r/65095/diff/1-2/


Testing
---

`make check`


Thanks,

Benjamin Bannier

Re: Review Request 65095: Made it possible to update an operation without mutating resources.

2018-01-11 Thread Greg Mann 

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




src/master/master.hpp
Lines 881-885 (original), 881-888 (patched)


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



src/master/master.cpp
Lines 10409-10416 (original), 10418-10427 (patched)


Is there any reason not to move all this into the first branch of the above 
conditional?


- Greg Mann


On Jan. 11, 2018, 1:53 p.m., Benjamin Bannier wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65095/
> ---
> 
> (Updated Jan. 11, 2018, 1:53 p.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 7ed15e4ba2a31c5fe4b8571f645cdca69a3e82f4 
> 
> 
> Diff: https://reviews.apache.org/r/65095/diff/1/
> 
> 
> Testing
> ---
> 
> `make check`
> 
> 
> Thanks,
> 
> Benjamin Bannier
> 
>

Review Request 65095: Made it possible to update an operation without mutating resources.

2018-01-11 Thread Benjamin Bannier 

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

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


Diff: https://reviews.apache.org/r/65095/diff/1/


Testing
---

`make check`


Thanks,

Benjamin Bannier