> 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?
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. > 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? 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. - Andrei ----------------------------------------------------------- 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/1/ > > > Testing > ------- > > > Thanks, > > Andrei Sekretenko > >
