You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@mesos.apache.org by Kapil Arya <ka...@mesosphere.io> on 2015/03/31 22:28:47 UTC
Re: Review Request 32583: Marked RunTaskMessage::framework_id as
optional.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/32583/
-----------------------------------------------------------
(Updated March 31, 2015, 4:28 p.m.)
Review request for mesos, Adam B and Niklas Nielsen.
Changes
-------
Addressed Adam's comments.
Bugs: MESOS-2558
https://issues.apache.org/jira/browse/MESOS-2558
Repository: mesos
Description
-------
For this release (N), we still keep setting it (handles 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 it (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 (updated)
-----
src/messages/messages.proto 97c45c01dfcea38b1ae555c036d61e10c152c2c8
src/slave/slave.cpp c7e65a6c095963feaa9d5fdbb519c68f8f761d16
Diff: https://reviews.apache.org/r/32583/diff/
Testing (updated)
-------
make check.
TODO: Test for upgrade path.
Thanks,
Kapil Arya
Re: Review Request 32583: Marked RunTaskMessage::framework_id as
optional.
Posted by Adam B <ad...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/32583/#review78494
-----------------------------------------------------------
Should mention in the Description (commit message) that the new preferred location for the FrameworkID is or will be in FrameworkInfo.id.
Would also like for you to replace ambiguous instances of 'it' in the description with the actual field/message to which 'it' refers.
src/slave/slave.cpp
<https://reviews.apache.org/r/32583/#comment127312>
CopyFrom?
- Adam B
On March 31, 2015, 1:28 p.m., Kapil Arya wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/32583/
> -----------------------------------------------------------
>
> (Updated March 31, 2015, 1:28 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
> -------
>
> For this release (N), we still keep setting it (handles 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 it (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
>
>
Re: Review Request 32583: Marked RunTaskMessage::framework_id as
optional.
Posted by Kapil Arya <ka...@mesosphere.io>.
> On April 11, 2015, 4:11 a.m., Adam B wrote:
> > Did you ever (manually?) "Test for upgrade path"?
Sorry for not replying earlier. I tested manually with 0.22.0 and HEAD. I tried combinations of old/new pairs of master, slave and test-framework. I also tried Niklas's upgrade script (https://reviews.apache.org/r/31645/) but had to make a small modification to it (possibly a bug in the script itself). Everything seemed to work fine.
- Kapil
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/32583/#review79798
-----------------------------------------------------------
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
>
>
Re: Review Request 32583: Marked RunTaskMessage::framework_id as
optional.
Posted by Adam B <ad...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/32583/#review79798
-----------------------------------------------------------
Ship it!
Did you ever (manually?) "Test for upgrade path"?
- Adam B
On April 7, 2015, 9:59 a.m., Kapil Arya wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/32583/
> -----------------------------------------------------------
>
> (Updated April 7, 2015, 9:59 a.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
>
>
Re: Review Request 32583: Marked RunTaskMessage::framework_id as
optional.
Posted by Kapil Arya <ka...@mesosphere.io>.
-----------------------------------------------------------
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.
Changes
-------
Addressed Adam's comments.
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 (updated)
-----
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
Re: Review Request 32583: Marked RunTaskMessage::framework_id as
optional.
Posted by Kapil Arya <ka...@mesosphere.io>.
> 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
>
>
Re: Review Request 32583: Marked RunTaskMessage::framework_id as
optional.
Posted by Adam B <ad...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/32583/#review79142
-----------------------------------------------------------
src/slave/slave.cpp
<https://reviews.apache.org/r/32583/#comment128278>
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.
src/slave/slave.cpp
<https://reviews.apache.org/r/32583/#comment128280>
Isn't this what MergeFrom does?
src/slave/slave.cpp
<https://reviews.apache.org/r/32583/#comment128276>
Is this another instance where we're taking a const reference of a temporary?
https://gist.github.com/jmlvanre/8a3de53ae88c2d19b375
- Adam B
On April 3, 2015, 7:05 a.m., Kapil Arya wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/32583/
> -----------------------------------------------------------
>
> (Updated April 3, 2015, 7:05 a.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
>
>
Re: Review Request 32583: Marked RunTaskMessage::framework_id as
optional.
Posted by Kapil Arya <ka...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/32583/
-----------------------------------------------------------
(Updated April 3, 2015, 10:05 a.m.)
Review request for mesos, Adam B and Niklas Nielsen.
Changes
-------
Addressed Joris's comment.
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 (updated)
-----
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
Re: Review Request 32583: Marked RunTaskMessage::framework_id as
optional.
Posted by Joris Van Remoortere <jo...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/32583/#review78704
-----------------------------------------------------------
src/messages/messages.proto
<https://reviews.apache.org/r/32583/#comment127658>
Add deprecated tag to indicate that no one should use this going forward. It doesn't affect the C++ code and only adds @deprecated for Java.
optional FrameworkID framework_id = 1 [deprecated = true];
- Joris Van Remoortere
On April 1, 2015, 7:34 p.m., Kapil Arya wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/32583/
> -----------------------------------------------------------
>
> (Updated April 1, 2015, 7:34 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
>
>
Re: Review Request 32583: Marked RunTaskMessage::framework_id as
optional.
Posted by Kapil Arya <ka...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/32583/
-----------------------------------------------------------
(Updated April 1, 2015, 3:34 p.m.)
Review request for mesos, Adam B and Niklas Nielsen.
Changes
-------
Addressed Adam's concerns.
Bugs: MESOS-2558
https://issues.apache.org/jira/browse/MESOS-2558
Repository: mesos
Description (updated)
-------
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 (updated)
-----
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