mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Anurag Singh <anurag.prakash.si...@gmail.com>
Subject Re: Review Request 44288: Changed MasterDetector/Contender namespace.
Date Wed, 09 Mar 2016 01:50:12 GMT


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

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.


- Anurag


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


On March 9, 2016, 1:35 a.m., Anurag Singh wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44288/
> -----------------------------------------------------------
> 
> (Updated March 9, 2016, 1:35 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 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
> 
>


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