mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Benjamin Bannier <benjamin.bann...@mesosphere.io>
Subject Re: Review Request 67671: Prevented resource providers from changing their name or type.
Date Fri, 06 Jul 2018 08:50:14 GMT


> On July 6, 2018, 12:47 a.m., Chun-Hung Hsiao wrote:
> > src/resource_provider/registry.hpp
> > Lines 38-50 (patched)
> > <https://reviews.apache.org/r/67671/diff/1/?file=2042695#file2042695line38>
> >
> >     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.

I am leaving this as is as I find above form pretty hard to parse due to the deep nesting
and the not apparent coupling between parameters (we also shouldn't assume that applying De
Morgan's law is second nature for every reader or maintainer of this code).


> On July 6, 2018, 12:47 a.m., Chun-Hung Hsiao wrote:
> > src/resource_provider/registry.hpp
> > Lines 66 (patched)
> > <https://reviews.apache.org/r/67671/diff/1/?file=2042695#file2042695line66>
> >
> >     Would `google::protobuf::util::MessageToJsonString` be better here?

Possibly, but this is only used in a single place in the code while we use the debug repr
everywhere else.

Suggest to leave as is, and possibly perform a global cleanup if we really care. Dropping.


- Benjamin


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


On July 6, 2018, 10:50 a.m., Benjamin Bannier wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/67671/
> -----------------------------------------------------------
> 
> (Updated July 6, 2018, 10:50 a.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/2/
> 
> 
> Testing
> -------
> 
> `sudo make check`
> 
> 
> Thanks,
> 
> Benjamin Bannier
> 
>


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