> 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. > > Charlie Carson wrote: > 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 Carson wrote: > I just DO want a concrete decision :-) > > Benjamin Hindman wrote: > Awesome, in the interest of not adding too much more here, I think the > name Dependents really wasn't that bad! It's explicitly descriptive to say > the least! > > The reason a struct existed was because originally we wanted to be able > to expose different "components" of a single started master (or slave), so we > would keep everything around. Eventually this became just the list of things > to cleanup. > > I think in the future with C++11 we'll be able to do the deleting even > easier than making things inherit from an IDeleteMe interface, so I'd prefer > not to do all that for now.
Hey Charlie. Mind opening a new review for the changes since this is already submitted? Thanks. - Vinod ----------------------------------------------------------- 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 > >
