> On Sept. 19, 2016, 10:40 p.m., Vinod Kone wrote: > > src/cli/execute.cpp, line 384 > > <https://reviews.apache.org/r/52069/diff/1/?file=1504202#file1504202line384> > > > > While it's true that this can only be done once, I would vote to keep > > it close to the `if (task.isSome())` loop because the CHECK just above it > > makes it clear that `taskGroup` is some in the `else` block. > > Guangya Liu wrote: > Thanks Viond, got it. If this is the case, then for > https://github.com/apache/mesos/blob/master/src/cli/execute.cpp#L557-L558 , > do we need to move this right before > https://github.com/apache/mesos/blob/master/src/cli/execute.cpp#L600 ?
yup, good catch! you can use this review to do that :) also needs a new line before #562 - Vinod ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/52069/#review149558 ----------------------------------------------------------- On Sept. 19, 2016, 10:34 p.m., Guangya Liu wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/52069/ > ----------------------------------------------------------- > > (Updated Sept. 19, 2016, 10:34 p.m.) > > > Review request for mesos, Abhishek Dasgupta and Vinod Kone. > > > Repository: mesos > > > Description > ------- > > We only need to `CHECK_NE(task.isSome(), taskGroup.isSome())` when > get offers and there is no need to `CHECK_NE` for each offer. > > > Diffs > ----- > > src/cli/execute.cpp f1806723b25c72839475769e85fd7cbe0126d67d > > Diff: https://reviews.apache.org/r/52069/diff/ > > > Testing > ------- > > make > make check > > > Thanks, > > Guangya Liu > >
