> 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 > >
