----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/19176/#review37944 -----------------------------------------------------------
Hey Adam, looks pretty good overall! I'm wondering if we can make the diff smaller for reviewers as we really don't want to make any mistakes in the Master/Slave code. Per my comment below, can we preserve the 'FrameworkID' member inside the 'Framework' structs? There are also some unrelated style changes in this diff that would be great to pull out and depend on in this change for easier reviews, but up to you! src/master/master.hpp <https://reviews.apache.org/r/19176/#comment69789> It looks like we could really minimize the number of changes we need to make to the Master by keeping the 'id' field for convenient access. Keeping your constructor change above as is though. There are still two ways to get the id with this new code: via 'id()' and 'info.id()', so I'm not sure what we're gaining by introducing the id accessor? src/master/master.cpp <https://reviews.apache.org/r/19176/#comment69783> How about we add a little comment above here saying: // Assign a new FrameworkID. Also if you leave '// mutable copy', can you update it to '// Mutable copy.'? src/slave/slave.hpp <https://reviews.apache.org/r/19176/#comment69791> Ditto here. - Ben Mahler On March 20, 2014, 7:43 a.m., Adam B wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/19176/ > ----------------------------------------------------------- > > (Updated March 20, 2014, 7:43 a.m.) > > > Review request for mesos and Ben Mahler. > > > Bugs: MESOS-905 > https://issues.apache.org/jira/browse/MESOS-905 > > > Repository: mesos-git > > > Description > ------- > > Eliminated the Framework.id FrameworkID field in master and slave, and > replaced each with a Framework.id() accessor that references > Framework.FrameworkInfo.id. > Removed redundant FrameworkID parameters when FrameworkInfo parameters also > exist with valid contained FrameworkID. > Some messages (RunTaskMessage, ExecutorRegisteredMessage) will no longer need > the FrameworkID member, but it is 'required' and cannot be easily deprecated. > Due to rolling upgrades, we must (a) continue to set the FrameworkID in the > messages, in case the recipient is on an older version that needs it; and (b) > merge the FrameworkID into the FrameworkInfo.id, in case we are receiving a > message from an older sender. > > > Diffs > ----- > > src/master/allocator.hpp 2e6a910 > src/master/hierarchical_allocator_process.hpp 3ec453a > src/master/http.cpp 72d8e91 > src/master/master.hpp 0c7c520 > src/master/master.cpp 6da7766 > src/messages/messages.proto c26a3d0 > src/slave/http.cpp 594032d > src/slave/slave.hpp 01b80df > src/slave/slave.cpp d8d3e0f > src/tests/allocator_tests.cpp 31cc836 > src/tests/allocator_zookeeper_tests.cpp 9ad0fa7 > src/tests/mesos.hpp f77fbfe > src/tests/resource_offers_tests.cpp cf910e5 > src/tests/slave_recovery_tests.cpp 40a9599 > > Diff: https://reviews.apache.org/r/19176/diff/ > > > Testing > ------- > > make check on ubuntu(Mint14)/gcc4.7.2. > > > Thanks, > > Adam B > >
