> On Aug. 31, 2020, 9:05 p.m., Benjamin Mahler wrote: > > src/common/protobuf_utils.hpp > > Lines 142-159 (patched) > > <https://reviews.apache.org/r/72822/diff/1/?file=2238951#file2238951line142> > > > > In type_utils.hpp, we have a lot of operator == overloads for all of > > our protobuf types, including (a broken) one for FrameworkInfo. Any > > thoughts on why we might want to break the consistency here? > > > > Perhaps we should fix the existing one? For diffs, maybe we have a > > bunch of `typeutils::diff(X x1, X x2)` overloads in that file? > > Andrei Sekretenko wrote: > Yes, I'm also wondering what to do with the supposedly broken `operator > == ` for FrameworkInfo declared in the public header that has been "broken" > since at least 2012. > > By "fixing" that operator (and adjusting all the Mesos tests that rely on > its behaviour), we might silently break external code that relies on the > current semantics. > > Maybe we should simply remove this overload, so that any depending code > fails to compile? > > In that case, we could replace it with `typeutils::equivalent()` in that > file, so that the potential replacement is easy to find, and also add > `typeutils::diff()` there. > > -- > In either case, switching our tests to comparison backed by > MessageDifferencer - regardless of how we name it - sounds like a good idea. > This means that leaving `operator ==` like this is not an option, > otherwise it will be accidentally used somewhere else again.
I don't think we need to worry about external code (if by that, you mean any users of our public headers). We don't bother with any compatibility there. I think == is a bit easier to use but I guess the downside there is one might just assume it's more of an "exactly equals" than "equivalent", so having a more explicit name seems good. > On Aug. 31, 2020, 9:05 p.m., Benjamin Mahler wrote: > > src/common/protobuf_utils.cpp > > Lines 174-176 (patched) > > <https://reviews.apache.org/r/72822/diff/1/?file=2238952#file2238952line174> > > > > Perhaps a comment saying that this treats unset the same as set to > > default? > > > > Do we want that? > > > > We often use set-ness checks: > > > > if (x.has_foo()) { > > // do something > > } > > > > so that might make set to default different than not set? > > > > Perhaps we should just err on the side of more strict equality here and > > use EQUALS? > > Andrei Sekretenko wrote: > That's a good question whether we actually want this, especially given > that API endpoints tend to report the missing (i.e. not set by the framework) > fields as set to their default value. > > I'll need to investigate how our tests handle that. > > If removing `EQUIVALENT` boils down to only adjusting a couple of tests, > I'll just remove it. I guess in this line of thinking we need to distinguish between field presence checks for built in types vs sub-messages. For the latter, we definitely do a bunch of presence checks, but I'm not sure if we do it as often for `string`, `int32` etc fields. - Benjamin ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/72822/#review221758 ----------------------------------------------------------- On Aug. 31, 2020, 6:03 p.m., Andrei Sekretenko wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/72822/ > ----------------------------------------------------------- > > (Updated Aug. 31, 2020, 6:03 p.m.) > > > Review request for mesos and Benjamin Mahler. > > > Bugs: MESOS-10166 > https://issues.apache.org/jira/browse/MESOS-10166 > > > Repository: mesos > > > Description > ------- > > This patch extracts the code that performs `FrameworkInfo` comparison > via protobuf `MessageDifferencer` from the framework update tests > into a set of general-purpose methods. > > > Diffs > ----- > > src/common/protobuf_utils.hpp 0558249023b39863b4030ddbcfe5c83dc945570a > src/common/protobuf_utils.cpp 723d85a8656e61f77ab99e5e63f844ec95303ff0 > src/tests/master/update_framework_tests.cpp > 6346a4eaf2b6c70d1a7d5f32706e781436d36521 > > > Diff: https://reviews.apache.org/r/72822/diff/2/ > > > Testing > ------- > > > Thanks, > > Andrei Sekretenko > >
