> 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. > > Benjamin Mahler wrote: > 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.
Moved `equivalent`/`diff` into `type_utils.hpp` (and, for v1, into `v1/mesos.hpp`). After adjusting the existing tests that were relying on the stale `operator==` (and that actually need to compare `FrameworkInfo`s in slightly different ways), I'm even more in favour of `equivalent`. The next patch (https://reviews.apache.org/r/72833) removes the `operator==` and fixes the tests. > On Aug. 31, 2020, 9:05 p.m., Benjamin Mahler wrote: > > src/common/protobuf_utils.cpp > > Lines 155-156 (patched) > > <https://reviews.apache.org/r/72822/diff/1/?file=2238952#file2238952line155> > > > > Is this saying that MessageDifferencer is thread safe or not thread > > safe? > > > > I guess this comment is commenting on why we don't have a function like > > the following? > > > > ``` > > MessageDifferencer frameworkInfoDifferencer() > > { > > MessageDifferencer differencer; > > // set it up > > return differencer; > > } > > ``` Yes, exactly. Extended the comment (added a conditional TODO in case MessageDifferencer becomes move-constructible some day). > 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. > > Benjamin Mahler wrote: > 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. Removed `EQUIVALENT`, as it helps to avoid only a single trivial adjustment in two tests (`checkpointing` field). Indeed, we'd better have rare false positives when a field is changed from not set to the value equal to default. > On Aug. 31, 2020, 9:05 p.m., Benjamin Mahler wrote: > > src/common/protobuf_utils.cpp > > Lines 184 (patched) > > <https://reviews.apache.org/r/72822/diff/1/?file=2238952#file2238952line184> > > > > Why does this work off of an rvalue reference but `equivalent(...)` > > does not? Added rvalue-ref qualifier to `equivalent` as well and slightly expanded the comment. `diff` needs to be rvalue-ref qualified because it leaves the differencer in an invalid state (diff reported to a no longer existing string). In principle, I could add a cleanup code, but that cleanup code would be a dead code unless we bother to unit-test this class (note that testing `diff()`/`equivalent()` wrapper functions will not cover this). - Andrei ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/72822/#review221758 ----------------------------------------------------------- On Sept. 1, 2020, 7:45 p.m., Andrei Sekretenko wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/72822/ > ----------------------------------------------------------- > > (Updated Sept. 1, 2020, 7:45 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 > ----- > > include/mesos/type_utils.hpp da9fd9694b08827b968514b4b078097c912640c8 > include/mesos/v1/mesos.hpp d8304f10d6ac6bbcf014e515e0bfd77a2b96e138 > src/Makefile.am 1043c7b860372a17dba1e84fe5547388cb6a3b63 > src/common/type_utils.cpp bb437844183688e7f69eb0874d87efe5a7d6daf2 > src/common/type_utils_impl.hpp PRE-CREATION > src/tests/master/update_framework_tests.cpp > d2a63b696e976d9e3c729390d80f33180ab4aa1c > src/v1/mesos.cpp c2fb528d323899c42b32c0713b38ebf6f909dd29 > > > Diff: https://reviews.apache.org/r/72822/diff/3/ > > > Testing > ------- > > > Thanks, > > Andrei Sekretenko > >
