> On Aug. 12, 2014, 9:34 p.m., Vinod Kone wrote:
> > src/master/master.cpp, lines 2140-2145
> > <https://reviews.apache.org/r/24576/diff/1/?file=658113#file658113line2140>
> >
> > Why loop through all validators and offers when one of them returns
> > error? Short circuting as did earlier seems fine to me, though a bit
> > verbose.
This was definitely an interesting one, here are the two versions:
New code:
// Find the first error.
foreach (const OfferID& offerId, offerIds) {
foreach (const Owned<OfferVisitor>& visitor, offerVisitors) {
if (error.isNone()) {
error = (*visitor)(offerId, *framework, this);
}
}
}
Old code:
// Find the first error.
foreach (const OfferID& offerId, offerIds) {
foreach (const Owned<OfferVisitor>& visitor, offerVisitors) {
error = (*visitor)(offerId, *framework, this);
if (error.isSome()) {
break;
}
}
if (error.isSome()) {
break;
}
}
The first one is easier to read and understand since it has less code and
control flow, the second one captures more of what we could do if we had the
ability to 'return' the first error, but we have to explicitly break both
loops, which as you said is a bit verbose and clunky.
Since the goal here was to simplify the master code for readers, I went with
the cleaner version. Keep in mind that the extra looping here will be
negligible since here since the run time complexity is the same either way. :)
Sound good?
- Ben
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/24576/#review50366
-----------------------------------------------------------
On Aug. 11, 2014, 11:32 p.m., Ben Mahler wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/24576/
> -----------------------------------------------------------
>
> (Updated Aug. 11, 2014, 11:32 p.m.)
>
>
> Review request for mesos, Niklas Nielsen and Vinod Kone.
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> There were a few cleanups here that I did on the way to MESOS-1620.
>
> (1) Remove the need for Offer/Task Visitor memory cleanup by using Owned<>.
> (2) Restructured the launch task code to be easier to read and understand.
>
>
> Diffs
> -----
>
> src/master/master.cpp e688b41b9f2e555acd8fe0da5d3eb4e8bce32211
>
> Diff: https://reviews.apache.org/r/24576/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Ben Mahler
>
>
