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