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 2013/08/01 07:30:57 UTC
Review Request 13187: Fixed slave to properly handle terminated tasks that
have pending updates.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/13187/
-----------------------------------------------------------
Review request for mesos, Benjamin Hindman and Ben Mahler.
Bugs: MESOS-606
https://issues.apache.org/jira/browse/MESOS-606
Repository: mesos-git
Description
-------
Changed the semantics of StatusUpdateStream::acknowedgement()'s return value.
Instead of informing about duplicate, it now informs whether the status update stream is terminated or not. This is definitely more powerful/useful information for slave to have. Duplicates are now considered Errors.
Diffs
-----
src/slave/slave.hpp 97f2083db0dec2c1c9415f65d9bf8a834653bd18
src/slave/slave.cpp 7f6e6b456890db438092f19a22e4dd816bb33d04
src/slave/status_update_manager.hpp 795e74c2b88a071eb7ba720118e06077b6e41238
src/slave/status_update_manager.cpp 9e9e4e2a47a609d65ed69a57de595852144a86c8
src/tests/slave_recovery_tests.cpp 1871e3ba41e65dcbd4b95779dda068f6a1a2ecb3
src/tests/status_update_manager_tests.cpp 42395324dfe49659bee2229c6573ffef0874d923
Diff: https://reviews.apache.org/r/13187/diff/
Testing
-------
make check
Thanks,
Vinod Kone
Re: Review Request 13187: Fixed slave to properly handle terminated tasks
that have pending updates.
Posted by Benjamin Hindman <be...@berkeley.edu>.
> On Aug. 1, 2013, 6:48 a.m., Benjamin Hindman wrote:
> > src/slave/status_update_manager.hpp, line 110
> > <https://reviews.apache.org/r/13187/diff/1/?file=332292#file332292line110>
> >
> > Any reason not to flatten these: Future<Try<bool>> => Future<bool>?
>
> Vinod Kone wrote:
> good call. i will flatten all the status update manager functions. will do that in a subsequent review though. sg?
SGTM!
- Benjamin
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/13187/#review24419
-----------------------------------------------------------
On Aug. 1, 2013, 5:30 a.m., Vinod Kone wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/13187/
> -----------------------------------------------------------
>
> (Updated Aug. 1, 2013, 5:30 a.m.)
>
>
> Review request for mesos, Benjamin Hindman and Ben Mahler.
>
>
> Bugs: MESOS-606
> https://issues.apache.org/jira/browse/MESOS-606
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> Changed the semantics of StatusUpdateStream::acknowedgement()'s return value.
>
> Instead of informing about duplicate, it now informs whether the status update stream is terminated or not. This is definitely more powerful/useful information for slave to have. Duplicates are now considered Errors.
>
>
> Diffs
> -----
>
> src/slave/slave.hpp 97f2083db0dec2c1c9415f65d9bf8a834653bd18
> src/slave/slave.cpp 7f6e6b456890db438092f19a22e4dd816bb33d04
> src/slave/status_update_manager.hpp 795e74c2b88a071eb7ba720118e06077b6e41238
> src/slave/status_update_manager.cpp 9e9e4e2a47a609d65ed69a57de595852144a86c8
> src/tests/slave_recovery_tests.cpp 1871e3ba41e65dcbd4b95779dda068f6a1a2ecb3
> src/tests/status_update_manager_tests.cpp 42395324dfe49659bee2229c6573ffef0874d923
>
> Diff: https://reviews.apache.org/r/13187/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Vinod Kone
>
>
Re: Review Request 13187: Fixed slave to properly handle terminated tasks
that have pending updates.
Posted by Vinod Kone <vi...@gmail.com>.
> On Aug. 1, 2013, 6:48 a.m., Benjamin Hindman wrote:
> > src/slave/slave.cpp, line 865
> > <https://reviews.apache.org/r/13187/diff/1/?file=332291#file332291line865>
> >
> > Remove framework framework. ;) Why the change?
The reason I initially changed it was because I was writing a test where I wanted to catch a dispatch on it. Since 'remove' was an overloaded function FUTURE_DISPATCH(_, &Slave::remove) complained. I resolved it by calling FUTURE_DISPATCH(_, (void)(Slave::*)(Framework*, Executor*) &Slave::remove), which satisfied the compiler. But I still wasn't sure if the canonicalization done by FUTURE_DISPATCH would correctly catch it. So I ended up renaming the overloaded remove() for ease of catching the dispatch.
I ended up reverting this test because it was getting too complicated. Mainly because I realized that remove(Framework*, Executor*) was never dispatched but rather just directly called as a function.
I would've reverted the rename too but looking at other methods in this file, they all seemed to follow the new convention.
shutdownFramework()
shutdownExecutor()
recoverFramework()
recoverExecutor()
recoverTask()
So I decided to keep the rename for consistency.
I am ok with renaming all of these into shorter names for consistency. Not sure if we want to do it in this review or not.
> On Aug. 1, 2013, 6:48 a.m., Benjamin Hindman wrote:
> > src/slave/status_update_manager.hpp, line 110
> > <https://reviews.apache.org/r/13187/diff/1/?file=332292#file332292line110>
> >
> > Any reason not to flatten these: Future<Try<bool>> => Future<bool>?
good call. i will flatten all the status update manager functions. will do that in a subsequent review though. sg?
- Vinod
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/13187/#review24419
-----------------------------------------------------------
On Aug. 1, 2013, 5:30 a.m., Vinod Kone wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/13187/
> -----------------------------------------------------------
>
> (Updated Aug. 1, 2013, 5:30 a.m.)
>
>
> Review request for mesos, Benjamin Hindman and Ben Mahler.
>
>
> Bugs: MESOS-606
> https://issues.apache.org/jira/browse/MESOS-606
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> Changed the semantics of StatusUpdateStream::acknowedgement()'s return value.
>
> Instead of informing about duplicate, it now informs whether the status update stream is terminated or not. This is definitely more powerful/useful information for slave to have. Duplicates are now considered Errors.
>
>
> Diffs
> -----
>
> src/slave/slave.hpp 97f2083db0dec2c1c9415f65d9bf8a834653bd18
> src/slave/slave.cpp 7f6e6b456890db438092f19a22e4dd816bb33d04
> src/slave/status_update_manager.hpp 795e74c2b88a071eb7ba720118e06077b6e41238
> src/slave/status_update_manager.cpp 9e9e4e2a47a609d65ed69a57de595852144a86c8
> src/tests/slave_recovery_tests.cpp 1871e3ba41e65dcbd4b95779dda068f6a1a2ecb3
> src/tests/status_update_manager_tests.cpp 42395324dfe49659bee2229c6573ffef0874d923
>
> Diff: https://reviews.apache.org/r/13187/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Vinod Kone
>
>
Re: Review Request 13187: Fixed slave to properly handle terminated tasks
that have pending updates.
Posted by Benjamin Hindman <be...@berkeley.edu>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/13187/#review24419
-----------------------------------------------------------
Ship it!
src/slave/slave.cpp
<https://reviews.apache.org/r/13187/#comment48328>
Remove framework framework. ;) Why the change?
src/slave/status_update_manager.hpp
<https://reviews.apache.org/r/13187/#comment48327>
Any reason not to flatten these: Future<Try<bool>> => Future<bool>?
- Benjamin Hindman
On Aug. 1, 2013, 5:30 a.m., Vinod Kone wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/13187/
> -----------------------------------------------------------
>
> (Updated Aug. 1, 2013, 5:30 a.m.)
>
>
> Review request for mesos, Benjamin Hindman and Ben Mahler.
>
>
> Bugs: MESOS-606
> https://issues.apache.org/jira/browse/MESOS-606
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> Changed the semantics of StatusUpdateStream::acknowedgement()'s return value.
>
> Instead of informing about duplicate, it now informs whether the status update stream is terminated or not. This is definitely more powerful/useful information for slave to have. Duplicates are now considered Errors.
>
>
> Diffs
> -----
>
> src/slave/slave.hpp 97f2083db0dec2c1c9415f65d9bf8a834653bd18
> src/slave/slave.cpp 7f6e6b456890db438092f19a22e4dd816bb33d04
> src/slave/status_update_manager.hpp 795e74c2b88a071eb7ba720118e06077b6e41238
> src/slave/status_update_manager.cpp 9e9e4e2a47a609d65ed69a57de595852144a86c8
> src/tests/slave_recovery_tests.cpp 1871e3ba41e65dcbd4b95779dda068f6a1a2ecb3
> src/tests/status_update_manager_tests.cpp 42395324dfe49659bee2229c6573ffef0874d923
>
> Diff: https://reviews.apache.org/r/13187/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Vinod Kone
>
>
Re: Review Request 13187: Fixed slave to properly handle terminated tasks
that have pending updates.
Posted by Vinod Kone <vi...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/13187/
-----------------------------------------------------------
(Updated Aug. 3, 2013, 8:29 p.m.)
Review request for mesos, Benjamin Hindman and Ben Mahler.
Changes
-------
rebased. nnfr.
Bugs: MESOS-606
https://issues.apache.org/jira/browse/MESOS-606
Repository: mesos-git
Description
-------
Changed the semantics of StatusUpdateStream::acknowedgement()'s return value.
Instead of informing about duplicate, it now informs whether the status update stream is terminated or not. This is definitely more powerful/useful information for slave to have. Duplicates are now considered Errors.
Diffs (updated)
-----
src/slave/slave.hpp 97f2083db0dec2c1c9415f65d9bf8a834653bd18
src/slave/slave.cpp 7f6e6b456890db438092f19a22e4dd816bb33d04
src/slave/status_update_manager.hpp 795e74c2b88a071eb7ba720118e06077b6e41238
src/slave/status_update_manager.cpp 9e9e4e2a47a609d65ed69a57de595852144a86c8
src/tests/slave_recovery_tests.cpp 1871e3ba41e65dcbd4b95779dda068f6a1a2ecb3
src/tests/status_update_manager_tests.cpp 42395324dfe49659bee2229c6573ffef0874d923
Diff: https://reviews.apache.org/r/13187/diff/
Testing
-------
make check
Thanks,
Vinod Kone