> On March 5, 2014, 12:54 a.m., Ben Mahler wrote: > > src/log/replica.cpp, lines 268-269 > > <https://reviews.apache.org/r/18368/diff/3/?file=507720#file507720line268> > > > > If this is really an error, why are we returning an empty interval set? > > Jie Yu wrote: > Well, this matches the previous semantics. I don't wanna introduce a new > semantics here. > > Jie Yu wrote: > Well, this matches the previous semantics. I don't wanna introduce a new > semantics here. Added a TODO to clean this up later. > > Ben Mahler wrote: > What I mean is: is this really an error or is the definition of 'missing' > the empty set when from > to? The fact that you added a LOG(ERROR) makes me > think it is the former. > > Ben Mahler wrote: > Chatted with Jie about what to do here. > > Since the definition of 'missing' for the empty interval is the empty > set, we should just do the right thing and return the empty set without > reporting an internal error. Mathematically, (and this is how boost's > interval works), 'from' > 'to' represents the empty interval. Ideally we can > design our API here to push the 'Interval' semantics to the callers, for > example, Replica::missing can take an 'Interval': > > Future<IntervalSet> ReplicaProcess::missing(const Interval& interval) > { > // Here we simply return the set of positions in the interval that are > "missing", > // when the empty interval is passed, the empty set is returned. > } > > Technically, 'Replica::read' has similar semantics where we return a > Failure instead of the empty list, so it would be interesting to know why > Failure is returned there.
Fixed. Added a comment and a TODO. > Technically, 'Replica::read' has similar semantics where we return a Failure > instead of the empty list, so it would > be interesting to know why Failure > is returned there. IMO, 'missing' is a check and 'read' is an action. We should not return a failure for a 'check', but we should return failure for an invalid 'action'. - Jie ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/18368/#review36198 ----------------------------------------------------------- On March 5, 2014, 2:14 a.m., Jie Yu wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/18368/ > ----------------------------------------------------------- > > (Updated March 5, 2014, 2:14 a.m.) > > > Review request for mesos, Benjamin Hindman and Ben Mahler. > > > Repository: mesos-git > > > Description > ------- > > See summary. > > > Diffs > ----- > > src/log/catchup.hpp 45dc016 > src/log/catchup.cpp dae4619 > src/log/coordinator.cpp 6bfff1e > src/log/recover.cpp 3403b47 > src/log/replica.hpp 08ddcb1 > src/log/replica.cpp 1f1a945 > src/log/storage.hpp c0eba1b > src/tests/log_tests.cpp 7f2a740 > > Diff: https://reviews.apache.org/r/18368/diff/ > > > Testing > ------- > > make check > > > Thanks, > > Jie Yu > >
