You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@mesos.apache.org by Jiang Yan Xu <ya...@jxu.me> on 2013/12/16 11:12:49 UTC
Review Request 16291: Changed master detector to return Future
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/16291/
-----------------------------------------------------------
Review request for mesos, Benjamin Hindman, Ben Mahler, and Vinod Kone.
Bugs: MESOS-883
https://issues.apache.org/jira/browse/MESOS-883
Repository: mesos-git
Description
-------
Now that local session timeouts are transparent to the clients of the master detector, they are not supposed to retry anymore. Thus passing "Result<UPID> previous" back into MasterDetector::detect() is no longer correct.
Diffs
-----
src/cli/resolve.cpp dddadfc2bff41752372e0e817beb0493cd0cd127
src/master/detector.hpp 6e7a4c42c8a1dcfe95b2244213013d3f0aff311a
src/master/detector.cpp 2f73f669f0b3a8dccbce9fdfa941593314302171
src/master/master.hpp 6c168a2cdbd8343516cb47adceaff70c3d46690b
src/master/master.cpp dd6111946289b82008a66e46df8ef5e538de70ea
src/sched/sched.cpp c46535643ce2ea456bd939571fe78358c1d3871b
src/slave/slave.hpp 71fa4f0d0ccde8cdc023b18c6a4d7eb7478fd0cf
src/slave/slave.cpp 6e6107e3551f29566ff233dad47dac8c29b4fab5
src/tests/master_contender_detector_tests.cpp 76464eab479461e6e3cb8b5afe85860e60428cf5
src/tests/zookeeper_tests.cpp a0660cbfb1b7073654b05b82bffd69fbb04d0165
src/zookeeper/detector.hpp de4acab422e2fa4b7420f44b4e8ebc941f91f200
src/zookeeper/detector.cpp 1de3663ecaed6d8c362c20a5a46d2f4d73f8fbd0
Diff: https://reviews.apache.org/r/16291/diff/
Testing
-------
make check.
Thanks,
Jiang Yan Xu
Re: Review Request 16291: Changed master detector to return Future
Posted by Ben Mahler <be...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/16291/#review30716
-----------------------------------------------------------
Ship it!
Ship It!
- Ben Mahler
On Dec. 19, 2013, 7:57 a.m., Jiang Yan Xu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/16291/
> -----------------------------------------------------------
>
> (Updated Dec. 19, 2013, 7:57 a.m.)
>
>
> Review request for mesos, Benjamin Hindman, Ben Mahler, and Vinod Kone.
>
>
> Bugs: MESOS-883
> https://issues.apache.org/jira/browse/MESOS-883
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> Now that local session timeouts are transparent to the clients of the master detector, they are not supposed to retry anymore. Thus passing "Result<UPID> previous" back into MasterDetector::detect() is no longer correct.
>
>
> Diffs
> -----
>
> src/cli/resolve.cpp dddadfc2bff41752372e0e817beb0493cd0cd127
> src/master/detector.hpp 6e7a4c42c8a1dcfe95b2244213013d3f0aff311a
> src/master/detector.cpp 2f73f669f0b3a8dccbce9fdfa941593314302171
> src/master/master.hpp 6c168a2cdbd8343516cb47adceaff70c3d46690b
> src/master/master.cpp dd6111946289b82008a66e46df8ef5e538de70ea
> src/sched/sched.cpp c46535643ce2ea456bd939571fe78358c1d3871b
> src/slave/slave.hpp 71fa4f0d0ccde8cdc023b18c6a4d7eb7478fd0cf
> src/slave/slave.cpp 6e6107e3551f29566ff233dad47dac8c29b4fab5
> src/tests/master_contender_detector_tests.cpp 76464eab479461e6e3cb8b5afe85860e60428cf5
> src/tests/zookeeper_tests.cpp a0660cbfb1b7073654b05b82bffd69fbb04d0165
> src/zookeeper/detector.hpp de4acab422e2fa4b7420f44b4e8ebc941f91f200
> src/zookeeper/detector.cpp 1de3663ecaed6d8c362c20a5a46d2f4d73f8fbd0
>
> Diff: https://reviews.apache.org/r/16291/diff/
>
>
> Testing
> -------
>
> make check.
>
>
> Thanks,
>
> Jiang Yan Xu
>
>
Re: Review Request 16291: Changed master detector to return Future
Posted by Jiang Yan Xu <ya...@jxu.me>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/16291/
-----------------------------------------------------------
(Updated Dec. 19, 2013, 10:23 p.m.)
Review request for mesos, Benjamin Hindman, Ben Mahler, and Vinod Kone.
Changes
-------
Rebase. NNFR.
Bugs: MESOS-883
https://issues.apache.org/jira/browse/MESOS-883
Repository: mesos-git
Description
-------
Now that local session timeouts are transparent to the clients of the master detector, they are not supposed to retry anymore. Thus passing "Result<UPID> previous" back into MasterDetector::detect() is no longer correct.
Diffs (updated)
-----
src/cli/resolve.cpp dddadfc2bff41752372e0e817beb0493cd0cd127
src/master/detector.hpp 6e7a4c42c8a1dcfe95b2244213013d3f0aff311a
src/master/detector.cpp 2f73f669f0b3a8dccbce9fdfa941593314302171
src/master/master.hpp 6c168a2cdbd8343516cb47adceaff70c3d46690b
src/master/master.cpp 30cea5431a8eb81cb3751a66889a3c3d27b425a0
src/sched/sched.cpp c46535643ce2ea456bd939571fe78358c1d3871b
src/slave/slave.hpp 71fa4f0d0ccde8cdc023b18c6a4d7eb7478fd0cf
src/slave/slave.cpp 6e6107e3551f29566ff233dad47dac8c29b4fab5
src/tests/master_contender_detector_tests.cpp 76464eab479461e6e3cb8b5afe85860e60428cf5
src/tests/zookeeper_tests.cpp a0660cbfb1b7073654b05b82bffd69fbb04d0165
src/zookeeper/detector.hpp de4acab422e2fa4b7420f44b4e8ebc941f91f200
src/zookeeper/detector.cpp 1de3663ecaed6d8c362c20a5a46d2f4d73f8fbd0
Diff: https://reviews.apache.org/r/16291/diff/
Testing
-------
make check.
Thanks,
Jiang Yan Xu
Re: Review Request 16291: Changed master detector to return Future
Posted by Jiang Yan Xu <ya...@jxu.me>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/16291/
-----------------------------------------------------------
(Updated Dec. 19, 2013, 7:57 a.m.)
Review request for mesos, Benjamin Hindman, Ben Mahler, and Vinod Kone.
Changes
-------
Moved some changes to r16290.
Bugs: MESOS-883
https://issues.apache.org/jira/browse/MESOS-883
Repository: mesos-git
Description
-------
Now that local session timeouts are transparent to the clients of the master detector, they are not supposed to retry anymore. Thus passing "Result<UPID> previous" back into MasterDetector::detect() is no longer correct.
Diffs (updated)
-----
src/cli/resolve.cpp dddadfc2bff41752372e0e817beb0493cd0cd127
src/master/detector.hpp 6e7a4c42c8a1dcfe95b2244213013d3f0aff311a
src/master/detector.cpp 2f73f669f0b3a8dccbce9fdfa941593314302171
src/master/master.hpp 6c168a2cdbd8343516cb47adceaff70c3d46690b
src/master/master.cpp dd6111946289b82008a66e46df8ef5e538de70ea
src/sched/sched.cpp c46535643ce2ea456bd939571fe78358c1d3871b
src/slave/slave.hpp 71fa4f0d0ccde8cdc023b18c6a4d7eb7478fd0cf
src/slave/slave.cpp 6e6107e3551f29566ff233dad47dac8c29b4fab5
src/tests/master_contender_detector_tests.cpp 76464eab479461e6e3cb8b5afe85860e60428cf5
src/tests/zookeeper_tests.cpp a0660cbfb1b7073654b05b82bffd69fbb04d0165
src/zookeeper/detector.hpp de4acab422e2fa4b7420f44b4e8ebc941f91f200
src/zookeeper/detector.cpp 1de3663ecaed6d8c362c20a5a46d2f4d73f8fbd0
Diff: https://reviews.apache.org/r/16291/diff/
Testing
-------
make check.
Thanks,
Jiang Yan Xu
Re: Review Request 16291: Changed master detector to return Future
Posted by Jiang Yan Xu <ya...@jxu.me>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/16291/
-----------------------------------------------------------
(Updated Dec. 18, 2013, 6:01 a.m.)
Review request for mesos, Benjamin Hindman, Ben Mahler, and Vinod Kone.
Changes
-------
Minor.
Bugs: MESOS-883
https://issues.apache.org/jira/browse/MESOS-883
Repository: mesos-git
Description
-------
Now that local session timeouts are transparent to the clients of the master detector, they are not supposed to retry anymore. Thus passing "Result<UPID> previous" back into MasterDetector::detect() is no longer correct.
Diffs (updated)
-----
src/cli/resolve.cpp dddadfc2bff41752372e0e817beb0493cd0cd127
src/master/detector.hpp 6e7a4c42c8a1dcfe95b2244213013d3f0aff311a
src/master/detector.cpp 2f73f669f0b3a8dccbce9fdfa941593314302171
src/master/master.hpp 6c168a2cdbd8343516cb47adceaff70c3d46690b
src/master/master.cpp dd6111946289b82008a66e46df8ef5e538de70ea
src/sched/sched.cpp c46535643ce2ea456bd939571fe78358c1d3871b
src/slave/slave.hpp 71fa4f0d0ccde8cdc023b18c6a4d7eb7478fd0cf
src/slave/slave.cpp 6e6107e3551f29566ff233dad47dac8c29b4fab5
src/tests/master_contender_detector_tests.cpp 76464eab479461e6e3cb8b5afe85860e60428cf5
src/tests/zookeeper_tests.cpp a0660cbfb1b7073654b05b82bffd69fbb04d0165
src/zookeeper/detector.hpp de4acab422e2fa4b7420f44b4e8ebc941f91f200
src/zookeeper/detector.cpp 1de3663ecaed6d8c362c20a5a46d2f4d73f8fbd0
Diff: https://reviews.apache.org/r/16291/diff/
Testing
-------
make check.
Thanks,
Jiang Yan Xu
Re: Review Request 16291: Changed master detector to return Future
Posted by Jiang Yan Xu <ya...@jxu.me>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/16291/
-----------------------------------------------------------
(Updated Dec. 18, 2013, 1:01 a.m.)
Review request for mesos, Benjamin Hindman, Ben Mahler, and Vinod Kone.
Bugs: MESOS-883
https://issues.apache.org/jira/browse/MESOS-883
Repository: mesos-git
Description
-------
Now that local session timeouts are transparent to the clients of the master detector, they are not supposed to retry anymore. Thus passing "Result<UPID> previous" back into MasterDetector::detect() is no longer correct.
Diffs
-----
src/cli/resolve.cpp dddadfc2bff41752372e0e817beb0493cd0cd127
src/master/detector.hpp 6e7a4c42c8a1dcfe95b2244213013d3f0aff311a
src/master/detector.cpp 2f73f669f0b3a8dccbce9fdfa941593314302171
src/master/master.hpp 6c168a2cdbd8343516cb47adceaff70c3d46690b
src/master/master.cpp dd6111946289b82008a66e46df8ef5e538de70ea
src/sched/sched.cpp c46535643ce2ea456bd939571fe78358c1d3871b
src/slave/slave.hpp 71fa4f0d0ccde8cdc023b18c6a4d7eb7478fd0cf
src/slave/slave.cpp 6e6107e3551f29566ff233dad47dac8c29b4fab5
src/tests/master_contender_detector_tests.cpp 76464eab479461e6e3cb8b5afe85860e60428cf5
src/tests/zookeeper_tests.cpp a0660cbfb1b7073654b05b82bffd69fbb04d0165
src/zookeeper/detector.hpp de4acab422e2fa4b7420f44b4e8ebc941f91f200
src/zookeeper/detector.cpp 1de3663ecaed6d8c362c20a5a46d2f4d73f8fbd0
Diff: https://reviews.apache.org/r/16291/diff/
Testing
-------
make check.
Thanks,
Jiang Yan Xu
Re: Review Request 16291: Changed master detector to return Future
Posted by Jiang Yan Xu <ya...@jxu.me>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/16291/
-----------------------------------------------------------
(Updated Dec. 18, 2013, 1 a.m.)
Review request for mesos, Benjamin Hindman, Ben Mahler, and Vinod Kone.
Changes
-------
BenM's comments.
Bugs: MESOS-883
https://issues.apache.org/jira/browse/MESOS-883
Repository: mesos-git
Description
-------
Now that local session timeouts are transparent to the clients of the master detector, they are not supposed to retry anymore. Thus passing "Result<UPID> previous" back into MasterDetector::detect() is no longer correct.
Diffs (updated)
-----
src/cli/resolve.cpp dddadfc2bff41752372e0e817beb0493cd0cd127
src/master/detector.hpp 6e7a4c42c8a1dcfe95b2244213013d3f0aff311a
src/master/detector.cpp 2f73f669f0b3a8dccbce9fdfa941593314302171
src/master/master.hpp 6c168a2cdbd8343516cb47adceaff70c3d46690b
src/master/master.cpp dd6111946289b82008a66e46df8ef5e538de70ea
src/sched/sched.cpp c46535643ce2ea456bd939571fe78358c1d3871b
src/slave/slave.hpp 71fa4f0d0ccde8cdc023b18c6a4d7eb7478fd0cf
src/slave/slave.cpp 6e6107e3551f29566ff233dad47dac8c29b4fab5
src/tests/master_contender_detector_tests.cpp 76464eab479461e6e3cb8b5afe85860e60428cf5
src/tests/zookeeper_tests.cpp a0660cbfb1b7073654b05b82bffd69fbb04d0165
src/zookeeper/detector.hpp de4acab422e2fa4b7420f44b4e8ebc941f91f200
src/zookeeper/detector.cpp 1de3663ecaed6d8c362c20a5a46d2f4d73f8fbd0
Diff: https://reviews.apache.org/r/16291/diff/
Testing
-------
make check.
Thanks,
Jiang Yan Xu
Re: Review Request 16291: Changed master detector to return Future
Posted by Jiang Yan Xu <ya...@jxu.me>.
> On Dec. 17, 2013, 1:44 a.m., Ben Mahler wrote:
> > src/master/detector.hpp, lines 66-67
> > <https://reviews.apache.org/r/16291/diff/1/?file=398168#file398168line66>
> >
> > This feels odd to me (see my comment below) since we haven't encountered a failure in this case.
This is now obsolete because we are indeed returning failed futures for "errors".
> On Dec. 17, 2013, 1:44 a.m., Ben Mahler wrote:
> > src/master/detector.hpp, line 90
> > <https://reviews.apache.org/r/16291/diff/1/?file=398168#file398168line90>
> >
> > What does it mean to be "detected by default"?
Removed "by default". Here "can be *detected*" just means "we are really telling the detector what should be detected".
> On Dec. 17, 2013, 1:44 a.m., Ben Mahler wrote:
> > src/master/detector.hpp, line 91
> > <https://reviews.apache.org/r/16291/diff/1/?file=398168#file398168line91>
> >
> > I'm not sure why you need to call this out. Given the argument is an Option, None can be passed.
Okay
> On Dec. 17, 2013, 1:44 a.m., Ben Mahler wrote:
> > src/master/detector.hpp, lines 96-97
> > <https://reviews.apache.org/r/16291/diff/1/?file=398168#file398168line96>
> >
> > This is no longer true?
Killed the paragraph.
> On Dec. 17, 2013, 1:44 a.m., Ben Mahler wrote:
> > src/master/detector.cpp, line 150
> > <https://reviews.apache.org/r/16291/diff/1/?file=398169#file398169line150>
> >
> > We're using a failure here for what seems like a discard. Any reason?
> On Dec. 17, 2013, 1:44 a.m., Ben Mahler wrote:
> > src/master/detector.cpp, lines 312-314
> > <https://reviews.apache.org/r/16291/diff/1/?file=398169#file398169line312>
> >
> > It's a bit concerning that we do the group->data operation and the detector.detect operation at the same time. Have you thought about what might happen if, say, there's a race and the detected() callback happens before fetched()?
I think it's fine.
If the detect() returns first, the result of fetched() may not be the most up-to-date but it will be refreshed soon.
It's not different from the alternative: calling detect() in fetched() - leader could have changed on the server.
Moreover, we are not saving the Group::Membership returned in detected (at least for now) which is needed for re-detecting.
> On Dec. 17, 2013, 1:44 a.m., Ben Mahler wrote:
> > src/sched/sched.cpp, line 421
> > <https://reviews.apache.org/r/16291/diff/1/?file=398172#file398172line421>
> >
> > All the message handlers look at !master.isSome(), can you clean those up as well?
I think this file is all cleaned up. There are some master.isSome() without "!"
> On Dec. 17, 2013, 1:44 a.m., Ben Mahler wrote:
> > src/slave/slave.cpp, line 432
> > <https://reviews.apache.org/r/16291/diff/1/?file=398174#file398174line432>
> >
> > We may want to make equality operators for Option<T> vs T so that these comparisons can be cleaned up to be just:
> >
> > if (from && master != from) {
Done.
> On Dec. 17, 2013, 1:44 a.m., Ben Mahler wrote:
> > src/zookeeper/detector.hpp, line 30
> > <https://reviews.apache.org/r/16291/diff/1/?file=398177#file398177line30>
> >
> > s/some/a/
Done.
> On Dec. 17, 2013, 1:44 a.m., Ben Mahler wrote:
> > src/zookeeper/detector.hpp, lines 31-32
> > <https://reviews.apache.org/r/16291/diff/1/?file=398177#file398177line31>
> >
> > We probably should not refer to a 'user' here. How about:
> >
> > // Note that the detector transparently tries to recover from retryable errors.
Done.
> On Dec. 17, 2013, 1:44 a.m., Ben Mahler wrote:
> > src/master/detector.cpp, lines 325-333
> > <https://reviews.apache.org/r/16291/diff/1/?file=398169#file398169line325>
> >
> > Shouldn't this be setting the error field?
Removed error field as it's unnecessary - we can let future detector::detect() proceed and group will return failures directly.
- Jiang Yan
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/16291/#review30505
-----------------------------------------------------------
On Dec. 16, 2013, 10:12 a.m., Jiang Yan Xu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/16291/
> -----------------------------------------------------------
>
> (Updated Dec. 16, 2013, 10:12 a.m.)
>
>
> Review request for mesos, Benjamin Hindman, Ben Mahler, and Vinod Kone.
>
>
> Bugs: MESOS-883
> https://issues.apache.org/jira/browse/MESOS-883
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> Now that local session timeouts are transparent to the clients of the master detector, they are not supposed to retry anymore. Thus passing "Result<UPID> previous" back into MasterDetector::detect() is no longer correct.
>
>
> Diffs
> -----
>
> src/cli/resolve.cpp dddadfc2bff41752372e0e817beb0493cd0cd127
> src/master/detector.hpp 6e7a4c42c8a1dcfe95b2244213013d3f0aff311a
> src/master/detector.cpp 2f73f669f0b3a8dccbce9fdfa941593314302171
> src/master/master.hpp 6c168a2cdbd8343516cb47adceaff70c3d46690b
> src/master/master.cpp dd6111946289b82008a66e46df8ef5e538de70ea
> src/sched/sched.cpp c46535643ce2ea456bd939571fe78358c1d3871b
> src/slave/slave.hpp 71fa4f0d0ccde8cdc023b18c6a4d7eb7478fd0cf
> src/slave/slave.cpp 6e6107e3551f29566ff233dad47dac8c29b4fab5
> src/tests/master_contender_detector_tests.cpp 76464eab479461e6e3cb8b5afe85860e60428cf5
> src/tests/zookeeper_tests.cpp a0660cbfb1b7073654b05b82bffd69fbb04d0165
> src/zookeeper/detector.hpp de4acab422e2fa4b7420f44b4e8ebc941f91f200
> src/zookeeper/detector.cpp 1de3663ecaed6d8c362c20a5a46d2f4d73f8fbd0
>
> Diff: https://reviews.apache.org/r/16291/diff/
>
>
> Testing
> -------
>
> make check.
>
>
> Thanks,
>
> Jiang Yan Xu
>
>
Re: Review Request 16291: Changed master detector to return Future
Posted by Ben Mahler <be...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/16291/#review30505
-----------------------------------------------------------
I've commented on some bugs and cleanup here, as well as some cleanup leftover from the original changes. With the latter, you may want to consider addressing these in an earlier change.
src/cli/resolve.cpp
<https://reviews.apache.org/r/16291/#comment58372>
Can you check directly against discarded?
We can check this above the failure check:
} else {
CHECK(!pid.isDiscarded());
if (pid.isFailed()) {
...
}
}
src/master/detector.hpp
<https://reviews.apache.org/r/16291/#comment58388>
Returns a ...?
src/master/detector.hpp
<https://reviews.apache.org/r/16291/#comment58387>
s/some/a/
src/master/detector.hpp
<https://reviews.apache.org/r/16291/#comment58389>
See my other comment about referring to a 'user'.
src/master/detector.hpp
<https://reviews.apache.org/r/16291/#comment58399>
This feels odd to me (see my comment below) since we haven't encountered a failure in this case.
src/master/detector.hpp
<https://reviews.apache.org/r/16291/#comment58402>
What does it mean to be "detected by default"?
src/master/detector.hpp
<https://reviews.apache.org/r/16291/#comment58390>
I'm not sure why you need to call this out. Given the argument is an Option, None can be passed.
src/master/detector.hpp
<https://reviews.apache.org/r/16291/#comment58403>
This is no longer true?
src/master/detector.hpp
<https://reviews.apache.org/r/16291/#comment58404>
In general I don't think it's necessary to mention things like "A constructor overload" here since it is self-evident. It seems sufficient to say:
// Used for testing purposes.
src/master/detector.cpp
<https://reviews.apache.org/r/16291/#comment58405>
Option<UPID> leader; // The appointed master.
src/master/detector.cpp
<https://reviews.apache.org/r/16291/#comment58407>
Feel free to just kill this comment.
src/master/detector.cpp
<https://reviews.apache.org/r/16291/#comment58406>
This comment seems out of place.
src/master/detector.cpp
<https://reviews.apache.org/r/16291/#comment58391>
Ditto my other comment below about 'operable'.
src/master/detector.cpp
<https://reviews.apache.org/r/16291/#comment58392>
Ditto about Option<Error>.
src/master/detector.cpp
<https://reviews.apache.org/r/16291/#comment58393>
We're using a failure here for what seems like a discard. Any reason?
src/master/detector.cpp
<https://reviews.apache.org/r/16291/#comment58408>
You're missing an if in this sentence? This comment and the one in ZK should just be the same, no? I probably would kill this comment since the code is now much simpler.
src/master/detector.cpp
<https://reviews.apache.org/r/16291/#comment58409>
Ditto about discarded.
src/master/detector.cpp
<https://reviews.apache.org/r/16291/#comment58410>
Ditto about killing this comment now that the code is obvious.
src/master/detector.cpp
<https://reviews.apache.org/r/16291/#comment58411>
Ditto about checking for discarded explicitly and not attributing the discard to anyone specifically.
src/master/detector.cpp
<https://reviews.apache.org/r/16291/#comment58412>
It's a bit concerning that we do the group->data operation and the detector.detect operation at the same time. Have you thought about what might happen if, say, there's a race and the detected() callback happens before fetched()?
src/master/detector.cpp
<https://reviews.apache.org/r/16291/#comment58414>
Shouldn't this be setting the error field?
src/master/master.cpp
<https://reviews.apache.org/r/16291/#comment58420>
Ditto about explicitly checking and not attributing the discard to a particular component.
src/sched/sched.cpp
<https://reviews.apache.org/r/16291/#comment58417>
Ditto about checking for discard directly.
src/sched/sched.cpp
<https://reviews.apache.org/r/16291/#comment58418>
else now?
src/sched/sched.cpp
<https://reviews.apache.org/r/16291/#comment58419>
All the message handlers look at !master.isSome(), can you clean those up as well?
src/slave/slave.cpp
<https://reviews.apache.org/r/16291/#comment58421>
We may want to make equality operators for Option<T> vs T so that these comparisons can be cleaned up to be just:
if (from && master != from) {
src/slave/slave.cpp
<https://reviews.apache.org/r/16291/#comment58422>
Ditto discard/attribution.
src/slave/slave.cpp
<https://reviews.apache.org/r/16291/#comment58424>
Just else now?
src/tests/master_contender_detector_tests.cpp
<https://reviews.apache.org/r/16291/#comment58384>
No need to store joined in a variable.
src/tests/master_contender_detector_tests.cpp
<https://reviews.apache.org/r/16291/#comment58385>
No need to store result in a variable.
src/zookeeper/detector.hpp
<https://reviews.apache.org/r/16291/#comment58374>
s/some/a/
src/zookeeper/detector.hpp
<https://reviews.apache.org/r/16291/#comment58375>
We probably should not refer to a 'user' here. How about:
// Note that the detector transparently tries to recover from retryable errors.
src/zookeeper/detector.cpp
<https://reviews.apache.org/r/16291/#comment58376>
What does no longer operable mean? Can you be more explicit?
We should probably mention that these are non-retryable?
src/zookeeper/detector.cpp
<https://reviews.apache.org/r/16291/#comment58373>
Option<Error>
src/zookeeper/detector.cpp
<https://reviews.apache.org/r/16291/#comment58377>
Is this still supposed to start over when a failure is encountered? Looks like this will spin against Group::watch returning failures immediately.
src/zookeeper/detector.cpp
<https://reviews.apache.org/r/16291/#comment58379>
CHECK not discarded directly. I'm also not sure that mentioning Group is the right thing to do here given we can't accurately attribute the discard to Group.
src/zookeeper/detector.cpp
<https://reviews.apache.org/r/16291/#comment58383>
Looks like this logic be simplified now that you're using an Option:
if (current != leader) {
foreach (promise, promises) {
promise->set(current);
}
promises.clear();
}
src/zookeeper/detector.cpp
<https://reviews.apache.org/r/16291/#comment58381>
Does this log message make sense? This case looks to be more aptly described as there being no masters detected?
- Ben Mahler
On Dec. 16, 2013, 10:12 a.m., Jiang Yan Xu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/16291/
> -----------------------------------------------------------
>
> (Updated Dec. 16, 2013, 10:12 a.m.)
>
>
> Review request for mesos, Benjamin Hindman, Ben Mahler, and Vinod Kone.
>
>
> Bugs: MESOS-883
> https://issues.apache.org/jira/browse/MESOS-883
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> Now that local session timeouts are transparent to the clients of the master detector, they are not supposed to retry anymore. Thus passing "Result<UPID> previous" back into MasterDetector::detect() is no longer correct.
>
>
> Diffs
> -----
>
> src/cli/resolve.cpp dddadfc2bff41752372e0e817beb0493cd0cd127
> src/master/detector.hpp 6e7a4c42c8a1dcfe95b2244213013d3f0aff311a
> src/master/detector.cpp 2f73f669f0b3a8dccbce9fdfa941593314302171
> src/master/master.hpp 6c168a2cdbd8343516cb47adceaff70c3d46690b
> src/master/master.cpp dd6111946289b82008a66e46df8ef5e538de70ea
> src/sched/sched.cpp c46535643ce2ea456bd939571fe78358c1d3871b
> src/slave/slave.hpp 71fa4f0d0ccde8cdc023b18c6a4d7eb7478fd0cf
> src/slave/slave.cpp 6e6107e3551f29566ff233dad47dac8c29b4fab5
> src/tests/master_contender_detector_tests.cpp 76464eab479461e6e3cb8b5afe85860e60428cf5
> src/tests/zookeeper_tests.cpp a0660cbfb1b7073654b05b82bffd69fbb04d0165
> src/zookeeper/detector.hpp de4acab422e2fa4b7420f44b4e8ebc941f91f200
> src/zookeeper/detector.cpp 1de3663ecaed6d8c362c20a5a46d2f4d73f8fbd0
>
> Diff: https://reviews.apache.org/r/16291/diff/
>
>
> Testing
> -------
>
> make check.
>
>
> Thanks,
>
> Jiang Yan Xu
>
>