> On Feb. 18, 2016, 12: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`.) > > Mark Cavage wrote: > 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.
We generally break apart commits to make them easier to review. In this case, we will probably want to take advantage of your refactor to revisit the interface; and make sure we don't have an interface that is too specific for our two implementations. We might do this by sending out the patch (ideally with just the interface) out to the wider community. --- It may also be good to add the modules-expert @karya to this review. > On Feb. 18, 2016, 12: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. > > Mark Cavage wrote: > 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." IMO, it is not preferable to conflate the "type" and "mechanism". --- Some food for thought: When someone goes to load a custom Contender/Detector module, they will have an entirely different way of specifying the "mechanism". They would specify a new flag that tells the master which module to load, like `--master_contender="org_apache_mesos_ContenderThingy"`, and a module configuration flag `--modules=file:///path/to/modules.json`, which points to a file like: ``` { "libraries": [ { "file": "/path/to/module/lib/lib_leader_election.so", "modules": [ { "name": "org_apache_mesos_ContenderThingy", "parameters": [ { "key": "mechanism", "value": "zk://.../mesos" }, { "key": "other_custom_module_parameters", "value": "foo" }, ... ] } ] } ] } ``` Notice that any configuration of said module now happens in the "parameters". The old `--zk` flag would essentially be a special override. - Joseph ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/43269/#review119715 ----------------------------------------------------------- On Feb. 19, 2016, 10:52 a.m., Mark Cavage wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/43269/ > ----------------------------------------------------------- > > (Updated Feb. 19, 2016, 10:52 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 27aec37 > 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 > >
