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/04/23 19:46:07 UTC
Review Request: Fixed the master to shutdown deactivated slaves that send
further messages to the master.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/10734/
-----------------------------------------------------------
Review request for mesos, Benjamin Hindman and Vinod Kone.
Description
-------
See MESOS-446.
This addresses bug MESOS-446.
https://issues.apache.org/jira/browse/MESOS-446
Diffs
-----
src/master/master.hpp 9776a7cb8448e41e5d52288e3c637737cee15a08
src/master/master.cpp c3b26b136a529eee34e9cdf9700176c232f6e436
src/tests/fault_tolerance_tests.cpp 0348f20a8f4333f7d2f3786c33e55713cbcbcbe0
Diff: https://reviews.apache.org/r/10734/diff/
Testing
-------
Added a test.
Thanks,
Ben Mahler
Re: Review Request: Fixed the master to shutdown deactivated slaves that
send further messages to the master.
Posted by Benjamin Hindman <be...@berkeley.edu>.
> On April 29, 2013, 11 p.m., Benjamin Hindman wrote:
> > src/master/master.cpp, line 1031
> > <https://reviews.apache.org/r/10734/diff/3/?file=284531#file284531line1031>
> >
> > This check seems very redundant.
>
> Ben Mahler wrote:
> This checks that the maps are kept in sync, that is, slaves and deactivatedSlaves are disjoint. I'd like to keep this until we move this into the registrar you've been working on. Sound good?
SGTM.
- Benjamin
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/10734/#review19911
-----------------------------------------------------------
On April 25, 2013, 9:38 p.m., Ben Mahler wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/10734/
> -----------------------------------------------------------
>
> (Updated April 25, 2013, 9:38 p.m.)
>
>
> Review request for mesos, Benjamin Hindman and Vinod Kone.
>
>
> Description
> -------
>
> See MESOS-446.
>
>
> This addresses bug MESOS-446.
> https://issues.apache.org/jira/browse/MESOS-446
>
>
> Diffs
> -----
>
> src/master/master.hpp 9776a7cb8448e41e5d52288e3c637737cee15a08
> src/master/master.cpp c3b26b136a529eee34e9cdf9700176c232f6e436
> src/tests/fault_tolerance_tests.cpp d3476f7f5d1b7e2ed45385f5145eaccd6c114d21
>
> Diff: https://reviews.apache.org/r/10734/diff/
>
>
> Testing
> -------
>
> Added a test.
>
>
> Thanks,
>
> Ben Mahler
>
>
Re: Review Request: Fixed the master to shutdown deactivated slaves that
send further messages to the master.
Posted by Ben Mahler <be...@gmail.com>.
> On April 29, 2013, 11 p.m., Benjamin Hindman wrote:
> > src/master/master.cpp, line 1031
> > <https://reviews.apache.org/r/10734/diff/3/?file=284531#file284531line1031>
> >
> > This check seems very redundant.
This checks that the maps are kept in sync, that is, slaves and deactivatedSlaves are disjoint. I'd like to keep this until we move this into the registrar you've been working on. Sound good?
- Ben
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/10734/#review19911
-----------------------------------------------------------
On April 25, 2013, 9:38 p.m., Ben Mahler wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/10734/
> -----------------------------------------------------------
>
> (Updated April 25, 2013, 9:38 p.m.)
>
>
> Review request for mesos, Benjamin Hindman and Vinod Kone.
>
>
> Description
> -------
>
> See MESOS-446.
>
>
> This addresses bug MESOS-446.
> https://issues.apache.org/jira/browse/MESOS-446
>
>
> Diffs
> -----
>
> src/master/master.hpp 9776a7cb8448e41e5d52288e3c637737cee15a08
> src/master/master.cpp c3b26b136a529eee34e9cdf9700176c232f6e436
> src/tests/fault_tolerance_tests.cpp d3476f7f5d1b7e2ed45385f5145eaccd6c114d21
>
> Diff: https://reviews.apache.org/r/10734/diff/
>
>
> Testing
> -------
>
> Added a test.
>
>
> Thanks,
>
> Ben Mahler
>
>
Re: Review Request: Fixed the master to shutdown deactivated slaves that
send further messages to the master.
Posted by Benjamin Hindman <be...@berkeley.edu>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/10734/#review19911
-----------------------------------------------------------
Ship it!
src/master/master.cpp
<https://reviews.apache.org/r/10734/#comment41074>
This check seems very redundant.
- Benjamin Hindman
On April 25, 2013, 9:38 p.m., Ben Mahler wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/10734/
> -----------------------------------------------------------
>
> (Updated April 25, 2013, 9:38 p.m.)
>
>
> Review request for mesos, Benjamin Hindman and Vinod Kone.
>
>
> Description
> -------
>
> See MESOS-446.
>
>
> This addresses bug MESOS-446.
> https://issues.apache.org/jira/browse/MESOS-446
>
>
> Diffs
> -----
>
> src/master/master.hpp 9776a7cb8448e41e5d52288e3c637737cee15a08
> src/master/master.cpp c3b26b136a529eee34e9cdf9700176c232f6e436
> src/tests/fault_tolerance_tests.cpp d3476f7f5d1b7e2ed45385f5145eaccd6c114d21
>
> Diff: https://reviews.apache.org/r/10734/diff/
>
>
> Testing
> -------
>
> Added a test.
>
>
> Thanks,
>
> Ben Mahler
>
>
Re: Review Request: Fixed the master to shutdown deactivated slaves that
send further messages to the master.
Posted by Ben Mahler <be...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/10734/
-----------------------------------------------------------
(Updated April 30, 2013, 1:30 a.m.)
Review request for mesos, Benjamin Hindman and Vinod Kone.
Changes
-------
Rebased off trunk.
Description
-------
See MESOS-446.
This addresses bug MESOS-446.
https://issues.apache.org/jira/browse/MESOS-446
Diffs (updated)
-----
src/master/master.hpp 4a8aaee5a9970c0dd5cb022f04e48fb308241e20
src/master/master.cpp ff2f9546b3e5c885da0a5986606beaca57ba4d5c
src/tests/fault_tolerance_tests.cpp 70e2d558af72cc267240042577cf9f0fbfebe6d6
Diff: https://reviews.apache.org/r/10734/diff/
Testing
-------
Added a test.
Thanks,
Ben Mahler
Re: Review Request: Fixed the master to shutdown deactivated slaves that
send further messages to the master.
Posted by Ben Mahler <be...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/10734/
-----------------------------------------------------------
(Updated April 25, 2013, 9:38 p.m.)
Review request for mesos, Benjamin Hindman and Vinod Kone.
Changes
-------
Vinod's comments.
Description
-------
See MESOS-446.
This addresses bug MESOS-446.
https://issues.apache.org/jira/browse/MESOS-446
Diffs (updated)
-----
src/master/master.hpp 9776a7cb8448e41e5d52288e3c637737cee15a08
src/master/master.cpp c3b26b136a529eee34e9cdf9700176c232f6e436
src/tests/fault_tolerance_tests.cpp d3476f7f5d1b7e2ed45385f5145eaccd6c114d21
Diff: https://reviews.apache.org/r/10734/diff/
Testing
-------
Added a test.
Thanks,
Ben Mahler
Re: Review Request: Fixed the master to shutdown deactivated slaves that
send further messages to the master.
Posted by Ben Mahler <be...@gmail.com>.
> On April 25, 2013, 7:23 p.m., Vinod Kone wrote:
> > src/tests/fault_tolerance_tests.cpp, lines 594-598
> > <https://reviews.apache.org/r/10734/diff/1-2/?file=283422#file283422line594>
> >
> > Should these be re-ordered?
Why? The shutdown message to the slave is sent first.
> On April 25, 2013, 7:23 p.m., Vinod Kone wrote:
> > src/tests/fault_tolerance_tests.cpp, lines 486-492
> > <https://reviews.apache.org/r/10734/diff/1-2/?file=283422#file283422line486>
> >
> > I wonder if we should move part of this excellent comment up into the master where we actually send shutdowns. Seems more useful to have it up there?
> >
> > Ditto, for the above test comment.
Both locations have this comment currently:
// If the slave is deactivated, we have already informed
// frameworks that its tasks were LOST, so the slave should
// shut down.
Seems sufficient to me, no? Did you want the network partition scenario explained there as an example possibility?
- Ben
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/10734/#review19714
-----------------------------------------------------------
On April 25, 2013, 1:10 a.m., Ben Mahler wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/10734/
> -----------------------------------------------------------
>
> (Updated April 25, 2013, 1:10 a.m.)
>
>
> Review request for mesos, Benjamin Hindman and Vinod Kone.
>
>
> Description
> -------
>
> See MESOS-446.
>
>
> This addresses bug MESOS-446.
> https://issues.apache.org/jira/browse/MESOS-446
>
>
> Diffs
> -----
>
> src/master/master.hpp 9776a7cb8448e41e5d52288e3c637737cee15a08
> src/master/master.cpp c3b26b136a529eee34e9cdf9700176c232f6e436
> src/tests/fault_tolerance_tests.cpp d3476f7f5d1b7e2ed45385f5145eaccd6c114d21
>
> Diff: https://reviews.apache.org/r/10734/diff/
>
>
> Testing
> -------
>
> Added a test.
>
>
> Thanks,
>
> Ben Mahler
>
>
Re: Review Request: Fixed the master to shutdown deactivated slaves that
send further messages to the master.
Posted by Vinod Kone <vi...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/10734/#review19714
-----------------------------------------------------------
Ship it!
src/master/master.cpp
<https://reviews.apache.org/r/10734/#comment40657>
Can you add a comment here on why you can't use 'from' here?
src/master/master.cpp
<https://reviews.apache.org/r/10734/#comment40654>
s/Status/Ignoring status/
src/tests/fault_tolerance_tests.cpp
<https://reviews.apache.org/r/10734/#comment40659>
I wonder if we should move part of this excellent comment up into the master where we actually send shutdowns. Seems more useful to have it up there?
Ditto, for the above test comment.
src/tests/fault_tolerance_tests.cpp
<https://reviews.apache.org/r/10734/#comment40660>
s/, allow/ and allow/ ?
src/tests/fault_tolerance_tests.cpp
<https://reviews.apache.org/r/10734/#comment40661>
Should these be re-ordered?
- Vinod Kone
On April 25, 2013, 1:10 a.m., Ben Mahler wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/10734/
> -----------------------------------------------------------
>
> (Updated April 25, 2013, 1:10 a.m.)
>
>
> Review request for mesos, Benjamin Hindman and Vinod Kone.
>
>
> Description
> -------
>
> See MESOS-446.
>
>
> This addresses bug MESOS-446.
> https://issues.apache.org/jira/browse/MESOS-446
>
>
> Diffs
> -----
>
> src/master/master.hpp 9776a7cb8448e41e5d52288e3c637737cee15a08
> src/master/master.cpp c3b26b136a529eee34e9cdf9700176c232f6e436
> src/tests/fault_tolerance_tests.cpp d3476f7f5d1b7e2ed45385f5145eaccd6c114d21
>
> Diff: https://reviews.apache.org/r/10734/diff/
>
>
> Testing
> -------
>
> Added a test.
>
>
> Thanks,
>
> Ben Mahler
>
>
Re: Review Request: Fixed the master to shutdown deactivated slaves that
send further messages to the master.
Posted by Ben Mahler <be...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/10734/
-----------------------------------------------------------
(Updated April 25, 2013, 1:10 a.m.)
Review request for mesos, Benjamin Hindman and Vinod Kone.
Changes
-------
Rebased and added a test for ExitedExecutorMessage as well.
Description
-------
See MESOS-446.
This addresses bug MESOS-446.
https://issues.apache.org/jira/browse/MESOS-446
Diffs (updated)
-----
src/master/master.hpp 9776a7cb8448e41e5d52288e3c637737cee15a08
src/master/master.cpp c3b26b136a529eee34e9cdf9700176c232f6e436
src/tests/fault_tolerance_tests.cpp d3476f7f5d1b7e2ed45385f5145eaccd6c114d21
Diff: https://reviews.apache.org/r/10734/diff/
Testing
-------
Added a test.
Thanks,
Ben Mahler
Re: Review Request: Fixed the master to shutdown deactivated slaves that
send further messages to the master.
Posted by Ben Mahler <be...@gmail.com>.
> On April 23, 2013, 7:52 p.m., Vinod Kone wrote:
> > src/master/master.cpp, line 1012
> > <https://reviews.apache.org/r/10734/diff/1/?file=283421#file283421line1012>
> >
> > Why is this inside if (slave == NULL). Is the invariant that a slave in deactivatedSlaves cannot be slaves? If yes, we need to CHECK that.
> >
> >
Good call.
> On April 23, 2013, 7:52 p.m., Vinod Kone wrote:
> > src/tests/fault_tolerance_tests.cpp, lines 459-471
> > <https://reviews.apache.org/r/10734/diff/1/?file=283422#file283422line459>
> >
> > Actually, instead of simulating an update why the new "post" method, could you launch a task and just keep dropping status updates from master to scheduler.
> >
> > That way the slave keeps retrying the update even after master sends the first shutdown and eventually gets the shutdown after deactivation.
> >
> > Also, then you can drop the previous review :)
Worked with Vinod to come up with a better way to test this with a launched task, and we were unable to come up with a clean way to do so due to the status update manager's retried updates making the expectations complicated.
- Ben
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/10734/#review19597
-----------------------------------------------------------
On April 23, 2013, 5:46 p.m., Ben Mahler wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/10734/
> -----------------------------------------------------------
>
> (Updated April 23, 2013, 5:46 p.m.)
>
>
> Review request for mesos, Benjamin Hindman and Vinod Kone.
>
>
> Description
> -------
>
> See MESOS-446.
>
>
> This addresses bug MESOS-446.
> https://issues.apache.org/jira/browse/MESOS-446
>
>
> Diffs
> -----
>
> src/master/master.hpp 9776a7cb8448e41e5d52288e3c637737cee15a08
> src/master/master.cpp c3b26b136a529eee34e9cdf9700176c232f6e436
> src/tests/fault_tolerance_tests.cpp 0348f20a8f4333f7d2f3786c33e55713cbcbcbe0
>
> Diff: https://reviews.apache.org/r/10734/diff/
>
>
> Testing
> -------
>
> Added a test.
>
>
> Thanks,
>
> Ben Mahler
>
>
Re: Review Request: Fixed the master to shutdown deactivated slaves that
send further messages to the master.
Posted by Vinod Kone <vi...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/10734/#review19597
-----------------------------------------------------------
src/master/master.cpp
<https://reviews.apache.org/r/10734/#comment40493>
Why is this inside if (slave == NULL). Is the invariant that a slave in deactivatedSlaves cannot be slaves? If yes, we need to CHECK that.
src/master/master.cpp
<https://reviews.apache.org/r/10734/#comment40495>
s/Status update/Ignoring status update/
src/master/master.cpp
<https://reviews.apache.org/r/10734/#comment40494>
ditto
src/master/master.cpp
<https://reviews.apache.org/r/10734/#comment40496>
quotes around executor?
src/master/master.cpp
<https://reviews.apache.org/r/10734/#comment40497>
ditto
src/tests/fault_tolerance_tests.cpp
<https://reviews.apache.org/r/10734/#comment40498>
These could be pulled down to after master start?
src/tests/fault_tolerance_tests.cpp
<https://reviews.apache.org/r/10734/#comment40499>
Pull this down to after master start
src/tests/fault_tolerance_tests.cpp
<https://reviews.apache.org/r/10734/#comment40500>
Actually, instead of simulating an update why the new "post" method, could you launch a task and just keep dropping status updates from master to scheduler.
That way the slave keeps retrying the update even after master sends the first shutdown and eventually gets the shutdown after deactivation.
Also, then you can drop the previous review :)
- Vinod Kone
On April 23, 2013, 5:46 p.m., Ben Mahler wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/10734/
> -----------------------------------------------------------
>
> (Updated April 23, 2013, 5:46 p.m.)
>
>
> Review request for mesos, Benjamin Hindman and Vinod Kone.
>
>
> Description
> -------
>
> See MESOS-446.
>
>
> This addresses bug MESOS-446.
> https://issues.apache.org/jira/browse/MESOS-446
>
>
> Diffs
> -----
>
> src/master/master.hpp 9776a7cb8448e41e5d52288e3c637737cee15a08
> src/master/master.cpp c3b26b136a529eee34e9cdf9700176c232f6e436
> src/tests/fault_tolerance_tests.cpp 0348f20a8f4333f7d2f3786c33e55713cbcbcbe0
>
> Diff: https://reviews.apache.org/r/10734/diff/
>
>
> Testing
> -------
>
> Added a test.
>
>
> Thanks,
>
> Ben Mahler
>
>