-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/13087/#review28606
-----------------------------------------------------------
Reviewed only the new detector / contender code:
src/master/{contender|detector}.{hpp|cpp}
src/master/contender.cpp
<https://reviews.apache.org/r/13087/#comment55473>
Looks like the code below uses std::string even though we're using the
whole namespace. Let's not use the full std namespace, can you update this to:
using std::string;
and below s/std::string/string/
src/master/contender.cpp
<https://reviews.apache.org/r/13087/#comment55474>
We can turn 17 lines of code into 8 lines using os::read and Error! And
we'll get to see the underlying read error :)
const string& path = zk.substr(7);
const Try<string> read = os::read(path);
if (read.isError()) {
return Error(...);
}
return create(strings::trim(read.get());
src/master/contender.cpp
<https://reviews.apache.org/r/13087/#comment55475>
promise->set(Nothing)); // Leadership lost.
src/master/contender.cpp
<https://reviews.apache.org/r/13087/#comment55472>
Seems like a good location for a CHECK instead?
src/master/contender.cpp
<https://reviews.apache.org/r/13087/#comment55470>
Seems like a nice location for a CHECK instead?
src/master/contender.cpp
<https://reviews.apache.org/r/13087/#comment55471>
So we rely on the withdrawal to occur via the destructor, makes me wonder
if we need withdraw() as opposed to using discard() / the destructor to
withdraw.
src/master/detector.cpp
<https://reviews.apache.org/r/13087/#comment55476>
s/std::set/set/ and ditto for the rest of this file (you're using the
process namespace but writing process::UPID, process::Promise, etc)
But we should make this a queue instead :)
src/master/detector.cpp
<https://reviews.apache.org/r/13087/#comment55480>
Seems like a queue is more intuitive here?
src/master/detector.cpp
<https://reviews.apache.org/r/13087/#comment55482>
Can we do the same cleanup here with os::read() and Error()?
src/master/detector.cpp
<https://reviews.apache.org/r/13087/#comment55483>
Do you think discarded makes a little more sense?
src/master/detector.cpp
<https://reviews.apache.org/r/13087/#comment55484>
empty()?
But why is this a WARNING? Won't we notify later when they call detect?
src/master/detector.cpp
<https://reviews.apache.org/r/13087/#comment55477>
Can you make the same change in this logic as I suggested in
https://reviews.apache.org/r/13086/ ?
src/master/detector.cpp
<https://reviews.apache.org/r/13087/#comment55478>
Error or discarded both seem to make sense here but I think discarded makes
a little more sense?
src/master/detector.cpp
<https://reviews.apache.org/r/13087/#comment55479>
It seems like a discard / failure should propagate up to the caller instead.
We can fail/discard the Promises futures instead of CHECKing, no?
src/master/detector.cpp
<https://reviews.apache.org/r/13087/#comment55481>
We need to make sure the fetched() and the detected() cannot be re-ordered
by enforcing order on the underlying group.
The ordering problems in GroupProcess make me a little concerned here. We
do not want to re-order detection results! (As in, we need to make sure
fetched() runs before the subsequent detected())).
A simple workaround here could be to do the subsequent detect() once we've
fetched():
if (!_leader.get().isSome()) {
...
// Continue detecting.
detector.detect(_leader.get())
.onAny(defer(self(), &Self::detected, lambda::_1));
} else {
// Fetch the data.
group->data(_leader.get().get())
.onAny(defer(self(), &Self::fetched, lambda::_1));
}
}
Then inside fetched() {
// Continue detecting.
detector.detect(...)
.onAny(...);
}
What do you think?
- Ben Mahler
On Nov. 5, 2013, 10:29 p.m., Jiang Yan Xu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/13087/
> -----------------------------------------------------------
>
> (Updated Nov. 5, 2013, 10:29 p.m.)
>
>
> Review request for mesos, Benjamin Hindman, Ben Mahler, and Vinod Kone.
>
>
> Bugs: MESOS-496
> https://issues.apache.org/jira/browse/MESOS-496
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> See summary.
>
>
> Diffs
> -----
>
> include/mesos/scheduler.hpp fa1ffe8b24a054556593d31eeae3fd1ac1761fb5
> src/Makefile.am 9780d07a23ca196c541a44a85499c2f44a574b9c
> src/cli/resolve.cpp b05f5103ca898f74acb7dfe8f423ffc7e1872e09
> src/detector/detector.hpp 3aaebfe89f0b4bb5c64e7f4c8974d8b13bc6f6b1
> src/detector/detector.cpp 8d9f118757192ca38bcdb26663ed86effbcf3c9e
> src/local/local.cpp 180756a929baed4dce97fdb8774f07010587468f
> src/local/main.cpp 5995c538077dea301f231668b4b5f905e73e7b3b
> src/master/contender.hpp PRE-CREATION
> src/master/contender.cpp PRE-CREATION
> src/master/detector.hpp PRE-CREATION
> src/master/detector.cpp PRE-CREATION
> src/master/http.cpp f2a535a002f07784db724f6fdedfb26b271d6327
> src/master/main.cpp 45caf9dde0af5995dde73ae82e83e1d7169d17b8
> src/master/master.hpp e377af8b3ccd932ae411fa2df4c19642a7310d02
> src/master/master.cpp 8e14a070e87ebe579b54d05fb1e8b286edb5e459
> src/messages/messages.proto a5dded2c0f3f88a5f7d789ccde6e59f68880374b
> src/sched/sched.cpp 042206853a68dd12c88801bd7bc6764b9b6b6c3f
> src/slave/http.cpp 62fbb37a1924062543bf9db4229704bdef91601d
> src/slave/main.cpp 750a12766bde64059bfd4635ea077cbd43cb4301
> src/slave/slave.hpp 68526f3fb01be6dc19d7e3c8e4ba8220b2c56ccb
> src/slave/slave.cpp 90575db0ed9ce0953745cc23460fbcda240c7c0e
> src/tests/allocator_tests.cpp b0beb72273e9d44a407d0e83e6c18a541b212089
> src/tests/authentication_tests.cpp 48a9323d03416ad9ee25fc19838d89678ff613bc
> src/tests/cluster.hpp bea395a73b09df287fef743dae1aa836f4dac691
> src/tests/fault_tolerance_tests.cpp
> d521f4be3e5297b7ab8f21f4173733bc2a2001c9
> src/tests/gc_tests.cpp 5459b78126d3607114ea171d3f76883f8355751c
> src/tests/isolator_tests.cpp ab5b00aa7f12dbd299debc1a405ac50c59cec85c
> src/tests/master_contender_detector_tests.cpp PRE-CREATION
> src/tests/master_detector_tests.cpp
> 06c586d29996cd599ae58dfd49cf1324aac0a6d6
> src/tests/master_tests.cpp bf790d24ff6b2fb199a04caebef523797b075e63
> src/tests/mesos.hpp ad39c095f9699fbe7958d52698fd2b0319e84eb9
> src/tests/mesos.cpp 351ffd614250847c52657b721a560eb2cf7c9443
> src/tests/slave_recovery_tests.cpp 37faf2054750080902cb06b62da9a9e2252c566f
>
> Diff: https://reviews.apache.org/r/13087/diff/
>
>
> Testing
> -------
>
> make check 100 times
>
>
> Thanks,
>
> Jiang Yan Xu
>
>
