mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Benjamin Mahler <bmah...@apache.org>
Subject Re: Review Request 72822: Extracted FrameworkInfo comparison logic from tests.
Date Mon, 31 Aug 2020 21:05:50 GMT

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




src/common/protobuf_utils.hpp
Lines 142-159 (patched)
<https://reviews.apache.org/r/72822/#comment310811>

    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?



src/common/protobuf_utils.cpp
Lines 155-156 (patched)
<https://reviews.apache.org/r/72822/#comment310814>

    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;
    }
    ```



src/common/protobuf_utils.cpp
Lines 170 (patched)
<https://reviews.apache.org/r/72822/#comment310812>

    no period at the end



src/common/protobuf_utils.cpp
Lines 174-176 (patched)
<https://reviews.apache.org/r/72822/#comment310813>

    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?



src/common/protobuf_utils.cpp
Lines 184 (patched)
<https://reviews.apache.org/r/72822/#comment310815>

    Why does this work off of an rvalue reference but `equivalent(...)` does not?


- Benjamin Mahler


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