----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/2763/#review3214 -----------------------------------------------------------
src/master/http.cpp <https://reviews.apache.org/r/2763/#comment7137> 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). src/master/http.cpp <https://reviews.apache.org/r/2763/#comment7138> 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). src/master/http.cpp <https://reviews.apache.org/r/2763/#comment7143> Kill tab. src/master/http.cpp <https://reviews.apache.org/r/2763/#comment7144> 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! src/master/http.cpp <https://reviews.apache.org/r/2763/#comment7145> Kill tab. src/master/http.cpp <https://reviews.apache.org/r/2763/#comment7146> s/completedFrameworks/completed_frameworks src/master/master.hpp <https://reviews.apache.org/r/2763/#comment7148> 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. src/master/master.hpp <https://reviews.apache.org/r/2763/#comment7149> Space between 'if' and '('. src/master/master.hpp <https://reviews.apache.org/r/2763/#comment7155> Probably a memory leak here ... this is where the Task object is supposed to get deallocated right? src/master/master.hpp <https://reviews.apache.org/r/2763/#comment7139> s/completedTime/unregisteredTime src/master/master.hpp <https://reviews.apache.org/r/2763/#comment7151> Let's stick this in master/constants.hpp too. src/master/master.cpp <https://reviews.apache.org/r/2763/#comment7153> Memory leak. src/master/master.cpp <https://reviews.apache.org/r/2763/#comment7154> Update this comment since you are no longer deallocating the memory. src/master/master.cpp <https://reviews.apache.org/r/2763/#comment7157> 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? src/webui/master/index.tpl <https://reviews.apache.org/r/2763/#comment7160> Why was this necessary? src/webui/master/index.tpl <https://reviews.apache.org/r/2763/#comment7164> 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. src/webui/master/index.tpl <https://reviews.apache.org/r/2763/#comment7166> Probably don't need that right? - Benjamin On 2011-11-08 00:36:07, Thomas Marshall wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/2763/ > ----------------------------------------------------------- > > (Updated 2011-11-08 00:36:07) > > > 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. > > > 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 > >
