> On March 6, 2014, 7:52 p.m., Benjamin Hindman wrote: > > src/tests/cluster.hpp, line 108 > > <https://reviews.apache.org/r/18370/diff/5/?file=507531#file507531line108> > > > > I'm late to this party, but why did we change this to be called > > MasterInfo? This clashes with another type we have called MasterInfo and > > upon a cursory look seems unnecessary. Moreover if we needed this change we > > didn't change the Slave struct below to be called SlaveInfo for consistency. > > Vinod Kone wrote: > Charlie can you follow up on benh's comment by sending a new patch? You > can resolve the above issue once you do so. Thanks.
Hey Ben - first I'm agreeable and I'm happy to make the corresponding change to Slave, but I think we need to decide on what the blessed change IS. I'll give you the history ------>begin history I sent the review, w/ no name change and it confused Dominic b/c we have a Master class which REALLY represents the resources allocated by the Start method (and therefore that need to be released by the Stop method). It's confusing, b/c you have Cluster::Masters::Master which itself has a member which is of type mesos::internal::Master. so, in the same methods, you have two different Master types and in fact assign one to a member of the other. anyways, I explained to Dominic what was going on and suggested naming it MasterDependents since that's more accurately what it is. I didn't love that name, but no one suggested a better one, at the time. Vinod saw that patch and suggested MasterInfo. <-----end of history 1. agreed that whatever we do should be consistent between Master & Slave "dependents" objects 2. agreed that MasterInfo isn't really descriptive 3. personally, I prefer anything besides Master (or Slave), b/c Master already is a well-known concept, and it's being used SxS in the same method. 4. from a design pov, I'm inclined to say that this shouldn't even be a struct. Ideally, it's a simple "list of things to cleanup". I'll need to verify that is safe to do (ordering) and I'm not sure how much boiler-plating it will require. IOW, if all these guys inherited from IDeleteMe, then I'd make a list<IDeleteMe*> and as we allocated things we'd push them onto the list and delete them in reverse order. since there's no common super class we'll need some template magic (at a minimum boost::variant with a visitor to do the delete). It's possible that boost has something better & I will take a look. so, possible fixes in order of my preference: 1) switch to "list of things to delete" (will send a code review unless someone says don't bother) 2) change MasterInfo to better represent "Resources Allocated by Start that need to be deleted by Stop" (again, a name suggestion is very welcome here :-) ). whatever we pick, make Slave match. 3) leave MasterInfo and change Slave -> SlaveInfo 4) go back to Master & Slave (and document at the declaration what it's for and at the use points that it's not actual master) Make sense? again, I'm fine to do any of the above (and in fact, at this point I've done 4, 2, 3 :-) ) I just don't want a concrete decision. if you feel strongly let me know and that's what I'll do. - Charlie ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/18370/#review36415 ----------------------------------------------------------- On March 1, 2014, 1:15 a.m., Charlie Carson wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/18370/ > ----------------------------------------------------------- > > (Updated March 1, 2014, 1:15 a.m.) > > > Review request for mesos and Vinod Kone. > > > Bugs: MESOS-1024 > https://issues.apache.org/jira/browse/MESOS-1024 > > > Repository: mesos-git > > > Description > ------- > > Refactor Cluster::Master::start methods > > There are currently two overloads of the Cluster::Masters::start > function. One takes a argument of AllocatorProcess* and the other > does not. The AllocatorProcess* overload serves two purposes: > 1) it allows an alternative implementation of AllocatorProcess to be > passed (i.e. a mock) > 2) it changes the destruction timing so that the passed in argument > can outlive the master. > > Beyond that, the two functions are identical. This changes the > parameter to be Option<AllocatorProcess*> and allows all the logic > to be in one method. The old function exists for back-compat but > now simply forward by passing in None() to the other function. > > This is for two purposes: > 1) reduce code duplication > 2) position the code so that we can also optionally pass in a mock > repaier. > > Review: https://reviews.apache.org/r/18370 > > > Diffs > ----- > > src/tests/cluster.hpp d1bf680ed3f6a0fb16af85a21409d653d44522ca > > Diff: https://reviews.apache.org/r/18370/diff/ > > > Testing > ------- > > make check > > > Thanks, > > Charlie Carson > >
