> On Oct. 30, 2015, 9:43 p.m., Ben Mahler wrote: > > Hm.. we're still relying on the update uuid, shouldn't we be trying to move > > off of it onto the status uuid?
As mentioned in the comments, we can't yet remove uuid because of old checkpointed updates :( > On Oct. 30, 2015, 9:43 p.m., Ben Mahler wrote: > > src/master/master.cpp, line 4365 > > <https://reviews.apache.org/r/39792/diff/1/?file=1112647#file1112647line4365> > > > > Just a side note, it would be great to have a status update benchmark > > for throughput, since we're introducing an extra copy of the status update > > here (which might be expensive for large updates). Ideally libprocess could > > move construct this 'update' field (but it doesn't support this currently). agreed. added a TODO for now. will hopefully follow up on a review soon with a benchmark test. > On Oct. 30, 2015, 9:43 p.m., Ben Mahler wrote: > > src/sched/sched.cpp, line 903 > > <https://reviews.apache.org/r/39792/diff/1/?file=1112648#file1112648line903> > > > > Mind adding a newline to separate the TODO from the rest of the > > comment? I find that clearer to read, especially when the TODO is at the > > top and it becomes ambiguous where a multi-line TODO ends and the comment > > starts. done. > On Oct. 30, 2015, 9:43 p.m., Ben Mahler wrote: > > src/sched/sched.cpp, lines 903-904 > > <https://reviews.apache.org/r/39792/diff/1/?file=1112648#file1112648line903> > > > > Hm.. why wasn't it enough that the slave was setting it? I'm guessing > > the concern was due to the old checkpointed updates per your change below? > > Seems helpful to spell out the slave side of this in the comment. removed the mention of slave because master is the only one that sends updates the driver. > On Oct. 30, 2015, 9:43 p.m., Ben Mahler wrote: > > src/slave/slave.cpp, lines 3025-3027 > > <https://reviews.apache.org/r/39792/diff/1/?file=1112649#file1112649line3025> > > > > Hm.. could you reduce jaggedness here? I like how you formatted your > > comment in master.cpp above, easy on my eyes. done. as an aside, there should be a way to automate the jaggedness, otherwise it is tedious to get it right. the master.cpp jagedness was coincidental, i was just wrapping them up at 80. > On Oct. 30, 2015, 9:43 p.m., Ben Mahler wrote: > > src/slave/slave.cpp, lines 3028-3029 > > <https://reviews.apache.org/r/39792/diff/1/?file=1112649#file1112649line3028> > > > > Hm.. not immediately obvious to me why we set the status uuid from the > > update uuid, should we spell that out here? expanded the comment. - Vinod ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/39792/#review104622 ----------------------------------------------------------- On Oct. 30, 2015, 12:23 a.m., Vinod Kone wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/39792/ > ----------------------------------------------------------- > > (Updated Oct. 30, 2015, 12:23 a.m.) > > > Review request for mesos, Anand Mazumdar and Ben Mahler. > > > Repository: mesos > > > Description > ------- > > This just makes sure master and slave properly set the uuid in task status to > setup the stage for deprecating the messy logic in scheduler driver in a > future release. > > > Diffs > ----- > > src/master/master.hpp b76d30197b3decda0a742e03ce01a17a64b633ac > src/master/master.cpp 9f4586e668a2141f4937497d42853fbdea7751a5 > src/sched/sched.cpp 9c5e3b8e42605f4647d897ba02ea3a17e494f355 > src/slave/slave.cpp 5f9b52b41eaab0c24965f28e192074340e00bde5 > > Diff: https://reviews.apache.org/r/39792/diff/ > > > Testing > ------- > > make check > > > Thanks, > > Vinod Kone > >
