You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@mesos.apache.org by Vinod Kone <vi...@gmail.com> on 2014/09/23 01:50:41 UTC
Re: Review Request 25866: Updated semantics of
disconnected/deactivated slaves/frameworks in master.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/25866/
-----------------------------------------------------------
(Updated Sept. 22, 2014, 11:50 p.m.)
Review request for mesos, Adam B and Ben Mahler.
Changes
-------
dominic's and adam's comments. PTAL.
Summary (updated)
-----------------
Updated semantics of disconnected/deactivated slaves/frameworks in master.
Bugs: MESOS-1081 and MESOS-1811
https://issues.apache.org/jira/browse/MESOS-1081
https://issues.apache.org/jira/browse/MESOS-1811
Repository: mesos-git
Description
-------
Made consistent what connected and active frameworks/slaves means.
Fixed MESOS-1811 along the way.
Diffs (updated)
-----
src/master/http.cpp 3f5a01dfddca9cea73563100d88e0c03f600d6b1
src/master/master.hpp f5d74aef185fad861139186be1cab089f8005a94
src/master/master.cpp e5d30e9c7ba1ec0cdd640c81610790f3397f3062
src/tests/fault_tolerance_tests.cpp 154386044d0247b39d84719d7ff14250682a0695
src/tests/master_tests.cpp 8e4ec1d85c4530b5421387de55036f7d40ee3180
Diff: https://reviews.apache.org/r/25866/diff/
Testing
-------
make check
Thanks,
Vinod Kone
Re: Review Request 25866: Updated semantics of
disconnected/deactivated slaves/frameworks in master.
Posted by Adam B <ad...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/25866/#review54215
-----------------------------------------------------------
Ship it!
Great job.
src/master/master.cpp
<https://reviews.apache.org/r/25866/#comment94218>
Only need to set connected=true once.
- Adam B
On Sept. 22, 2014, 4:50 p.m., Vinod Kone wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/25866/
> -----------------------------------------------------------
>
> (Updated Sept. 22, 2014, 4:50 p.m.)
>
>
> Review request for mesos, Adam B and Ben Mahler.
>
>
> Bugs: MESOS-1081 and MESOS-1811
> https://issues.apache.org/jira/browse/MESOS-1081
> https://issues.apache.org/jira/browse/MESOS-1811
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> Made consistent what connected and active frameworks/slaves means.
>
> Fixed MESOS-1811 along the way.
>
>
> Diffs
> -----
>
> src/master/http.cpp 3f5a01dfddca9cea73563100d88e0c03f600d6b1
> src/master/master.hpp f5d74aef185fad861139186be1cab089f8005a94
> src/master/master.cpp e5d30e9c7ba1ec0cdd640c81610790f3397f3062
> src/tests/fault_tolerance_tests.cpp 154386044d0247b39d84719d7ff14250682a0695
> src/tests/master_tests.cpp 8e4ec1d85c4530b5421387de55036f7d40ee3180
>
> Diff: https://reviews.apache.org/r/25866/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Vinod Kone
>
>
Re: Review Request 25866: Updated semantics of
disconnected/deactivated slaves/frameworks in master.
Posted by Vinod Kone <vi...@gmail.com>.
> On Sept. 24, 2014, 11:39 p.m., Ben Mahler wrote:
> > src/master/master.cpp, lines 743-744
> > <https://reviews.apache.org/r/25866/diff/3/?file=700961#file700961line743>
> >
> > Why do we check slave->connected before calling disconnect below, but we don't check framework->connected here?
> >
> > Probably warrants a comment.
this was done as part of MESOS-675 (by you :)). Added a comment.
> On Sept. 24, 2014, 11:39 p.m., Ben Mahler wrote:
> > src/master/master.cpp, lines 1653-1666
> > <https://reviews.apache.org/r/25866/diff/3/?file=700961#file700961line1653>
> >
> > Should this be below deactivateFramework, to keep disconnect and deactivate adjacent (like in the header file)?
done.
> On Sept. 24, 2014, 11:39 p.m., Ben Mahler wrote:
> > src/master/master.cpp, line 1655
> > <https://reviews.apache.org/r/25866/diff/3/?file=700961#file700961line1655>
> >
> > Re: question above
> >
> > Should this be idempotent? i.e. do nothing when already disconnected.
> >
> > Or do you want the caller to never call this when already disconnected?
yup, the latter, esp when calling methods that take pointers.
- Vinod
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/25866/#review54384
-----------------------------------------------------------
On Sept. 22, 2014, 11:50 p.m., Vinod Kone wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/25866/
> -----------------------------------------------------------
>
> (Updated Sept. 22, 2014, 11:50 p.m.)
>
>
> Review request for mesos, Adam B and Ben Mahler.
>
>
> Bugs: MESOS-1081 and MESOS-1811
> https://issues.apache.org/jira/browse/MESOS-1081
> https://issues.apache.org/jira/browse/MESOS-1811
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> Made consistent what connected and active frameworks/slaves means.
>
> Fixed MESOS-1811 along the way.
>
>
> Diffs
> -----
>
> src/master/http.cpp 3f5a01dfddca9cea73563100d88e0c03f600d6b1
> src/master/master.hpp f5d74aef185fad861139186be1cab089f8005a94
> src/master/master.cpp e5d30e9c7ba1ec0cdd640c81610790f3397f3062
> src/tests/fault_tolerance_tests.cpp 154386044d0247b39d84719d7ff14250682a0695
> src/tests/master_tests.cpp 8e4ec1d85c4530b5421387de55036f7d40ee3180
>
> Diff: https://reviews.apache.org/r/25866/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Vinod Kone
>
>
Re: Review Request 25866: Updated semantics of
disconnected/deactivated slaves/frameworks in master.
Posted by Ben Mahler <be...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/25866/#review54384
-----------------------------------------------------------
Ship it!
Phew that was a bit tricky to review because of the renaming + metrics changes sneaking in here. Split it next time to help me out? :)
src/master/master.hpp
<https://reviews.apache.org/r/25866/#comment94520>
Hm.. seems a bit odd to describe the false case explicitly. How about we phrase this a bit differently?
// Slaves become disconnected when the socket closes.
// No offers are sent for inactive slaves. Currently, ...
Does it make sense to comment these together, so that we can explain that disconnected slaves are always inactive?
Ditto below for Framework.
src/master/master.cpp
<https://reviews.apache.org/r/25866/#comment94617>
Why do we check slave->connected before calling disconnect below, but we don't check framework->connected here?
Probably warrants a comment.
src/master/master.cpp
<https://reviews.apache.org/r/25866/#comment94621>
This can't be moved down to be alongside the framework->active check?
Ditto below.
src/master/master.cpp
<https://reviews.apache.org/r/25866/#comment94615>
Should this be below deactivateFramework, to keep disconnect and deactivate adjacent (like in the header file)?
src/master/master.cpp
<https://reviews.apache.org/r/25866/#comment94619>
Re: question above
Should this be idempotent? i.e. do nothing when already disconnected.
Or do you want the caller to never call this when already disconnected?
src/master/master.cpp
<https://reviews.apache.org/r/25866/#comment94623>
What about saying that we don't expect a slave to be connected and deactivated?
Ditto below.
src/master/master.cpp
<https://reviews.apache.org/r/25866/#comment94620>
Looks like connected = true doesn't need to be done before removing offers, correct?
- Ben Mahler
On Sept. 22, 2014, 11:50 p.m., Vinod Kone wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/25866/
> -----------------------------------------------------------
>
> (Updated Sept. 22, 2014, 11:50 p.m.)
>
>
> Review request for mesos, Adam B and Ben Mahler.
>
>
> Bugs: MESOS-1081 and MESOS-1811
> https://issues.apache.org/jira/browse/MESOS-1081
> https://issues.apache.org/jira/browse/MESOS-1811
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> Made consistent what connected and active frameworks/slaves means.
>
> Fixed MESOS-1811 along the way.
>
>
> Diffs
> -----
>
> src/master/http.cpp 3f5a01dfddca9cea73563100d88e0c03f600d6b1
> src/master/master.hpp f5d74aef185fad861139186be1cab089f8005a94
> src/master/master.cpp e5d30e9c7ba1ec0cdd640c81610790f3397f3062
> src/tests/fault_tolerance_tests.cpp 154386044d0247b39d84719d7ff14250682a0695
> src/tests/master_tests.cpp 8e4ec1d85c4530b5421387de55036f7d40ee3180
>
> Diff: https://reviews.apache.org/r/25866/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Vinod Kone
>
>
Re: Review Request 25866: Updated semantics of
disconnected/deactivated slaves/frameworks in master.
Posted by Dominic Hamon <dh...@twopensource.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/25866/#review54278
-----------------------------------------------------------
src/master/master.cpp
<https://reviews.apache.org/r/25866/#comment94282>
duplicate of 3977?
- Dominic Hamon
On Sept. 22, 2014, 4:50 p.m., Vinod Kone wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/25866/
> -----------------------------------------------------------
>
> (Updated Sept. 22, 2014, 4:50 p.m.)
>
>
> Review request for mesos, Adam B and Ben Mahler.
>
>
> Bugs: MESOS-1081 and MESOS-1811
> https://issues.apache.org/jira/browse/MESOS-1081
> https://issues.apache.org/jira/browse/MESOS-1811
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> Made consistent what connected and active frameworks/slaves means.
>
> Fixed MESOS-1811 along the way.
>
>
> Diffs
> -----
>
> src/master/http.cpp 3f5a01dfddca9cea73563100d88e0c03f600d6b1
> src/master/master.hpp f5d74aef185fad861139186be1cab089f8005a94
> src/master/master.cpp e5d30e9c7ba1ec0cdd640c81610790f3397f3062
> src/tests/fault_tolerance_tests.cpp 154386044d0247b39d84719d7ff14250682a0695
> src/tests/master_tests.cpp 8e4ec1d85c4530b5421387de55036f7d40ee3180
>
> Diff: https://reviews.apache.org/r/25866/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Vinod Kone
>
>
Re: Review Request 25866: Updated semantics of
disconnected/deactivated slaves/frameworks in master.
Posted by Vinod Kone <vi...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/25866/
-----------------------------------------------------------
(Updated Sept. 25, 2014, 8:37 p.m.)
Review request for mesos, Adam B and Ben Mahler.
Changes
-------
Addressed comments. NNFR.
Bugs: MESOS-1081 and MESOS-1811
https://issues.apache.org/jira/browse/MESOS-1081
https://issues.apache.org/jira/browse/MESOS-1811
Repository: mesos-git
Description
-------
Made consistent what connected and active frameworks/slaves means.
Fixed MESOS-1811 along the way.
Diffs (updated)
-----
src/master/http.cpp 3f5a01dfddca9cea73563100d88e0c03f600d6b1
src/master/master.hpp f5d74aef185fad861139186be1cab089f8005a94
src/master/master.cpp e5d30e9c7ba1ec0cdd640c81610790f3397f3062
src/tests/fault_tolerance_tests.cpp 154386044d0247b39d84719d7ff14250682a0695
src/tests/master_tests.cpp 8e4ec1d85c4530b5421387de55036f7d40ee3180
Diff: https://reviews.apache.org/r/25866/diff/
Testing
-------
make check
Thanks,
Vinod Kone