You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@mesos.apache.org by Benjamin Mahler <bm...@apache.org> on 2017/12/21 02:24:29 UTC
Re: Review Request 63976: Modified `downgradeResources` to use
protobuf reflection.
-----------------------------------------------------------
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
>
>
Re: Review Request 63976: Modified `downgradeResources` to use
protobuf reflection.
Posted by Michael Park <mp...@apache.org>.
> 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
>
>