> On March 3, 2016, 10:15 p.m., Joseph Wu wrote: > > src/master/contender.hpp, lines 17-18 > > <https://reviews.apache.org/r/44288/diff/3/?file=1279703#file1279703line17> > > > > This whole file seems like a pretty substantial change. I'd recommend > > pulling it out into a separate review (rather than hiding it in this > > review). > > > > Also, you'll want to consider making folders "contenders" and > > "detectors". Then renaming this file "standalone.hpp". > > Anurag Singh wrote: > Ok. Although then zookeeper's classes should also go into their own files. > > Anurag Singh wrote: > Looking at this again, the changes in this file are really just namespace > changes (the only thing substantial is the removal of the MasterContender > class definition) - I can't put them into a different commit without breaking > the build (I'm trying to make sure individual commits don't break builds, > which I think is a sensible goal). However, I can leave this commit as is but > create a separate commit to create the contenders/detectors directories. Is > that acceptable to you? > > Anurag Singh wrote: > Ping ... will the suggestion I made work for you? > > Joseph Wu wrote: > Don't worry about having atomic commits (especially for verbose changes). > > There are a couple of changes to consider here, each of which might > deserve its own review: > - Pulling out the interface deletions. (You also consider pulling it > into the previous review, as you're really moving the code from private to > public headers.) > - Adding a folder for each module you are adding ("contender" and > "detector"). > - Breaking apart the Standalone vs Zookeeper logic. > > Anurag Singh wrote: > While separating the zookeeper and standalone detectors and moving them > into detectors, I realized that the functions in the 'promises' namespace > will be duplicated unless I move them into their own header file. I can > either put them in include/mesos and change the namespace to mesos > mesos::internal::promises, or just leave the header file in detectors. Which > one is preferable? bmahler had a todo for doing this so I'm not sure if your > team already has some work planned in this area. > > Joseph Wu wrote: > I would *tentatively* recommend putting those helpers inside > `3rdparty/libprocess/include/process/collect.hpp`, where we have a bunch of > other helpers that operate on groups of `Futures`. We definitely want those > helpers in a libprocess header somewhere, but I'm not sure what the most > acceptable place would be. (I'll ask BenM about it.) > > Anurag Singh wrote: > The logic separation change has turned into a major change - if you see a > lot of issues with the commits, we should spin the separation change into an > issue by itself (I can work on that if needed). It will allow this review to > proceed faster - unfortunately we're blocked on our module changes until this > review completes.
Hi ... have you had a chance to take another look? - Anurag ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/44288/#review121944 ----------------------------------------------------------- On March 10, 2016, 11:45 p.m., Anurag Singh wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/44288/ > ----------------------------------------------------------- > > (Updated March 10, 2016, 11:45 p.m.) > > > Review request for mesos and Benjamin Hindman. > > > Bugs: MESOS-4610 > https://issues.apache.org/jira/browse/MESOS-4610 > > > Repository: mesos > > > Description > ------- > > Also modified users of MasterContender and MasterDetector to use this > namespace. > > > Diffs > ----- > > include/mesos/scheduler.hpp 14c7ff964aa7b94f439d16e605380661d2279d54 > include/mesos/v1/scheduler.hpp 765935e97b6c1686ab464a5cf1cf2dfd816f51f1 > src/cli/resolve.cpp 257e29034abf32491511f9a4e476b6859714829d > src/local/local.cpp 359fc54d7c4081f536a8de8b1dfcde413d75c9a9 > src/master/contender.cpp 9ad49ce10439fb41d78d52eaa4c1e6b9c5c7f735 > src/master/detector.cpp 9274435802d6292b183be48f42b43999476e016e > src/master/main.cpp 4263110c9b889984ef74eb94fed629958f2abd79 > src/master/master.hpp ea26670e6c6c67314406fded510e8fdd46053dc8 > src/master/master.cpp 57ff4a39039f573b8586bc03f873f97826b97f6f > src/sched/sched.cpp 525255eec808c3fe5c0e38b3d1a2086bbd4eb171 > src/scheduler/scheduler.cpp b010a819132fb80810e7f8ce96778109f2e8b35e > src/slave/main.cpp e3a4d13ddaeb89ba01c9b2ddfc72c37934f753eb > src/slave/slave.hpp 7520cc356e2b1b7f5fff15f33071a46a7b05e762 > src/tests/authentication_tests.cpp 85f14c3d453ca5aeffa1c915f38fe3031c2cf712 > src/tests/cluster.hpp 99a785ab0d4ee1a1e745202d2551de58a7631a85 > src/tests/cluster.cpp 084fb1ce37a315c561c4587c4761c870f54c8625 > src/tests/fault_tolerance_tests.cpp > d193897e636efd0e3ef67bf67fcd6255a3de0341 > src/tests/master_allocator_tests.cpp > cba7c36471f93b678d94e1da0251a28a893696b1 > src/tests/master_authorization_tests.cpp > 29c89fb11da792c3e71eb880a19657ea225b3cc8 > src/tests/master_contender_detector_tests.cpp > 255ab8119a04b55bb4f1b61dee19c4be64499376 > src/tests/master_slave_reconciliation_tests.cpp > d41178eb41df519073fc0890c5716bbc9fed6ad2 > src/tests/master_tests.cpp 2f4d820e223a48700ce1ac3a91b7256cc836c268 > src/tests/oversubscription_tests.cpp > e528476cd83b0e3f7ae8cea7d86dfabc1f66484e > src/tests/partition_tests.cpp 3776a0a104582f60b9f19ea58b011485194399b9 > src/tests/persistent_volume_tests.cpp > bf19c81fbcf973d1ac27fbd42eedfd7118b7ba50 > src/tests/reconciliation_tests.cpp 97112c4d64c75a16fdd7bbefd517a039fbf55b64 > src/tests/reservation_tests.cpp d7f9de6f2bce061316916260f356efdb96ecd482 > src/tests/scheduler_event_call_tests.cpp > 8c02ceeb3ec1783cb2f63f100700508e70f586e4 > src/tests/scheduler_http_api_tests.cpp > dfb0f51fec67a3951e396eab28eedb0dbf9493ae > src/tests/scheduler_tests.cpp fa42fb42f2d18060a867ade547cebbdcaead07d4 > src/tests/slave_recovery_tests.cpp bd7b94f3f1fac6705e5bf14c6f6103b540cde56c > src/tests/slave_tests.cpp 124e9587180f2a55e659d966d1c9060234c19457 > > Diff: https://reviews.apache.org/r/44288/diff/ > > > Testing > ------- > > See https://reviews.apache.org/r/44289/. > > > Thanks, > > Anurag Singh > >
