mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Mark Cavage <mcav...@gmail.com>
Subject Re: Review Request 43269: MasterContender/MasterDetector loadable as modules.
Date Fri, 19 Feb 2016 18:51:26 GMT


> On Feb. 18, 2016, 8:56 p.m., Joseph Wu wrote:
> > 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`.)

By different groups, do you mean go undo this commit and break it up into new commits that
does that set of things "step by step"? If so, why? This whole change is essentially a glorified
refactoring, so I don't see how that would help anything.

As to the separate modules, I had considered making a single module, but ultimately left it
as-is since the slave doesn't use Contender, as you point out. Additionally, "internally"
the code was already factored to two interfaces, and (1) I wanted to make the most surgical
change I could, and (2) I sort of assumed the code base was designed this way for a reason,
so I wouldn't go collapse it.


> On Feb. 18, 2016, 8:56 p.m., Joseph Wu wrote:
> > src/master/detector.cpp, lines 204-205
> > <https://reviews.apache.org/r/43269/diff/4/?file=1254961#file1254961line204>
> >
> >     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.

I had considered making the ZK detector it's own module, but again, erred on the side of minimally
invasive code change that enabled people to write their own. As-is, the code basically acts
exactly like it did before, with the caveat that if Standalone/ZK aren't specified, then now
it falls through to new behavior, which is to load an external module.


> On Feb. 18, 2016, 8:56 p.m., Joseph Wu wrote:
> > include/mesos/master/contender.hpp, lines 41-47
> > <https://reviews.apache.org/r/43269/diff/4/?file=1254948#file1254948line41>
> >
> >     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.

Ok, for now I renamed it to "type" and stripped out the comments in the header. Note that
this is backwards compatible with the existing codebase. My thought was that the core process
should continue to do what it does today, and if those all fallthrough, then try to load a
plugin. This should be the least disruptive behavior (imo), and module authors don't need
to worry about anything else "legacy."


- Mark


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


On Feb. 18, 2016, 7:51 p.m., Mark Cavage wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/43269/
> -----------------------------------------------------------
> 
> (Updated Feb. 18, 2016, 7:51 p.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
> 
>


Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message