You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@mesos.apache.org by Ben Mahler <be...@gmail.com> on 2013/12/05 09:45:27 UTC
Review Request 16038: Fixed an exception raised in the master related to
task statuses.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/16038/
-----------------------------------------------------------
Review request for mesos, Benjamin Hindman, Brenden Matthews, and Vinod Kone.
Repository: mesos-git
Description
-------
This was discovered in 0.16.0-rc1, https://reviews.apache.org/r/14434/ causes the master to raise an exception when there are no statuses available (pre-0.16.0 slave will not have any statuses when it re-registers with the master).
I'm not convinced this is the only case where this can occur, since we do not always insert a status in Tasks, so I'm making this condition explicit to prevent this exception. We should take up a subsequent change to ensure that a status is always inserted for each state (we don't do this for STAGING in some places).
Diffs
-----
src/master/master.cpp 4f4db93a3c66d79e25783c7ea0617f5cf807c1aa
src/slave/slave.cpp 75d9e5dfce9f546fb528d9f0fcf8ba127ac40b9c
Diff: https://reviews.apache.org/r/16038/diff/
Testing
-------
make check (I did not add a test here because the known case that triggers this occurs as the result of upgrading into 0.16.0)
Thanks,
Ben Mahler
Re: Review Request 16038: Fixed an exception raised in the master related to
task statuses.
Posted by Brenden Matthews <br...@diddyinc.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/16038/#review29812
-----------------------------------------------------------
Ship it!
Good catch!
- Brenden Matthews
On Dec. 5, 2013, 8:45 a.m., Ben Mahler wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/16038/
> -----------------------------------------------------------
>
> (Updated Dec. 5, 2013, 8:45 a.m.)
>
>
> Review request for mesos, Benjamin Hindman, Brenden Matthews, and Vinod Kone.
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> This was discovered in 0.16.0-rc1, https://reviews.apache.org/r/14434/ causes the master to raise an exception when there are no statuses available (pre-0.16.0 slave will not have any statuses when it re-registers with the master).
>
> I'm not convinced this is the only case where this can occur, since we do not always insert a status in Tasks, so I'm making this condition explicit to prevent this exception. We should take up a subsequent change to ensure that a status is always inserted for each state (we don't do this for STAGING in some places).
>
>
> Diffs
> -----
>
> src/master/master.cpp 4f4db93a3c66d79e25783c7ea0617f5cf807c1aa
> src/slave/slave.cpp 75d9e5dfce9f546fb528d9f0fcf8ba127ac40b9c
>
> Diff: https://reviews.apache.org/r/16038/diff/
>
>
> Testing
> -------
>
> make check (I did not add a test here because the known case that triggers this occurs as the result of upgrading into 0.16.0)
>
>
> Thanks,
>
> Ben Mahler
>
>
Re: Review Request 16038: Fixed an exception raised in the master related to
task statuses.
Posted by Ben Mahler <be...@gmail.com>.
> On Dec. 11, 2013, 7:35 p.m., Brenden Matthews wrote:
> > Hey Ben,
> >
> > It looks like this might not quite work as desired. I've written a patch here:
> >
> > https://github.com/airbnb/mesos/commit/4c47a4eaa3316c892e4551c9ca7703c74aaa56a5
> >
> > I'm not sure if this was intended or not, however.
>
> Ben Mahler wrote:
> Ah thank you! This should be:
>
> if (task->statuses_size() > 0 &&
> - task->statuses(task->statuses_size() - 1).state() == task->state()) {
> + task->statuses(task->statuses_size() - 1).state() == status.state()) {
>
> I don't think we can currently compare task->state() and status.state() directly because of: https://issues.apache.org/jira/browse/MESOS-869
I'll send a fix
- Ben
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/16038/#review30217
-----------------------------------------------------------
On Dec. 5, 2013, 8:45 a.m., Ben Mahler wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/16038/
> -----------------------------------------------------------
>
> (Updated Dec. 5, 2013, 8:45 a.m.)
>
>
> Review request for mesos, Benjamin Hindman, Brenden Matthews, and Vinod Kone.
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> This was discovered in 0.16.0-rc1, https://reviews.apache.org/r/14434/ causes the master to raise an exception when there are no statuses available (pre-0.16.0 slave will not have any statuses when it re-registers with the master).
>
> I'm not convinced this is the only case where this can occur, since we do not always insert a status in Tasks, so I'm making this condition explicit to prevent this exception. We should take up a subsequent change to ensure that a status is always inserted for each state (we don't do this for STAGING in some places).
>
>
> Diffs
> -----
>
> src/master/master.cpp 4f4db93a3c66d79e25783c7ea0617f5cf807c1aa
> src/slave/slave.cpp 75d9e5dfce9f546fb528d9f0fcf8ba127ac40b9c
>
> Diff: https://reviews.apache.org/r/16038/diff/
>
>
> Testing
> -------
>
> make check (I did not add a test here because the known case that triggers this occurs as the result of upgrading into 0.16.0)
>
>
> Thanks,
>
> Ben Mahler
>
>
Re: Review Request 16038: Fixed an exception raised in the master related to
task statuses.
Posted by Ben Mahler <be...@gmail.com>.
> On Dec. 11, 2013, 7:35 p.m., Brenden Matthews wrote:
> > Hey Ben,
> >
> > It looks like this might not quite work as desired. I've written a patch here:
> >
> > https://github.com/airbnb/mesos/commit/4c47a4eaa3316c892e4551c9ca7703c74aaa56a5
> >
> > I'm not sure if this was intended or not, however.
Ah thank you! This should be:
if (task->statuses_size() > 0 &&
- task->statuses(task->statuses_size() - 1).state() == task->state()) {
+ task->statuses(task->statuses_size() - 1).state() == status.state()) {
I don't think we can currently compare task->state() and status.state() directly because of: https://issues.apache.org/jira/browse/MESOS-869
- Ben
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/16038/#review30217
-----------------------------------------------------------
On Dec. 5, 2013, 8:45 a.m., Ben Mahler wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/16038/
> -----------------------------------------------------------
>
> (Updated Dec. 5, 2013, 8:45 a.m.)
>
>
> Review request for mesos, Benjamin Hindman, Brenden Matthews, and Vinod Kone.
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> This was discovered in 0.16.0-rc1, https://reviews.apache.org/r/14434/ causes the master to raise an exception when there are no statuses available (pre-0.16.0 slave will not have any statuses when it re-registers with the master).
>
> I'm not convinced this is the only case where this can occur, since we do not always insert a status in Tasks, so I'm making this condition explicit to prevent this exception. We should take up a subsequent change to ensure that a status is always inserted for each state (we don't do this for STAGING in some places).
>
>
> Diffs
> -----
>
> src/master/master.cpp 4f4db93a3c66d79e25783c7ea0617f5cf807c1aa
> src/slave/slave.cpp 75d9e5dfce9f546fb528d9f0fcf8ba127ac40b9c
>
> Diff: https://reviews.apache.org/r/16038/diff/
>
>
> Testing
> -------
>
> make check (I did not add a test here because the known case that triggers this occurs as the result of upgrading into 0.16.0)
>
>
> Thanks,
>
> Ben Mahler
>
>
Re: Review Request 16038: Fixed an exception raised in the master related to
task statuses.
Posted by Ben Mahler <be...@gmail.com>.
> On Dec. 11, 2013, 7:35 p.m., Brenden Matthews wrote:
> > Hey Ben,
> >
> > It looks like this might not quite work as desired. I've written a patch here:
> >
> > https://github.com/airbnb/mesos/commit/4c47a4eaa3316c892e4551c9ca7703c74aaa56a5
> >
> > I'm not sure if this was intended or not, however.
>
> Ben Mahler wrote:
> Ah thank you! This should be:
>
> if (task->statuses_size() > 0 &&
> - task->statuses(task->statuses_size() - 1).state() == task->state()) {
> + task->statuses(task->statuses_size() - 1).state() == status.state()) {
>
> I don't think we can currently compare task->state() and status.state() directly because of: https://issues.apache.org/jira/browse/MESOS-869
>
> Ben Mahler wrote:
> I'll send a fix
>
> Brenden Matthews wrote:
> Ah yes, that makes sense. Do you want me to submit a new review, or will you?
https://reviews.apache.org/r/16188/
- Ben
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/16038/#review30217
-----------------------------------------------------------
On Dec. 5, 2013, 8:45 a.m., Ben Mahler wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/16038/
> -----------------------------------------------------------
>
> (Updated Dec. 5, 2013, 8:45 a.m.)
>
>
> Review request for mesos, Benjamin Hindman, Brenden Matthews, and Vinod Kone.
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> This was discovered in 0.16.0-rc1, https://reviews.apache.org/r/14434/ causes the master to raise an exception when there are no statuses available (pre-0.16.0 slave will not have any statuses when it re-registers with the master).
>
> I'm not convinced this is the only case where this can occur, since we do not always insert a status in Tasks, so I'm making this condition explicit to prevent this exception. We should take up a subsequent change to ensure that a status is always inserted for each state (we don't do this for STAGING in some places).
>
>
> Diffs
> -----
>
> src/master/master.cpp 4f4db93a3c66d79e25783c7ea0617f5cf807c1aa
> src/slave/slave.cpp 75d9e5dfce9f546fb528d9f0fcf8ba127ac40b9c
>
> Diff: https://reviews.apache.org/r/16038/diff/
>
>
> Testing
> -------
>
> make check (I did not add a test here because the known case that triggers this occurs as the result of upgrading into 0.16.0)
>
>
> Thanks,
>
> Ben Mahler
>
>
Re: Review Request 16038: Fixed an exception raised in the master related to
task statuses.
Posted by Brenden Matthews <br...@diddyinc.com>.
> On Dec. 11, 2013, 7:35 p.m., Brenden Matthews wrote:
> > Hey Ben,
> >
> > It looks like this might not quite work as desired. I've written a patch here:
> >
> > https://github.com/airbnb/mesos/commit/4c47a4eaa3316c892e4551c9ca7703c74aaa56a5
> >
> > I'm not sure if this was intended or not, however.
>
> Ben Mahler wrote:
> Ah thank you! This should be:
>
> if (task->statuses_size() > 0 &&
> - task->statuses(task->statuses_size() - 1).state() == task->state()) {
> + task->statuses(task->statuses_size() - 1).state() == status.state()) {
>
> I don't think we can currently compare task->state() and status.state() directly because of: https://issues.apache.org/jira/browse/MESOS-869
>
> Ben Mahler wrote:
> I'll send a fix
Ah yes, that makes sense. Do you want me to submit a new review, or will you?
- Brenden
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/16038/#review30217
-----------------------------------------------------------
On Dec. 5, 2013, 8:45 a.m., Ben Mahler wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/16038/
> -----------------------------------------------------------
>
> (Updated Dec. 5, 2013, 8:45 a.m.)
>
>
> Review request for mesos, Benjamin Hindman, Brenden Matthews, and Vinod Kone.
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> This was discovered in 0.16.0-rc1, https://reviews.apache.org/r/14434/ causes the master to raise an exception when there are no statuses available (pre-0.16.0 slave will not have any statuses when it re-registers with the master).
>
> I'm not convinced this is the only case where this can occur, since we do not always insert a status in Tasks, so I'm making this condition explicit to prevent this exception. We should take up a subsequent change to ensure that a status is always inserted for each state (we don't do this for STAGING in some places).
>
>
> Diffs
> -----
>
> src/master/master.cpp 4f4db93a3c66d79e25783c7ea0617f5cf807c1aa
> src/slave/slave.cpp 75d9e5dfce9f546fb528d9f0fcf8ba127ac40b9c
>
> Diff: https://reviews.apache.org/r/16038/diff/
>
>
> Testing
> -------
>
> make check (I did not add a test here because the known case that triggers this occurs as the result of upgrading into 0.16.0)
>
>
> Thanks,
>
> Ben Mahler
>
>
Re: Review Request 16038: Fixed an exception raised in the master related to
task statuses.
Posted by Brenden Matthews <br...@diddyinc.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/16038/#review30217
-----------------------------------------------------------
Hey Ben,
It looks like this might not quite work as desired. I've written a patch here:
https://github.com/airbnb/mesos/commit/4c47a4eaa3316c892e4551c9ca7703c74aaa56a5
I'm not sure if this was intended or not, however.
- Brenden Matthews
On Dec. 5, 2013, 8:45 a.m., Ben Mahler wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/16038/
> -----------------------------------------------------------
>
> (Updated Dec. 5, 2013, 8:45 a.m.)
>
>
> Review request for mesos, Benjamin Hindman, Brenden Matthews, and Vinod Kone.
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> This was discovered in 0.16.0-rc1, https://reviews.apache.org/r/14434/ causes the master to raise an exception when there are no statuses available (pre-0.16.0 slave will not have any statuses when it re-registers with the master).
>
> I'm not convinced this is the only case where this can occur, since we do not always insert a status in Tasks, so I'm making this condition explicit to prevent this exception. We should take up a subsequent change to ensure that a status is always inserted for each state (we don't do this for STAGING in some places).
>
>
> Diffs
> -----
>
> src/master/master.cpp 4f4db93a3c66d79e25783c7ea0617f5cf807c1aa
> src/slave/slave.cpp 75d9e5dfce9f546fb528d9f0fcf8ba127ac40b9c
>
> Diff: https://reviews.apache.org/r/16038/diff/
>
>
> Testing
> -------
>
> make check (I did not add a test here because the known case that triggers this occurs as the result of upgrading into 0.16.0)
>
>
> Thanks,
>
> Ben Mahler
>
>