mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Chun-Hung Hsiao <chhs...@apache.org>
Subject Re: Review Request 67671: Prevented resource providers from changing their name or type.
Date Thu, 05 Jul 2018 22:47:01 GMT

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


Fix it, then Ship it!





src/resource_provider/registry.hpp
Lines 21 (patched)
<https://reviews.apache.org/r/67671/#comment288675>

    This is not used.
    
    I guess you probably has used this originally but changed the implementation to use explicit
comparison, which seems better as the code doesn't rely on the semantics of the message differencer.



src/resource_provider/registry.hpp
Lines 38-50 (patched)
<https://reviews.apache.org/r/67671/#comment288677>

    We could write the code in a style similar to `src/common/type_utils.cpp` is:
    ```
    return left.id() == right.id() &&
      (!left.has_type() || !right.has_type() || left.type() == right.type()) &&
      (!left.has_name() || !right.has_name() || left.name() == right.name()));
    ```
    
    But whether this is more readable or not is subjective so not opening an issue here.



src/resource_provider/registry.hpp
Lines 66 (patched)
<https://reviews.apache.org/r/67671/#comment288678>

    Would `google::protobuf::util::MessageToJsonString` be better here?



src/resource_provider/registry.proto
Lines 32-33 (patched)
<https://reviews.apache.org/r/67671/#comment288674>

    Let's swap the order:
    ```
      optional string type = 2;
      optional string name = 3;
    ```


- Chun-Hung Hsiao


On June 20, 2018, 12:35 p.m., Benjamin Bannier wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/67671/
> -----------------------------------------------------------
> 
> (Updated June 20, 2018, 12:35 p.m.)
> 
> 
> Review request for mesos, Chun-Hung Hsiao and Jan Schlicht.
> 
> 
> Bugs: MESOS-8838
>     https://issues.apache.org/jira/browse/MESOS-8838
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> Since the agent uses e.g., a resource provider's name or type to
> construct paths to persist resource provider state, changes to this
> information on resource provider resubscription are not supported.
> 
> This patch persists a resource provider's name and type in the
> resource provider registry and rejects a resource provider
> resubscription if incompatible changes are detected. Since we did not
> persist this information previous to mesos-1.7.0 we cannot and do not
> perform validation against resource provider registry information
> stored with earlier versions of Mesos.
> 
> 
> Diffs
> -----
> 
>   src/resource_provider/manager.cpp 6400e707ee834c73d4bf18c1f8d8344e8cf714cc 
>   src/resource_provider/registrar.hpp ded56e1c24a1f8b8db8dc70151682a7deb9e6544 
>   src/resource_provider/registrar.cpp a855a2b61fdbfc96a12a133c702ecaf7af666d8b 
>   src/resource_provider/registry.hpp 4c6c4d40686e3af8bbc7affbe3fa673479cc2fbf 
>   src/resource_provider/registry.proto 491263ef679cd4cea59338b002c6845d890f8eae 
>   src/tests/resource_provider_manager_tests.cpp 58bdbf0e88f59b5cb0cad42e38b24029fc0f2b41

> 
> 
> Diff: https://reviews.apache.org/r/67671/diff/1/
> 
> 
> Testing
> -------
> 
> `sudo make check`
> 
> 
> Thanks,
> 
> Benjamin Bannier
> 
>


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