> On Nov. 21, 2017, 7:59 p.m., Greg Mann wrote: > > src/master/master.cpp > > Line 10925 (original), 10922 (patched) > > <https://reviews.apache.org/r/63842/diff/2/?file=1896903#file1896903line10925> > > > > Especially now that we have code in this function which switches on > > operation type, could we add a comment to the method's declaration briefly > > explaining its use? For example, it's not clear from reading the header > > that this method should not be called on LAUNCH or LAUNCH_GROUP operations.
Dropping this issue as this code is now much less concerned about particular operation types. > On Nov. 21, 2017, 7:59 p.m., Greg Mann wrote: > > src/master/master.cpp > > Lines 10936-10939 (patched) > > <https://reviews.apache.org/r/63842/diff/2/?file=1896903#file1896903line10939> > > > > Nit: could we put this case at the end? I think we tend to place the > > UNKNOWN case last. Dropping this issue as this code is now simplified by using a helper. - Benjamin ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/63842/#review191610 ----------------------------------------------------------- On Nov. 24, 2017, 3:52 p.m., Benjamin Bannier wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/63842/ > ----------------------------------------------------------- > > (Updated Nov. 24, 2017, 3:52 p.m.) > > > Review request for mesos, Greg Mann, Jie Yu, and Jan Schlicht. > > > Repository: mesos > > > Description > ------- > > During reconcilation we might be required to remove non-terminal offer > operations from bookkeeping. > > > Diffs > ----- > > src/master/master.hpp 2a2e830354db4a2191fb8321beb8174b80f7ba7d > src/master/master.cpp 53263e499d88b906b6406c24c0dfb737e589e813 > src/slave/slave.cpp f93ff7b20815c3ccb274ce6990ee66a17b6ac51c > > > Diff: https://reviews.apache.org/r/63842/diff/6/ > > > Testing > ------- > > `make check`, tested as part of https://reviews.apache.org/r/63843/. > > This patch requires `protobuf::isSpeculativeOperation` from > https://reviews.apache.org/r/63751/ which is _not_ part of this review chain > (to avoid multiple dependent RRs). > > > Thanks, > > Benjamin Bannier > >
