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 72833: Removed `bool operator==(const FrameworkInfo&, const FrameworkInfo&)`.
Date Wed, 02 Sep 2020 23:31:22 GMT

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


Fix it, then Ship it!





include/mesos/type_utils.hpp
Lines 119-122 (original)
<https://reviews.apache.org/r/72833/#comment310860>

    Hm.. Can we actually leave these but as `delete`d functions? Seems like C++ does allow
non-member functions to be deleted?
    
    https://abseil.io/tips/143
    https://stackoverflow.com/questions/42332777/what-is-the-point-of-using-delete-on-a-non-member-function
    
    ```
    // This has been deleted in favor of ...
    inline bool operator==(const FrameworkInfo& left, const FrameworkInfo& right)
= delete;
    ```
    
    That will make it a lot less likely that someone re-adds this operator when they find
it's missing. Also helps them find the equivalent function.



src/tests/api_tests.cpp
Lines 3354-3358 (patched)
<https://reviews.apache.org/r/72833/#comment310861>

    Should we just have DEFAULT_FRAMEWORK_INFO explicitly set it to false? Just a thought..



src/tests/master_allocator_tests.cpp
Lines 374 (patched)
<https://reviews.apache.org/r/72833/#comment310862>

    Not sure that this test needs to be concerned about checking the framework info equality
on addFramework calls, it could just check the framework id matches, but even that doesn't
seem necessary for this test?
    
    Especially since not checking or checking just the ID wouldn't require a custom matcher.


- Benjamin Mahler


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/72833/
> -----------------------------------------------------------
> 
> (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 removes the outdated `operator==` for `FrameworkInfo`
> (which has been comparing only `name` and `user` fields)
> and replaces it with `equivalent()` in the tests that need to compare
> `FrameworkInfo`s.
> 
> 
> Diffs
> -----
> 
>   include/mesos/type_utils.hpp da9fd9694b08827b968514b4b078097c912640c8 
>   include/mesos/v1/mesos.hpp d8304f10d6ac6bbcf014e515e0bfd77a2b96e138 
>   src/tests/api_tests.cpp e0ce0335bf99f1f624efcca8c86f797df472d68f 
>   src/tests/master_allocator_tests.cpp 416b7ba733c3a9fc75e64fecf088ff13548bab3f 
>   src/tests/resources_tests.cpp bd044310e297174155b88d5694641acd5df4cf59 
> 
> 
> Diff: https://reviews.apache.org/r/72833/diff/1/
> 
> 
> Testing
> -------
> 
> `support/mesos-gtest-runner.py src/mesos-tests`
> 
> 
> Thanks,
> 
> Andrei Sekretenko
> 
>


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