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