-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/43269/#review119715
-----------------------------------------------------------



Can you split up this patch into the following groups?  (Its ok to run the 
tests at the end of a review chain, just add a note in the "Testing Done" 
section.)

* Interfaces for the new module.
* Modularization boilerplate (i.e. include/mesos/module/*, makefile changes, 
test modules, src/module/manager.cpp)
* The refactoring changes (i.e. src/master/contender.* src/master/detector.*)
* All the trivial header/namespace changes.

---

A general question:

* Does it make sense to have a separate `Contender` and `Detector` module?  Or 
just one `LeaderElection` module?  (One argument for separate modules is that 
the Mesos Agent only uses the `Detector`.)


include/mesos/master/contender.hpp (lines 41 - 47)
<https://reviews.apache.org/r/43269/#comment181055>

    For a module, specifying the "mechanism" here doesn't make much sense.  It 
should expect the type of module (i.e. `"org_apache_mesos_TestContender"`).
    
    Same for the Detector.
    
    ---
    
    To retain backwards compatibility with the master flag `--zk` and agent 
flag `--master`, it may be appropriate to pass both the module type and the 
mechanism as arguments.



include/mesos/master/contender.hpp (line 69)
<https://reviews.apache.org/r/43269/#comment181056>

    Nit: s/> >/>>/



include/mesos/master/detector.hpp (line 60)
<https://reviews.apache.org/r/43269/#comment181057>

    Nit: s/> >/>>/



src/master/detector.cpp (lines 204 - 205)
<https://reviews.apache.org/r/43269/#comment181059>

    Consider breaking apart the logic here into separate modules.
    
    * The default behavior (no module specified, no ZK string) is to create a 
`StandaloneMasterDetector`.
    * When the ZK string is specified, you should create a 
`ZooKeeperMasterDetector` with that ZK string.
    * Otherwise, load the module.
    
    ---
    
    You may even want to consider breaking the Zookeeper logic into an actual 
module (which would be loaded by default when required).  This would serve as 
the "example" module that we usually provide when modularizing anything.


- Joseph Wu


On Feb. 18, 2016, 11:51 a.m., Mark Cavage wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/43269/
> -----------------------------------------------------------
> 
> (Updated Feb. 18, 2016, 11:51 a.m.)
> 
> 
> Review request for mesos and Benjamin Hindman.
> 
> 
> Bugs: MESOS-4610
>     https://issues.apache.org/jira/browse/MESOS-4610
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> MasterContender/MasterDetector loadable as modules.
> 
> 
> Diffs
> -----
> 
>   include/mesos/master/contender.hpp PRE-CREATION 
>   include/mesos/master/detector.hpp PRE-CREATION 
>   include/mesos/module/contender.hpp PRE-CREATION 
>   include/mesos/module/detector.hpp PRE-CREATION 
>   include/mesos/scheduler.hpp 14c7ff9 
>   src/Makefile.am 54ebe91 
>   src/cli/resolve.cpp 257e290 
>   src/examples/test_contender_module.cpp PRE-CREATION 
>   src/examples/test_detector_module.cpp PRE-CREATION 
>   src/local/local.cpp 359fc54 
>   src/master/contender.hpp 3fd20f8 
>   src/master/contender.cpp 9ad49ce 
>   src/master/detector.hpp eb5d2a9 
>   src/master/detector.cpp 9274435 
>   src/master/main.cpp 4263110 
>   src/master/master.hpp 2f2ad2a 
>   src/master/master.cpp e1ca81d 
>   src/module/manager.cpp 6ae9950 
>   src/sched/sched.cpp 525255e 
>   src/scheduler/scheduler.cpp 99a7d0d 
>   src/slave/main.cpp e3a4d13 
>   src/slave/slave.hpp ced835d 
>   src/tests/authentication_tests.cpp 85f14c3 
>   src/tests/cluster.hpp 99a785a 
>   src/tests/cluster.cpp 084fb1c 
>   src/tests/fault_tolerance_tests.cpp 982468f 
>   src/tests/master_allocator_tests.cpp cba7c36 
>   src/tests/master_authorization_tests.cpp 29c89fb 
>   src/tests/master_contender_detector_tests.cpp 255ab81 
>   src/tests/master_slave_reconciliation_tests.cpp d41178e 
>   src/tests/master_tests.cpp 393a6f5f 
>   src/tests/module.hpp 4b32f29 
>   src/tests/module.cpp 8cc305c 
>   src/tests/oversubscription_tests.cpp d4ae819 
>   src/tests/partition_tests.cpp c5badbe 
>   src/tests/persistent_volume_tests.cpp e169e1b 
>   src/tests/reconciliation_tests.cpp 97112c4 
>   src/tests/reservation_tests.cpp d2ef159 
>   src/tests/scheduler_event_call_tests.cpp bd8920f 
>   src/tests/scheduler_http_api_tests.cpp 9eb1de7 
>   src/tests/slave_recovery_tests.cpp e2a78a0 
>   src/tests/slave_tests.cpp c7f5a70 
> 
> Diff: https://reviews.apache.org/r/43269/diff/
> 
> 
> Testing
> -------
> 
> In addition to all unit tests passing, we are currently using this 
> functionality in our environment with a custom consensus stack. In our world, 
> we have a C++ plugin that calls out to an HTTP REST service (implemented in 
> Java/Scala, not that it matters).
> 
> 
> Thanks,
> 
> Mark Cavage
> 
>

Reply via email to