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

2015-11-05 Thread Vinod Kone 

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

(Updated Nov. 5, 2015, 3:38 p.m.)


Review request for mesos, Anand Mazumdar and Ben Mahler.


Changes
---

benm's comments. NNFR.


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 (updated)
-

  src/master/master.hpp b19f23be135d191ce119cf87fd25c4d6746faed3 
  src/master/master.cpp cbae27e7a4059a72bc69e152ec8adaf4ef725965 
  src/sched/sched.cpp 4ec163315bb5b6a3d6f511d61863fbbff3254546 
  src/slave/slave.cpp ddeece4334b13cf8b5ab7d87012bbf946640025b 

Diff: https://reviews.apache.org/r/39792/diff/


Testing
---

make check


Thanks,

Vinod Kone

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

2015-11-05 Thread Vinod Kone 


> On Nov. 5, 2015, 1:41 a.m., Ben Mahler wrote:
> > src/master/master.cpp, lines 4410-4418
> > 
> >
> > Would be great to continue wrapping comment paragraphs at 70 even 
> > though it's not a hard rule, for consistency with the large amount of 
> > existing comment paragraphs and to make it easier to read.

>From http://www.mail-archive.com/dev%40mesos.apache.org/msg32944.html it 
>looked like we agreed to wrap all new and updated comments around 80 chars? 
>I'll make sure they are not jagged.


- Vinod


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


On Nov. 2, 2015, 7:32 p.m., Vinod Kone wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39792/
> ---
> 
> (Updated Nov. 2, 2015, 7:32 p.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
> 
>

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

2015-11-04 Thread Ben Mahler 

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

Ship it!



src/master/master.cpp (lines 4410 - 4418)


Would be great to continue wrapping comment paragraphs at 70 even though 
it's not a hard rule, for consistency with the large amount of existing comment 
paragraphs and to make it easier to read.



src/master/master.cpp (lines 4417 - 4418)


What can be made a CHECK? That the uuids match? Also, how is 0.27.0 
relevant given the notes about old checkpointed updates above? Would be great 
to clarify this in the comment.



src/slave/slave.cpp (lines 3025 - 3028)


Ditto, would be great to wrap at 70 to be consistent with the rest of this 
function and the code in general. :)



src/slave/slave.cpp (lines 3025 - 3028)


Ah.. I see. It was a bit confusing to read your remarks about old 
checkpointed updates in the master code, can you remove that and perhaps just 
reference the invariants before and after 0.26.0? Was a bit confusing for me.


- Ben Mahler


On Nov. 2, 2015, 7:32 p.m., Vinod Kone wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39792/
> ---
> 
> (Updated Nov. 2, 2015, 7:32 p.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
> 
>

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

2015-11-02 Thread Vinod Kone 


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

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

2015-11-02 Thread Vinod Kone 

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

(Updated Nov. 2, 2015, 7:32 p.m.)


Review request for mesos, Anand Mazumdar and Ben Mahler.


Changes
---

updated comments. still needs a ship it.


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 (updated)
-

  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

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

2015-10-30 Thread Ben Mahler 

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


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)


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)


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)


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)


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

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

2015-10-29 Thread Vinod Kone 

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

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

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

2015-10-29 Thread Mesos ReviewBot 

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


Patch looks great!

Reviews applied: [39791, 39792]

All tests passed.

- Mesos ReviewBot


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