> On 2011-11-14 20:19:11, Benjamin Hindman wrote: > > src/master/http.cpp, line 80 > > <https://reviews.apache.org/r/2763/diff/1/?file=56809#file56809line80> > > > > I know this isn't your stuff, but let's change this to registered_time > > (update everywhere else as appropriate) and let's also add > > reregistered_time (which will only be the last reregister, but that is > > okay).
Done. > On 2011-11-14 20:19:11, Benjamin Hindman wrote: > > src/master/http.cpp, line 81 > > <https://reviews.apache.org/r/2763/diff/1/?file=56809#file56809line81> > > > > Now let's change this to unregistered_time. And as long as we are here, > > let's go ahead and add whether or not this framework is active too > > (framework.active). Done. > On 2011-11-14 20:19:11, Benjamin Hindman wrote: > > src/master/http.cpp, line 97 > > <https://reviews.apache.org/r/2763/diff/1/?file=56809#file56809line97> > > > > Kill tab. Done. > On 2011-11-14 20:19:11, Benjamin Hindman wrote: > > src/master/http.cpp, line 100 > > <https://reviews.apache.org/r/2763/diff/1/?file=56809#file56809line100> > > > > Let's use snake case here ( s/completedTasks/completed_tasks) just to > > be consistent with the rest of the JSON we are sending. If there is some > > reason to be using camel case instead (JSON standards, best practices, etc) > > let us know but then let's change it all! Done. > On 2011-11-14 20:19:11, Benjamin Hindman wrote: > > src/master/http.cpp, line 259 > > <https://reviews.apache.org/r/2763/diff/1/?file=56809#file56809line259> > > > > Kill tab. Done. > On 2011-11-14 20:19:11, Benjamin Hindman wrote: > > src/master/http.cpp, line 262 > > <https://reviews.apache.org/r/2763/diff/1/?file=56809#file56809line262> > > > > s/completedFrameworks/completed_frameworks Done. > On 2011-11-14 20:19:11, Benjamin Hindman wrote: > > src/master/master.hpp, line 218 > > <https://reviews.apache.org/r/2763/diff/1/?file=56810#file56810line218> > > > > Since this is effectively a constant for now, lets stick it in > > master/constants.hpp as MAX_COMPLETED_FRAMEWORKS. You can add a TODO there > > to make it configurable later. Done. > On 2011-11-14 20:19:11, Benjamin Hindman wrote: > > src/master/master.hpp, line 402 > > <https://reviews.apache.org/r/2763/diff/1/?file=56810#file56810line402> > > > > Space between 'if' and '('. Done. > On 2011-11-14 20:19:11, Benjamin Hindman wrote: > > src/webui/master/index.tpl, line 101 > > <https://reviews.apache.org/r/2763/diff/1/?file=56813#file56813line101> > > > > Why was this necessary? I think this was an artifact left over from the fact that I started working on an old version. Done. > On 2011-11-14 20:19:11, Benjamin Hindman wrote: > > src/webui/master/index.tpl, line 240 > > <https://reviews.apache.org/r/2763/diff/1/?file=56813#file56813line240> > > > > Again, what's going on here? If I look above at model(const Offer& > > offer) I don't see any code that is adding a 'slaves' field. Done. > On 2011-11-14 20:19:11, Benjamin Hindman wrote: > > src/webui/master/index.tpl, lines 281-282 > > <https://reviews.apache.org/r/2763/diff/1/?file=56813#file56813line281> > > > > Probably don't need that right? Done. > On 2011-11-14 20:19:11, Benjamin Hindman wrote: > > src/master/master.hpp, line 478 > > <https://reviews.apache.org/r/2763/diff/1/?file=56810#file56810line478> > > > > s/completedTime/unregisteredTime Done. > On 2011-11-14 20:19:11, Benjamin Hindman wrote: > > src/master/master.hpp, line 483 > > <https://reviews.apache.org/r/2763/diff/1/?file=56810#file56810line483> > > > > Let's stick this in master/constants.hpp too. Done. > On 2011-11-14 20:19:11, Benjamin Hindman wrote: > > src/master/master.cpp, line 1765 > > <https://reviews.apache.org/r/2763/diff/1/?file=56811#file56811line1765> > > > > What happens if we ever set MAX_COMPLETED_TASKS to be 0? Will the task > > get deleted after calling removeTask suck that we'll segfault when we try > > and use the task? Try and think about the ownership here. It was more > > explicit with the delete below, but now the ownership is getting > > transferred. Maybe it would be easier to store a copy of the completed Task? Andy and I agreed that the best solution to this (and to the memory leak problems) was to make a copy of the frameworks and tasks before storing them, and store the new objects themselves rather than pointers. > On 2011-11-14 20:19:11, Benjamin Hindman wrote: > > src/master/master.cpp, line 1578 > > <https://reviews.apache.org/r/2763/diff/1/?file=56811#file56811line1578> > > > > Update this comment since you are no longer deallocating the memory. We're making a copy and deleting it now, so this isn't applicable. > On 2011-11-14 20:19:11, Benjamin Hindman wrote: > > src/master/master.cpp, line 1575 > > <https://reviews.apache.org/r/2763/diff/1/?file=56811#file56811line1575> > > > > Memory leak. Done. > On 2011-11-14 20:19:11, Benjamin Hindman wrote: > > src/master/master.hpp, line 403 > > <https://reviews.apache.org/r/2763/diff/1/?file=56810#file56810line403> > > > > Probably a memory leak here ... this is where the Task object is > > supposed to get deallocated right? Done. - Thomas ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/2763/#review3214 ----------------------------------------------------------- On 2011-11-14 23:03:35, Thomas Marshall wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/2763/ > ----------------------------------------------------------- > > (Updated 2011-11-14 23:03:35) > > > Review request for mesos and Andy Konwinski. > > > Summary > ------- > > Frameworks are no longer deleted when they are done executing; they are saved > in a list in the master, which is then displayed on the webui. Tasks are > similarly stored in their framework. > > > This addresses bug MESOS-8. > https://issues.apache.org/jira/browse/MESOS-8 > > > Diffs > ----- > > src/master/http.cpp 47caf48 > src/master/master.hpp fdacf36 > src/master/master.cpp b6cfde7 > src/webui/master/framework.tpl 0f41349 > src/webui/master/index.tpl 16e3446 > > Diff: https://reviews.apache.org/r/2763/diff > > > Testing > ------- > > > Thanks, > > Thomas > >
