Re: Review Request 60562: Updated `accept` to perform operation adjustment in one place.

[Benjamin Mahler]

> On July 1, 2017, 2:13 a.m., Vinod Kone wrote:
> > src/master/master.cpp
> > Lines 3997 (patched)
> > 
> >
> > Can you log a warning here?

I wouldn't log a warning here since it's the responsibility of the processing 
of operations that occurs later to log accordingly when dealing with unknown 
operations.


- Benjamin


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


On June 30, 2017, 9:01 a.m., Michael Park wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/60562/
> ---
> 
> (Updated June 30, 2017, 9:01 a.m.)
> 
> 
> Review request for mesos and Benjamin Mahler.
> 
> 
> Bugs: MESOS-7735
> https://issues.apache.org/jira/browse/MESOS-7735
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> It used to be that the minor adjustments that were made to operations
> were done in various places across `accept` and `_accept`.
> 
> The "executor-injection" for LAUNCH_GROUP was at the beginning of
> `accept`, "allocation-info-injection" for MULTI_ROLE was after offer
> validation, and "health-check-injection" for LAUNCH was in `_accept`.
> 
> The `Master::accept` function is now broken down into distinct
> "metrics accounting", "offer validation", "operation-adjustments", and
> "authorization" stages.
> 
> 
> Diffs
> -
> 
>   src/master/master.cpp 276b18e4d1bab7e9b28c1c93fe13c93a38abc5cd 
> 
> 
> Diff: https://reviews.apache.org/r/60562/diff/1/
> 
> 
> Testing
> ---
> 
> `make check`
> 
> 
> Thanks,
> 
> Michael Park
> 
>

Re: Review Request 60562: Updated `accept` to perform operation adjustment in one place.

[Benjamin Mahler]

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


Fix it, then Ship it!




Made some suggestions for cleanups, feel free to add TODOs and tackle them 
later in this chain. Looks good from a correctness perspective.


src/master/master.cpp
Lines 3883-3886 (original), 3870-3873 (patched)


I was about to make this exact suggestion to simplify this function, then 
studying the code more carefully I noticed we already added a TODO :)



src/master/master.cpp
Lines 3929-3930 (patched)


Any reason not to pull out the master specific normalization / upgrade of 
operations into a utility function that calls out to the various pieces? Seems 
to be a logical normalization / upgrade on Operations that the master needs to 
perform without consulting any of the master state?



src/master/master.cpp
Lines 3942-3947 (original), 4001-4005 (patched)


I would suggest putting the generalized normalization at the top before the 
switch, so that it's clear to the reader that there are general normalizations 
we apply before dealing with the operation specific logic here. At a first read 
of the code, it looks like it's all going to be within the switch.

Ideally, all the normalization could be factored together, I left another 
comment about that.



src/master/master.cpp
Lines 4738-4739 (original), 4764-4765 (patched)


Any reason these validateAndUpgradeResources in `_accept()` are not part of 
the normalization?

It's not obvious to me, maybe add a NOTE about how the normalization of 
operations isn't doing this, above the normalization block you added in 
accept()?


- Benjamin Mahler


On June 30, 2017, 9:01 a.m., Michael Park wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/60562/
> ---
> 
> (Updated June 30, 2017, 9:01 a.m.)
> 
> 
> Review request for mesos and Benjamin Mahler.
> 
> 
> Bugs: MESOS-7735
> https://issues.apache.org/jira/browse/MESOS-7735
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> It used to be that the minor adjustments that were made to operations
> were done in various places across `accept` and `_accept`.
> 
> The "executor-injection" for LAUNCH_GROUP was at the beginning of
> `accept`, "allocation-info-injection" for MULTI_ROLE was after offer
> validation, and "health-check-injection" for LAUNCH was in `_accept`.
> 
> The `Master::accept` function is now broken down into distinct
> "metrics accounting", "offer validation", "operation-adjustments", and
> "authorization" stages.
> 
> 
> Diffs
> -
> 
>   src/master/master.cpp 276b18e4d1bab7e9b28c1c93fe13c93a38abc5cd 
> 
> 
> Diff: https://reviews.apache.org/r/60562/diff/1/
> 
> 
> Testing
> ---
> 
> `make check`
> 
> 
> Thanks,
> 
> Michael Park
> 
>

