mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Ben Mahler" <benjamin.mah...@gmail.com>
Subject Re: Review Request 39792: Updated master and slave to properly set task status uuid.
Date Fri, 30 Oct 2015 21:43:06 GMT

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/39792/#review104622
-----------------------------------------------------------


Hm.. we're still relying on the update uuid, shouldn't we be trying to move off of it onto
the status uuid?


src/master/master.cpp (line 4365)
<https://reviews.apache.org/r/39792/#comment162847>

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



src/sched/sched.cpp (line 903)
<https://reviews.apache.org/r/39792/#comment162848>

    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.



src/sched/sched.cpp (lines 903 - 904)
<https://reviews.apache.org/r/39792/#comment162849>

    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.



src/slave/slave.cpp (lines 3025 - 3027)
<https://reviews.apache.org/r/39792/#comment162850>

    Hm.. could you reduce jaggedness here? I like how you formatted your comment in master.cpp
above, easy on my eyes.



src/slave/slave.cpp (lines 3028 - 3029)
<https://reviews.apache.org/r/39792/#comment162851>

    Hm.. not immediately obvious to me why we set the status uuid from the update uuid, should
we spell that out here?


- Ben Mahler


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


Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message