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 63976: Modified `downgradeResources` to use protobuf reflection.
Date Thu, 21 Dec 2017 02:24:29 GMT

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


Fix it, then Ship it!





src/common/resources_utils.hpp
Line 175 (original), 175-176 (patched)
<https://reviews.apache.org/r/63976/#comment272925>

    s/does/do/?
    
    Maybe remove the first sentence and just directly say "all-or-nothing"? Atomicity seems
a little obscure here to me



src/common/resources_utils.hpp
Lines 181-182 (patched)
<https://reviews.apache.org/r/63976/#comment272926>

    Does it keep going and finish the rest in the error case? Or does it stop converting and
return the error? From reading the above comments, it sounds like it wouldn't break early.



src/common/resources_utils.hpp
Lines 184-185 (patched)
<https://reviews.apache.org/r/63976/#comment272927>

    Are you saying that once a component has refined reservations, it cannot be downgraded
back to.. the version before we had reservation refinement? As written, it seems to be speaking
too generally about downgrades: whether 1.9 could be downgraded to 1.8 is a separate question?



src/common/resources_utils.hpp
Lines 186 (patched)
<https://reviews.apache.org/r/63976/#comment272928>

    s/message/resource/ ?
    
    s/Resources/Resource/ ?



src/common/resources_utils.cpp
Lines 743 (patched)
<https://reviews.apache.org/r/63976/#comment272929>

    s/Resources/Resource/ ?



src/common/resources_utils.cpp
Lines 779-781 (patched)
<https://reviews.apache.org/r/63976/#comment273047>

    Hm.. is returning bool here an optimization? I would imagine the resultant map could (or
I guess already does) contain the top level descriptor?
    
    Then, downgradeResourcesImpl would first check the passed in message descriptor and bail
if not contained?
    
    ```
    static Try<Nothing> downgradeResourcesImpl(
        Message* message,
        const hashmap<const Descriptor*, bool>& resourcesContainment)
    {
      CHECK_NOTNULL(message);
    
      const Descriptor* descriptor = message->GetDescriptor();
      
      if (!resourcesContainment.at(descriptor)) {
        return; // Done.
      }
      
      if (descriptor == mesos::Resource::descriptor()) {
        return downgradeResources(static_cast<mesos::Resource*>(message));
      }
      
      // When recursing into fields, I guess we don't bother checking
      // containement before recursing since the recursive call will check?
    ```
    
    Then the top level function gets a bit simpler?
    
    ```
    Try<Nothing> downgradeResources(Message* message)
    {
      const Descriptor* descriptor = message->GetDescriptor();
    
      hashmap<const Descriptor*, bool> resourcesContainment;
      internal::precomputeResourcesContainment(descriptor, &resourcesContainment);
      
      return internal::downgradeResourcesImpl(message, resourcesContainment);
    ```
    
    Just curious to get your thoughts on the two approaches.



src/common/resources_utils.cpp
Lines 822 (patched)
<https://reviews.apache.org/r/63976/#comment273049>

    newline?



src/common/resources_utils.cpp
Lines 825 (patched)
<https://reviews.apache.org/r/63976/#comment273050>

    newline?



src/tests/resources_tests.cpp
Lines 3144-3162 (patched)
<https://reviews.apache.org/r/63976/#comment273051>

    These arent pre- right? Maybe you want to add a comment saying that you're expecting a
partial downgrade after the error?


- Benjamin Mahler


On Nov. 21, 2017, 6:07 a.m., Michael Park wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/63976/
> -----------------------------------------------------------
> 
> (Updated Nov. 21, 2017, 6:07 a.m.)
> 
> 
> Review request for mesos and Benjamin Mahler.
> 
> 
> Bugs: MESOS-8221
>     https://issues.apache.org/jira/browse/MESOS-8221
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> Previously, our `downgradeResources` function only took a pointer to
> `RepeatedPtrField<Resource>`. This wasn't great since we needed manually
> invoke `downgradeResources` on every `Resource`s field of every message.
> 
> This patch makes it such that `downgradeResources` can take any
> `protobuf::Message` or `protobuf::RepeatedPtrField<Resource>`.
> 
> 
> Diffs
> -----
> 
>   src/common/resources_utils.hpp 5b74ff2dd3ecb1a0101671d11ea10e29a43524b0 
>   src/common/resources_utils.cpp 1676b72a9ad15bf8b131698a0600a1b0937c00b4 
>   src/tests/resources_tests.cpp 64bde85e0e7f0bd395189eb6a8635383095b2f07 
> 
> 
> Diff: https://reviews.apache.org/r/63976/diff/2/
> 
> 
> Testing
> -------
> 
> Added new tests + `make check`
> 
> 
> Thanks,
> 
> Michael Park
> 
>


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