You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@mesos.apache.org by Kapil Arya <ka...@mesosphere.io> on 2015/07/28 15:38:06 UTC
Re: Review Request 32587: Stopped using RunTaskMessage.framework_id.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/32587/
-----------------------------------------------------------
(Updated July 28, 2015, 9:38 a.m.)
Review request for mesos, Adam B and Niklas Nielsen.
Bugs: MESOS-2559
https://issues.apache.org/jira/browse/MESOS-2559
Repository: mesos
Description
-------
FrameworkID can be retrieved from RunTaskMessage.framework.
NOTE: This patch is only to be merged _ONLY_ after all the dependent patches have shipped, i.e. after 0.23.0 (tracked here: https://issues.apache.org/jira/browse/MESOS-2561) has released.
Diffs
-----
src/master/master.cpp dccd7c635da4b7031cd109bd84e7f17b31777ef1
src/slave/slave.cpp c7e65a6c095963feaa9d5fdbb519c68f8f761d16
src/tests/mesos.cpp 11e88330dd50913ab00da79054225d113541e83e
Diff: https://reviews.apache.org/r/32587/diff/
Testing
-------
make check
TODO: Test for upgrade path.
Thanks,
Kapil Arya
Re: Review Request 32587: Stopped using RunTaskMessage.framework_id.
Posted by Adam B <ad...@mesosphere.io>.
> On July 30, 2015, 9:24 a.m., Adam B wrote:
> > src/slave/slave.cpp, lines 1243-1245
> > <https://reviews.apache.org/r/32587/diff/6/?file=1025122#file1025122line1243>
> >
> > Maybe just warn and leave the CopyFrom there?
> > Next release, we'll remove the field entirely, and consequently this check too.
>
> Kapil Arya wrote:
> With the previous Master (0.23.0), the frameworkInfo.id field is always set, so I am not sure if we need this check at all. Right now, I can't think of a case where we would need this check. Do you have something in mind?
Fair enough. I wasn't thinking of a valid use case, just that, generally speaking, a misbehaving sender or invalid message should not kill the slave. Logging an error is probably better here than warning and continuing, if the master is misbehaving or really old. Dropping the issue.
- Adam
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/32587/#review93591
-----------------------------------------------------------
On July 30, 2015, 9:18 a.m., Kapil Arya wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/32587/
> -----------------------------------------------------------
>
> (Updated July 30, 2015, 9:18 a.m.)
>
>
> Review request for mesos, Adam B and Niklas Nielsen.
>
>
> Bugs: MESOS-2559
> https://issues.apache.org/jira/browse/MESOS-2559
>
>
> Repository: mesos
>
>
> Description
> -------
>
> FrameworkID can be retrieved from RunTaskMessage.framework.
>
> NOTE: This patch is only to be merged _ONLY_ after all the dependent patches have shipped, i.e. after 0.23.0 (tracked here: https://issues.apache.org/jira/browse/MESOS-2561) has released.
>
>
> Diffs
> -----
>
> src/master/master.cpp a8a195df07b5a97fdba7dfc5f312bbfa85a0d510
> src/slave/slave.cpp f91fa9204cd89596a3690c55c22e93429392cbfd
> src/tests/mesos.cpp f3b731542f9db4f966970ecb2bb96eb828350dea
>
> Diff: https://reviews.apache.org/r/32587/diff/
>
>
> Testing
> -------
>
> make check
>
> Also ran Nik's test-upgrade.py script (https://reviews.apache.org/r/31645) with 0.23.0 and the current master to verify compatibility checks.
>
>
> Thanks,
>
> Kapil Arya
>
>
Re: Review Request 32587: Stopped using RunTaskMessage.framework_id.
Posted by Kapil Arya <ka...@mesosphere.io>.
> On July 30, 2015, 12:24 p.m., Adam B wrote:
> > src/slave/slave.cpp, lines 1243-1245
> > <https://reviews.apache.org/r/32587/diff/6/?file=1025122#file1025122line1243>
> >
> > Maybe just warn and leave the CopyFrom there?
> > Next release, we'll remove the field entirely, and consequently this check too.
With the previous Master (0.23.0), the frameworkInfo.id field is always set, so I am not sure if we need this check at all. Right now, I can't think of a case where we would need this check. Do you have something in mind?
- Kapil
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/32587/#review93591
-----------------------------------------------------------
On July 30, 2015, 12:18 p.m., Kapil Arya wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/32587/
> -----------------------------------------------------------
>
> (Updated July 30, 2015, 12:18 p.m.)
>
>
> Review request for mesos, Adam B and Niklas Nielsen.
>
>
> Bugs: MESOS-2559
> https://issues.apache.org/jira/browse/MESOS-2559
>
>
> Repository: mesos
>
>
> Description
> -------
>
> FrameworkID can be retrieved from RunTaskMessage.framework.
>
> NOTE: This patch is only to be merged _ONLY_ after all the dependent patches have shipped, i.e. after 0.23.0 (tracked here: https://issues.apache.org/jira/browse/MESOS-2561) has released.
>
>
> Diffs
> -----
>
> src/master/master.cpp a8a195df07b5a97fdba7dfc5f312bbfa85a0d510
> src/slave/slave.cpp f91fa9204cd89596a3690c55c22e93429392cbfd
> src/tests/mesos.cpp f3b731542f9db4f966970ecb2bb96eb828350dea
>
> Diff: https://reviews.apache.org/r/32587/diff/
>
>
> Testing
> -------
>
> make check
>
> Also ran Nik's test-upgrade.py script (https://reviews.apache.org/r/31645) with 0.23.0 and the current master to verify compatibility checks.
>
>
> Thanks,
>
> Kapil Arya
>
>
Re: Review Request 32587: Stopped using RunTaskMessage.framework_id.
Posted by Adam B <ad...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/32587/#review93591
-----------------------------------------------------------
src/slave/slave.cpp (line 1242)
<https://reviews.apache.org/r/32587/#comment147973>
Trailing ' ' in log message.
src/slave/slave.cpp (lines 1241 - 1243)
<https://reviews.apache.org/r/32587/#comment147974>
Maybe just warn and leave the CopyFrom there?
Next release, we'll remove the field entirely, and consequently this check too.
- Adam B
On July 30, 2015, 9:18 a.m., Kapil Arya wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/32587/
> -----------------------------------------------------------
>
> (Updated July 30, 2015, 9:18 a.m.)
>
>
> Review request for mesos, Adam B and Niklas Nielsen.
>
>
> Bugs: MESOS-2559
> https://issues.apache.org/jira/browse/MESOS-2559
>
>
> Repository: mesos
>
>
> Description
> -------
>
> FrameworkID can be retrieved from RunTaskMessage.framework.
>
> NOTE: This patch is only to be merged _ONLY_ after all the dependent patches have shipped, i.e. after 0.23.0 (tracked here: https://issues.apache.org/jira/browse/MESOS-2561) has released.
>
>
> Diffs
> -----
>
> src/master/master.cpp a8a195df07b5a97fdba7dfc5f312bbfa85a0d510
> src/slave/slave.cpp f91fa9204cd89596a3690c55c22e93429392cbfd
> src/tests/mesos.cpp f3b731542f9db4f966970ecb2bb96eb828350dea
>
> Diff: https://reviews.apache.org/r/32587/diff/
>
>
> Testing
> -------
>
> make check
>
> Also ran Nik's test-upgrade.py script (https://reviews.apache.org/r/31645) with 0.23.0 and the current master to verify compatibility checks.
>
>
> Thanks,
>
> Kapil Arya
>
>
Re: Review Request 32587: Stopped using RunTaskMessage.framework_id.
Posted by Kapil Arya <ka...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/32587/
-----------------------------------------------------------
(Updated July 30, 2015, 12:54 p.m.)
Review request for mesos, Adam B and Niklas Nielsen.
Changes
-------
removed trailing space.
Bugs: MESOS-2559
https://issues.apache.org/jira/browse/MESOS-2559
Repository: mesos
Description
-------
FrameworkID can be retrieved from RunTaskMessage.framework.
NOTE: This patch is only to be merged _ONLY_ after all the dependent patches have shipped, i.e. after 0.23.0 (tracked here: https://issues.apache.org/jira/browse/MESOS-2561) has released.
Diffs (updated)
-----
src/master/master.cpp a8a195df07b5a97fdba7dfc5f312bbfa85a0d510
src/slave/slave.cpp f91fa9204cd89596a3690c55c22e93429392cbfd
src/tests/mesos.cpp f3b731542f9db4f966970ecb2bb96eb828350dea
Diff: https://reviews.apache.org/r/32587/diff/
Testing
-------
make check
Also ran Nik's test-upgrade.py script (https://reviews.apache.org/r/31645) with 0.23.0 and the current master to verify compatibility checks.
Thanks,
Kapil Arya
Re: Review Request 32587: Stopped using RunTaskMessage.framework_id.
Posted by Kapil Arya <ka...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/32587/
-----------------------------------------------------------
(Updated July 30, 2015, 12:18 p.m.)
Review request for mesos, Adam B and Niklas Nielsen.
Changes
-------
Addressed Adam's comments.
Bugs: MESOS-2559
https://issues.apache.org/jira/browse/MESOS-2559
Repository: mesos
Description
-------
FrameworkID can be retrieved from RunTaskMessage.framework.
NOTE: This patch is only to be merged _ONLY_ after all the dependent patches have shipped, i.e. after 0.23.0 (tracked here: https://issues.apache.org/jira/browse/MESOS-2561) has released.
Diffs (updated)
-----
src/master/master.cpp a8a195df07b5a97fdba7dfc5f312bbfa85a0d510
src/slave/slave.cpp f91fa9204cd89596a3690c55c22e93429392cbfd
src/tests/mesos.cpp f3b731542f9db4f966970ecb2bb96eb828350dea
Diff: https://reviews.apache.org/r/32587/diff/
Testing
-------
make check
Also ran Nik's test-upgrade.py script (https://reviews.apache.org/r/31645) with 0.23.0 and the current master to verify compatibility checks.
Thanks,
Kapil Arya
Re: Review Request 32587: Stopped using RunTaskMessage.framework_id.
Posted by Kapil Arya <ka...@mesosphere.io>.
> On July 30, 2015, 5:03 a.m., Adam B wrote:
> > src/slave/slave.cpp, line 1240
> > <https://reviews.apache.org/r/32587/diff/5/?file=1023551#file1023551line1240>
> >
> > Maybe not a CHECK, since that would kill the slave. How about just logging an error and, if you're feeling generous, maybe sending back TASK_LOST?
A TASK_LOST message would require a framework-id, so just ignoring the runTask message and logging an error instead.
- Kapil
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/32587/#review93560
-----------------------------------------------------------
On July 30, 2015, 12:18 p.m., Kapil Arya wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/32587/
> -----------------------------------------------------------
>
> (Updated July 30, 2015, 12:18 p.m.)
>
>
> Review request for mesos, Adam B and Niklas Nielsen.
>
>
> Bugs: MESOS-2559
> https://issues.apache.org/jira/browse/MESOS-2559
>
>
> Repository: mesos
>
>
> Description
> -------
>
> FrameworkID can be retrieved from RunTaskMessage.framework.
>
> NOTE: This patch is only to be merged _ONLY_ after all the dependent patches have shipped, i.e. after 0.23.0 (tracked here: https://issues.apache.org/jira/browse/MESOS-2561) has released.
>
>
> Diffs
> -----
>
> src/master/master.cpp a8a195df07b5a97fdba7dfc5f312bbfa85a0d510
> src/slave/slave.cpp f91fa9204cd89596a3690c55c22e93429392cbfd
> src/tests/mesos.cpp f3b731542f9db4f966970ecb2bb96eb828350dea
>
> Diff: https://reviews.apache.org/r/32587/diff/
>
>
> Testing
> -------
>
> make check
>
> Also ran Nik's test-upgrade.py script (https://reviews.apache.org/r/31645) with 0.23.0 and the current master to verify compatibility checks.
>
>
> Thanks,
>
> Kapil Arya
>
>
Re: Review Request 32587: Stopped using RunTaskMessage.framework_id.
Posted by Adam B <ad...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/32587/#review93560
-----------------------------------------------------------
Ship it!
LGTM, except for the scariness of the CHECK
src/slave/slave.cpp (line 1240)
<https://reviews.apache.org/r/32587/#comment147935>
Maybe not a CHECK, since that would kill the slave. How about just logging an error and, if you're feeling generous, maybe sending back TASK_LOST?
- Adam B
On July 28, 2015, 7:49 a.m., Kapil Arya wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/32587/
> -----------------------------------------------------------
>
> (Updated July 28, 2015, 7:49 a.m.)
>
>
> Review request for mesos, Adam B and Niklas Nielsen.
>
>
> Bugs: MESOS-2559
> https://issues.apache.org/jira/browse/MESOS-2559
>
>
> Repository: mesos
>
>
> Description
> -------
>
> FrameworkID can be retrieved from RunTaskMessage.framework.
>
> NOTE: This patch is only to be merged _ONLY_ after all the dependent patches have shipped, i.e. after 0.23.0 (tracked here: https://issues.apache.org/jira/browse/MESOS-2561) has released.
>
>
> Diffs
> -----
>
> src/master/master.cpp a8a195df07b5a97fdba7dfc5f312bbfa85a0d510
> src/slave/slave.cpp f91fa9204cd89596a3690c55c22e93429392cbfd
> src/tests/mesos.cpp f3b731542f9db4f966970ecb2bb96eb828350dea
>
> Diff: https://reviews.apache.org/r/32587/diff/
>
>
> Testing
> -------
>
> make check
>
> Also ran Nik's test-upgrade.py script (https://reviews.apache.org/r/31645) with 0.23.0 and the current master to verify compatibility checks.
>
>
> Thanks,
>
> Kapil Arya
>
>
Re: Review Request 32587: Stopped using RunTaskMessage.framework_id.
Posted by Kapil Arya <ka...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/32587/
-----------------------------------------------------------
(Updated July 28, 2015, 10:49 a.m.)
Review request for mesos, Adam B and Niklas Nielsen.
Bugs: MESOS-2559
https://issues.apache.org/jira/browse/MESOS-2559
Repository: mesos
Description
-------
FrameworkID can be retrieved from RunTaskMessage.framework.
NOTE: This patch is only to be merged _ONLY_ after all the dependent patches have shipped, i.e. after 0.23.0 (tracked here: https://issues.apache.org/jira/browse/MESOS-2561) has released.
Diffs
-----
src/master/master.cpp a8a195df07b5a97fdba7dfc5f312bbfa85a0d510
src/slave/slave.cpp f91fa9204cd89596a3690c55c22e93429392cbfd
src/tests/mesos.cpp f3b731542f9db4f966970ecb2bb96eb828350dea
Diff: https://reviews.apache.org/r/32587/diff/
Testing
-------
make check
Also ran Nik's test-upgrade.py script (https://reviews.apache.org/r/31645) with 0.23.0 and the current master to verify compatibility checks.
Thanks,
Kapil Arya
Re: Review Request 32587: Stopped using RunTaskMessage.framework_id.
Posted by Kapil Arya <ka...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/32587/
-----------------------------------------------------------
(Updated July 28, 2015, 10:49 a.m.)
Review request for mesos, Adam B and Niklas Nielsen.
Changes
-------
rebased
Bugs: MESOS-2559
https://issues.apache.org/jira/browse/MESOS-2559
Repository: mesos
Description
-------
FrameworkID can be retrieved from RunTaskMessage.framework.
NOTE: This patch is only to be merged _ONLY_ after all the dependent patches have shipped, i.e. after 0.23.0 (tracked here: https://issues.apache.org/jira/browse/MESOS-2561) has released.
Diffs (updated)
-----
src/master/master.cpp a8a195df07b5a97fdba7dfc5f312bbfa85a0d510
src/slave/slave.cpp f91fa9204cd89596a3690c55c22e93429392cbfd
src/tests/mesos.cpp f3b731542f9db4f966970ecb2bb96eb828350dea
Diff: https://reviews.apache.org/r/32587/diff/
Testing (updated)
-------
make check
Also ran Nik's test-upgrade.py script (https://reviews.apache.org/r/31645) with 0.23.0 and the current master to verify compatibility checks.
Thanks,
Kapil Arya