Re: Review Request 39792: Updated master and slave to properly set task status uuid.

[Vinod Kone <vi...@gmail.com>]

> 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
> 
>