> On April 7, 2015, 4:06 a.m., Adam B wrote: > > src/slave/slave.cpp, line 1108 > > <https://reviews.apache.org/r/32583/diff/4/?file=914631#file914631line1108> > > > > If we're always making the copy, should we just pass `frameworkInfo_` > > by value and force the copy at the call-site? Then there's no need for the > > `_` name.
That's a valid point. However, in a later patch (https://reviews.apache.org/r/32587), when the frameworkInfo is always containing a valid frameworkId, we need it to be const ref. I didn't want to change the signature of runTask() and that's why kept the code this way. If there is a preference, I can fix the code as you suggested. > On April 7, 2015, 4:06 a.m., Adam B wrote: > > src/slave/slave.cpp, lines 1109-1110 > > <https://reviews.apache.org/r/32583/diff/4/?file=914631#file914631line1109> > > > > Isn't this what MergeFrom does? I wanted it to be explicit so as to ensure readability. A later patch (https://reviews.apache.org/r/32587), removes the `if` block altogether. - Kapil ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/32583/#review79142 ----------------------------------------------------------- On April 7, 2015, 12:59 p.m., Kapil Arya wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/32583/ > ----------------------------------------------------------- > > (Updated April 7, 2015, 12:59 p.m.) > > > Review request for mesos, Adam B and Niklas Nielsen. > > > Bugs: MESOS-2558 > https://issues.apache.org/jira/browse/MESOS-2558 > > > Repository: mesos > > > Description > ------- > > The new preferred location for FrameworkID is FrameworkInfo.id. This patchset > achieves this goal by incrementally deprecating other locations for > FrameworkID. > > Here is a plan to deal with the upgrade path: > > For this release (N), we still keep setting RunTaskMessage::framework_id > - this would handle older Slaves with newer Master. > - added code to handle it being unset in the Slave (handles older > Master with newer Slaves). > > In the following release (N+1), stop reading/setting > RunTaskMessage::framework_id > - the previous version would handle the unset case. > > In release N+2, remove the field altogether: > - the previous release is not setting/reading it. > > > Diffs > ----- > > src/messages/messages.proto 97c45c01dfcea38b1ae555c036d61e10c152c2c8 > src/slave/slave.cpp c7e65a6c095963feaa9d5fdbb519c68f8f761d16 > > Diff: https://reviews.apache.org/r/32583/diff/ > > > Testing > ------- > > make check. > > TODO: Test for upgrade path. > > > Thanks, > > Kapil Arya > >