Re: Review Request 60562: Updated `accept` to perform operation adjustment in one place.

[Vinod Kone]

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


Fix it, then Ship it!




This is great. Thanks for the cleanup.


src/master/master.cpp
Line 3819 (original)


Can you add a TODO to add metrics for LAUNCH_GROUP launch and decline as 
well?



src/master/master.cpp
Line 3940 (original), 3927 (patched)


move this to #4005?



src/master/master.cpp
Lines 3997 (patched)


Can you log a warning here?


- Vinod Kone


On June 30, 2017, 9:01 a.m., Michael Park wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/60562/
> ---
> 
> (Updated June 30, 2017, 9:01 a.m.)
> 
> 
> Review request for mesos and Benjamin Mahler.
> 
> 
> Bugs: MESOS-7735
> https://issues.apache.org/jira/browse/MESOS-7735
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> It used to be that the minor adjustments that were made to operations
> were done in various places across `accept` and `_accept`.
> 
> The "executor-injection" for LAUNCH_GROUP was at the beginning of
> `accept`, "allocation-info-injection" for MULTI_ROLE was after offer
> validation, and "health-check-injection" for LAUNCH was in `_accept`.
> 
> The `Master::accept` function is now broken down into distinct
> "metrics accounting", "offer validation", "operation-adjustments", and
> "authorization" stages.
> 
> 
> Diffs
> -
> 
>   src/master/master.cpp 276b18e4d1bab7e9b28c1c93fe13c93a38abc5cd 
> 
> 
> Diff: https://reviews.apache.org/r/60562/diff/1/
> 
> 
> Testing
> ---
> 
> `make check`
> 
> 
> Thanks,
> 
> Michael Park
> 
>

Re: Review Request 60562: Updated `accept` to perform operation adjustment in one place.

[Benjamin Bannier]

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




src/master/master.cpp
Lines 3929 (patched)


Just for clarification, is the plan for this block to ultimately absorb the 
mutating part of `validateAndUpgrade`? Looking at your patch 
https://reviews.apache.org/r/60563/ makes this look pretty (validation and 
update order inverted there wrt. here).



src/master/master.cpp
Lines 3932-3936 (patched)


Nice to see these finally handled by a `switch` instead of a couple of `if` 
statements!


- Benjamin Bannier


On June 30, 2017, 11:01 a.m., Michael Park wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/60562/
> ---
> 
> (Updated June 30, 2017, 11:01 a.m.)
> 
> 
> Review request for mesos and Benjamin Mahler.
> 
> 
> Bugs: MESOS-7735
> https://issues.apache.org/jira/browse/MESOS-7735
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> It used to be that the minor adjustments that were made to operations
> were done in various places across `accept` and `_accept`.
> 
> The "executor-injection" for LAUNCH_GROUP was at the beginning of
> `accept`, "allocation-info-injection" for MULTI_ROLE was after offer
> validation, and "health-check-injection" for LAUNCH was in `_accept`.
> 
> The `Master::accept` function is now broken down into distinct
> "metrics accounting", "offer validation", "operation-adjustments", and
> "authorization" stages.
> 
> 
> Diffs
> -
> 
>   src/master/master.cpp 276b18e4d1bab7e9b28c1c93fe13c93a38abc5cd 
> 
> 
> Diff: https://reviews.apache.org/r/60562/diff/1/
> 
> 
> Testing
> ---
> 
> `make check`
> 
> 
> Thanks,
> 
> Michael Park
> 
>