----------------------------------------------------------- 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) <https://reviews.apache.org/r/60562/#comment254150> 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) <https://reviews.apache.org/r/60562/#comment254153> 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) <https://reviews.apache.org/r/60562/#comment254152> 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) <https://reviews.apache.org/r/60562/#comment254151> 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 > >
