> On March 3, 2016, 2: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?

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.


> On March 3, 2016, 2:15 p.m., Joseph Wu wrote:
> > src/master/main.cpp, line 61
> > <https://reviews.apache.org/r/44288/diff/3/?file=1279707#file1279707line61>
> >
> >     What about this header?
> 
> Anurag Singh wrote:
>     Do you mean why is this header not "<mesos/master/detector.hpp>"? It's 
> different from <mesos/master/detector.hpp> - it includes the 
> StandaloneMasterDetector and ZooperZooKeeperMasterDetector class definitions. 
> Although, after adding the contender and detect directories, these includes 
> will need to change.

After the module change, shouldn't all internal inclusions of "detector.hpp" 
and "contender.hpp" headers disappear?  I'm just wondering why you kept it 
here.  (And if you still need them here, there's probably more refactoring that 
needs to be done in the file than just a re-namespacing).


- Joseph


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


On March 3, 2016, 9:30 a.m., Anurag Singh wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44288/
> -----------------------------------------------------------
> 
> (Updated March 3, 2016, 9:30 a.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 a0fb73be2178171497dcce5c497ca38c3002f667 
>   src/cli/resolve.cpp 257e29034abf32491511f9a4e476b6859714829d 
>   src/local/local.cpp 359fc54d7c4081f536a8de8b1dfcde413d75c9a9 
>   src/master/contender.hpp 3fd20f8e94daab349b76d8f5ecc87398a187a847 
>   src/master/contender.cpp 9ad49ce10439fb41d78d52eaa4c1e6b9c5c7f735 
>   src/master/detector.hpp eb5d2a90b60c629150ddf04acf00f0edca1ca723 
>   src/master/detector.cpp 9274435802d6292b183be48f42b43999476e016e 
>   src/master/main.cpp 4263110c9b889984ef74eb94fed629958f2abd79 
>   src/master/master.hpp 13c6ff153e77c527822309e787942eb463d59e7d 
>   src/master/master.cpp f242b70a0ebeff9cd3204343735b8615230a6108 
>   src/sched/sched.cpp 525255eec808c3fe5c0e38b3d1a2086bbd4eb171 
>   src/scheduler/scheduler.cpp 7ea1c2567f37a73160bca346a25bb2f0c54e71a0 
>   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 
> f2b8dba809518cf716b2b5a7a6a8a5fe62e57646 
>   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 36595772b34bcb8d37dbc74d247bdf4614f10150 
>   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 
> 428e12646d80b45daec30cfe607b97f36170fdf5 
>   src/tests/scheduler_tests.cpp 2b1693eaf1a6106f5e7d269e4e3f6c353dd6f017 
>   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
> 
>

Reply via email to