> 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
> 
>