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 2014/07/24 01:18:46 UTC
Review Request 23867: Handle discards on the master detection futures.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/23867/
-----------------------------------------------------------
Review request for mesos, Vinod Kone and Jiang Yan Xu.
Bugs: MESOS-1529
https://issues.apache.org/jira/browse/MESOS-1529
Repository: mesos-git
Description
-------
In order to trigger re-detections in the slave, we need to be able to discard the Future returned from detect(). This adds discard support to detect().
Diffs
-----
src/master/detector.cpp a34cc211941972e2d64802d5c7ed3f5026229744
src/tests/master_contender_detector_tests.cpp 231648d3c564f9a38937b844459c08f0fdd66a92
Diff: https://reviews.apache.org/r/23867/diff/
Testing
-------
Updated the existing tests.
Thanks,
Ben Mahler
Re: Review Request 23867: Handle discards on the master detection futures.
Posted by Ben Mahler <be...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/23867/#review48596
-----------------------------------------------------------
src/master/detector.cpp
<https://reviews.apache.org/r/23867/#comment85307>
Cyclic reference, need to use a WeakFuture here.
- Ben Mahler
On July 23, 2014, 11:18 p.m., Ben Mahler wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/23867/
> -----------------------------------------------------------
>
> (Updated July 23, 2014, 11:18 p.m.)
>
>
> Review request for mesos, Vinod Kone and Jiang Yan Xu.
>
>
> Bugs: MESOS-1529
> https://issues.apache.org/jira/browse/MESOS-1529
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> In order to trigger re-detections in the slave, we need to be able to discard the Future returned from detect(). This adds discard support to detect().
>
>
> Diffs
> -----
>
> src/master/detector.cpp a34cc211941972e2d64802d5c7ed3f5026229744
> src/tests/master_contender_detector_tests.cpp 231648d3c564f9a38937b844459c08f0fdd66a92
>
> Diff: https://reviews.apache.org/r/23867/diff/
>
>
> Testing
> -------
>
> Updated the existing tests.
>
>
> Thanks,
>
> Ben Mahler
>
>
Re: Review Request 23867: Handle discards on the master detection futures.
Posted by Ben Mahler <be...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/23867/
-----------------------------------------------------------
(Updated Aug. 4, 2014, 7:25 p.m.)
Review request for mesos, Jie Yu and Jiang Yan Xu.
Changes
-------
Removed the use of WeakFuture as it is not necessary.
Bugs: MESOS-1529
https://issues.apache.org/jira/browse/MESOS-1529
Repository: mesos-git
Description
-------
In order to trigger re-detections in the slave, we need to be able to discard the Future returned from detect(). This adds discard support to detect().
Diffs (updated)
-----
src/master/detector.cpp a34cc211941972e2d64802d5c7ed3f5026229744
src/tests/master_contender_detector_tests.cpp 231648d3c564f9a38937b844459c08f0fdd66a92
Diff: https://reviews.apache.org/r/23867/diff/
Testing
-------
Updated the existing tests.
Thanks,
Ben Mahler
Re: Review Request 23867: Handle discards on the master detection futures.
Posted by Mesos ReviewBot <de...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/23867/#review48805
-----------------------------------------------------------
Bad patch!
Reviews applied: [23867]
Failed command: git apply --index 23867.patch
Error:
error: patch failed: src/master/detector.cpp:94
error: src/master/detector.cpp: patch does not apply
- Mesos ReviewBot
On July 24, 2014, 2:55 a.m., Ben Mahler wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/23867/
> -----------------------------------------------------------
>
> (Updated July 24, 2014, 2:55 a.m.)
>
>
> Review request for mesos, Jie Yu and Jiang Yan Xu.
>
>
> Bugs: MESOS-1529
> https://issues.apache.org/jira/browse/MESOS-1529
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> In order to trigger re-detections in the slave, we need to be able to discard the Future returned from detect(). This adds discard support to detect().
>
>
> Diffs
> -----
>
> src/master/detector.cpp a34cc211941972e2d64802d5c7ed3f5026229744
> src/tests/master_contender_detector_tests.cpp 231648d3c564f9a38937b844459c08f0fdd66a92
>
> Diff: https://reviews.apache.org/r/23867/diff/
>
>
> Testing
> -------
>
> Updated the existing tests.
>
>
> Thanks,
>
> Ben Mahler
>
>
Re: Review Request 23867: Handle discards on the master detection futures.
Posted by Jiang Yan Xu <ya...@jxu.me>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/23867/#review48691
-----------------------------------------------------------
Ship it!
src/master/detector.cpp
<https://reviews.apache.org/r/23867/#comment85453>
I don't if this is going overboard but perhaps it belongs with other methods (as a template) in the promises:: namespace in this file: "void discard(std::set<Promise<T>* >* promises, const WeakFuture<T>& future)"
There is a similar one in future.hpp too.
template <typename T>
void discard(WeakFuture<T> reference)
{
Option<Future<T> > future = reference.get();
if (future.isSome()) {
Future<T> future_ = future.get();
future_.discard();
}
}
I like that promises:: and futures:: namespaces are clearly separated to avoid confusion.
- Jiang Yan Xu
On July 23, 2014, 7:55 p.m., Ben Mahler wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/23867/
> -----------------------------------------------------------
>
> (Updated July 23, 2014, 7:55 p.m.)
>
>
> Review request for mesos, Jie Yu and Jiang Yan Xu.
>
>
> Bugs: MESOS-1529
> https://issues.apache.org/jira/browse/MESOS-1529
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> In order to trigger re-detections in the slave, we need to be able to discard the Future returned from detect(). This adds discard support to detect().
>
>
> Diffs
> -----
>
> src/master/detector.cpp a34cc211941972e2d64802d5c7ed3f5026229744
> src/tests/master_contender_detector_tests.cpp 231648d3c564f9a38937b844459c08f0fdd66a92
>
> Diff: https://reviews.apache.org/r/23867/diff/
>
>
> Testing
> -------
>
> Updated the existing tests.
>
>
> Thanks,
>
> Ben Mahler
>
>
Re: Review Request 23867: Handle discards on the master detection futures.
Posted by Ben Mahler <be...@gmail.com>.
> On July 25, 2014, 7:56 p.m., Jie Yu wrote:
> > src/master/detector.cpp, lines 145-151
> > <https://reviews.apache.org/r/23867/diff/2/?file=641171#file641171line145>
> >
> > This is more cleaner to me, But feel free to ignore:)
> >
> > promise->future()
> > .onDiscard(lambda::bind(discard, promise)) // discard is some helper just do promise->discard();
> > .onDiscarded(defer(self(), &Self::discarded, promise)); // in discarded, remove the promise from set
> >
> >
Thought about this, but it seemed a bit tricky to rely on the fact that the pointer is still valid when these get called.
> On July 25, 2014, 7:56 p.m., Jie Yu wrote:
> > src/master/detector.cpp, lines 145-151
> > <https://reviews.apache.org/r/23867/diff/2/?file=641171#file641171line145>
> >
> > I don't think WeakFuture is needed here because the reference to the internal Future::Data from the onDiscard callback will be cleared when Self::discard is called, and when the promise is deleted, the internal Future::Data will be freed.
Thanks Jie!
- Ben
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/23867/#review48760
-----------------------------------------------------------
On July 24, 2014, 2:55 a.m., Ben Mahler wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/23867/
> -----------------------------------------------------------
>
> (Updated July 24, 2014, 2:55 a.m.)
>
>
> Review request for mesos, Jie Yu and Jiang Yan Xu.
>
>
> Bugs: MESOS-1529
> https://issues.apache.org/jira/browse/MESOS-1529
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> In order to trigger re-detections in the slave, we need to be able to discard the Future returned from detect(). This adds discard support to detect().
>
>
> Diffs
> -----
>
> src/master/detector.cpp a34cc211941972e2d64802d5c7ed3f5026229744
> src/tests/master_contender_detector_tests.cpp 231648d3c564f9a38937b844459c08f0fdd66a92
>
> Diff: https://reviews.apache.org/r/23867/diff/
>
>
> Testing
> -------
>
> Updated the existing tests.
>
>
> Thanks,
>
> Ben Mahler
>
>
Re: Review Request 23867: Handle discards on the master detection futures.
Posted by Jie Yu <yu...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/23867/#review48760
-----------------------------------------------------------
src/master/detector.cpp
<https://reviews.apache.org/r/23867/#comment85506>
I don't think WeakFuture is needed here because the reference to the internal Future::Data from the onDiscard callback will be cleared when Self::discard is called, and when the promise is deleted, the internal Future::Data will be freed.
src/master/detector.cpp
<https://reviews.apache.org/r/23867/#comment85507>
This is more cleaner to me, But feel free to ignore:)
promise->future()
.onDiscard(lambda::bind(discard, promise)) // discard is some helper just do promise->discard();
.onDiscarded(defer(self(), &Self::discarded, promise)); // in discarded, remove the promise from set
- Jie Yu
On July 24, 2014, 2:55 a.m., Ben Mahler wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/23867/
> -----------------------------------------------------------
>
> (Updated July 24, 2014, 2:55 a.m.)
>
>
> Review request for mesos, Jie Yu and Jiang Yan Xu.
>
>
> Bugs: MESOS-1529
> https://issues.apache.org/jira/browse/MESOS-1529
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> In order to trigger re-detections in the slave, we need to be able to discard the Future returned from detect(). This adds discard support to detect().
>
>
> Diffs
> -----
>
> src/master/detector.cpp a34cc211941972e2d64802d5c7ed3f5026229744
> src/tests/master_contender_detector_tests.cpp 231648d3c564f9a38937b844459c08f0fdd66a92
>
> Diff: https://reviews.apache.org/r/23867/diff/
>
>
> Testing
> -------
>
> Updated the existing tests.
>
>
> Thanks,
>
> Ben Mahler
>
>
Re: Review Request 23867: Handle discards on the master detection futures.
Posted by Ben Mahler <be...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/23867/
-----------------------------------------------------------
(Updated July 24, 2014, 2:55 a.m.)
Review request for mesos, Jie Yu and Jiang Yan Xu.
Changes
-------
Used a WeakFuture to break the cyclic dependency.
Bugs: MESOS-1529
https://issues.apache.org/jira/browse/MESOS-1529
Repository: mesos-git
Description
-------
In order to trigger re-detections in the slave, we need to be able to discard the Future returned from detect(). This adds discard support to detect().
Diffs (updated)
-----
src/master/detector.cpp a34cc211941972e2d64802d5c7ed3f5026229744
src/tests/master_contender_detector_tests.cpp 231648d3c564f9a38937b844459c08f0fdd66a92
Diff: https://reviews.apache.org/r/23867/diff/
Testing
-------
Updated the existing tests.
Thanks,
Ben Mahler