mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Kapil Arya <ka...@mesosphere.io>
Subject Re: Review Request 66746: Replaced protobuf-specific comparators with MessageDifferencer.
Date Tue, 24 Apr 2018 06:42:36 GMT


> On April 23, 2018, 8:43 p.m., Benjamin Mahler wrote:
> > I'm not sure this approach is tenable, we may in the future have two different `foo`
fields with differing equality semantics.
> > 
> > I was expecting that this patch would add an equality operator for specific types
we want to check. Do we need the `Message&` level equality operator?
> 
> Kapil Arya wrote:
>     Right now we don't have any differing equality semantics for any of the messages.
There are four non-standard semantics are captured in the generic method. In future, when
we need differring techniques for some bar, we can overload the `==` operator for that particular
type. Does that sound reasonable?
>     
>     Since, MessageDifferencer recurses down to the fields without calling `==` itself,
if we were to have two message Bar and Baz with with different equality semantics for `foo`
fields in them , we'd can implement `==` for both Bar and Baz to reflect that. For example:
>     ```
>     operator==(const Bar& left, const Bar& right) {
>       MessageDifferencer diff;
>       
>       // Bar-specific enhancements for Foo.
>       diff.TreatAsSet(Foo::descriptor()->FindFieldByName("some_field"));
>       ...
>       return diff.compare(left, right);
>     }
>     
>     operator==(const Baz& left, const Baz& right) {
>       MessageDifferencer diff;
>       
>       // Baz-speficic enhancements for Foo.
>       diff.TreatAsList(Foo::descriptor()->FindFieldByName("some_field"));
>       ...
>       return diff.compare(left, right);
>     }
>     ```
>     
>     However, if Bar and Baz were to have another common field `qux` with a non-standard
comparator, we'd have to duplicate the qux-specific enhancement in both Bar and Baz comparators.
This leads to some duplication, but that can be fixed with some clever maneuvering.

There is another possibility where we implement some generic MesosMessageDifferencer like
the one in the patch. Any implementations that need to deviate from the standard then take
a copy of MesosMessageDifferencer and modify it as needed.


- Kapil


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/66746/#review201799
-----------------------------------------------------------


On April 20, 2018, 4:19 p.m., Kapil Arya wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/66746/
> -----------------------------------------------------------
> 
> (Updated April 20, 2018, 4:19 p.m.)
> 
> 
> Review request for mesos, Benjamin Mahler and Till Toenshoff.
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> The MessageDifferencer utility has been available since protobuf 3.0.0.
> When comparing most repeated fields, we treat them as sets so the order
> of elements doesn't matter. In some exceptional cases, we treat them as
> list so the order becomes important. Further, fields with default
> purposes are considered to be set for comparison purposes.
> 
> 
> Diffs
> -----
> 
>   include/mesos/type_utils.hpp 19ea81716496bcc0117a1b0ff157a0374f38bbfa 
>   include/mesos/v1/mesos.hpp fda3eb42061f820869a2d8da939fccadc4e5ddfb 
>   src/common/type_utils.cpp 33d63809b61a18e03ff745c88f024c71dd221ca2 
>   src/tests/master_allocator_tests.cpp e1aef8a9625a805e7ad2dfad37bfeedee82f160d 
>   src/v1/mesos.cpp 9b2df2dd798dff24a91a2604ab53c7fbb5ecfbcf 
> 
> 
> Diff: https://reviews.apache.org/r/66746/diff/1/
> 
> 
> Testing
> -------
> 
> `make check` currently fails with:
>     DefaultExecutorCheckTest.CommandCheckSharesWorkDirWithTask
>     DefaultExecutorCheckTest.MultipleTasksWithChecks
> 
> 
> Thanks,
> 
> Kapil Arya
> 
>


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