> 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 > >
