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 10:39:44 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?

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


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