mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Andrei Sekretenko <asekrete...@mesosphere.io>
Subject Re: Review Request 72822: Extracted FrameworkInfo comparison logic from tests.
Date Tue, 01 Sep 2020 20:04:37 GMT


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


Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message