mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Michael Park <mp...@apache.org>
Subject Re: Review Request 63976: Modified `downgradeResources` to use protobuf reflection.
Date Tue, 02 Jan 2018 20:37:36 GMT


> On Dec. 20, 2017, 6:24 p.m., Benjamin Mahler wrote:
> > src/common/resources_utils.hpp
> > Line 175 (original), 175-176 (patched)
> > <https://reviews.apache.org/r/63976/diff/2/?file=1924144#file1924144line175>
> >
> >     s/does/do/?
> >     
> >     Maybe remove the first sentence and just directly say "all-or-nothing"? Atomicity
seems a little obscure here to me

Removed the first sentence.


> On Dec. 20, 2017, 6:24 p.m., Benjamin Mahler wrote:
> > src/common/resources_utils.hpp
> > Lines 181-182 (patched)
> > <https://reviews.apache.org/r/63976/diff/2/?file=1924144#file1924144line181>
> >
> >     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.

The current behavior is that it stops converting and returns the error. I'll update the comment
accordingly.


> On Dec. 20, 2017, 6:24 p.m., Benjamin Mahler wrote:
> > src/common/resources_utils.hpp
> > Lines 184-185 (patched)
> > <https://reviews.apache.org/r/63976/diff/2/?file=1924144#file1924144line184>
> >
> >     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?

Yeah. Good point. Updated to include "... to a version that does not support reservation refinement."


> On Dec. 20, 2017, 6:24 p.m., Benjamin Mahler wrote:
> > src/common/resources_utils.cpp
> > Lines 779-781 (patched)
> > <https://reviews.apache.org/r/63976/diff/2/?file=1924145#file1924145line783>
> >
> >     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.

I've removed the `bool` return since it was kind of a silly, and was not even an optimization.

However, I believe moving the containment check inside `downgradeResourcesImpl` would be
a regression in performance. Consider a protobuf message:
```
message A {
  repeated B messages;
  optional Resource resource;
}
```
and suppose `B` is not `Resource` nor does it have any `Resource`s within.

Since `A` has `resource`, we'll pass the `resourcesContainment.at(descriptor)` check.

Ideally we would skip `messages` entirely, since from the schema of `B` we know that
there aren't any `Resource`s in there. If we were to wait until the recursive call,
we would fully execute this loop going over each `B` in `messages`:
```
      const int size = reflection->FieldSize(*message, field);

      for (int j = 0; j < size; ++j) {
        Try<Nothing> result = downgradeResourcesImpl(
            reflection->MutableRepeatedMessage(message, field, j),
            resourcesContainment);

        if (result.isError()) {
          return result;
        }
      }
```

I think the issue is mainly in the way in which we need to treat repeated fields.
If we were able to get access to a repeated field of `Message`s and pass it to
the recursive call, we could have the check at the top of `downgradeResourcesImpl`.
But as far as I know, the only way to get access to `Message`s of a repeated field
is via `MutableRepeatedMessage`.


- Michael


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


On Nov. 20, 2017, 10:07 p.m., Michael Park wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/63976/
> -----------------------------------------------------------
> 
> (Updated Nov. 20, 2017, 10:07 p.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