Re: Review Request 15114: Small cleanups of the Master code.

[Ben Mahler <be...@gmail.com>]

> On Nov. 7, 2013, 7:47 p.m., Vinod Kone wrote:
> > src/master/master.cpp, line 1552
> > <https://reviews.apache.org/r/15114/diff/2/?file=375378#file375378line1552>
> >
> >     Why did you pull this out when it's used only once?

This will be used by the Registrar integration and I wanted to minimize the amount of diff for that change since it's very critical code.


> On Nov. 7, 2013, 7:47 p.m., Vinod Kone wrote:
> > src/master/master.cpp, line 2772
> > <https://reviews.apache.org/r/15114/diff/2/?file=375378#file375378line2772>
> >
> >     How is slave->executors.contains(frameworkId) not possible? Didn't you just get the key above?

You're right, this becomes relevant only in a subsequent patch, so I'll remove it here.


> On Nov. 7, 2013, 7:47 p.m., Vinod Kone wrote:
> > src/master/master.cpp, line 1386
> > <https://reviews.apache.org/r/15114/diff/2/?file=375378#file375378line1386>
> >
> >     if (slave == NULL) {
> >       LOG...
> >       return;
> >     }

We don't return in the NULL case.


> On Nov. 7, 2013, 7:47 p.m., Vinod Kone wrote:
> > src/master/master.cpp, lines 1189-1196
> > <https://reviews.apache.org/r/15114/diff/2/?file=375378#file375378line1189>
> >
> >     Can you use protobuf::createStatusUpdate()?

If the slaveId was an Option in createStatusUpdate then I could. :)


- Ben


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


On Nov. 1, 2013, 6:17 p.m., Ben Mahler wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/15114/
> -----------------------------------------------------------
> 
> (Updated Nov. 1, 2013, 6:17 p.m.)
> 
> 
> Review request for mesos, Benjamin Hindman and Vinod Kone.
> 
> 
> Repository: mesos-git
> 
> 
> Description
> -------
> 
> This is to minimize the size of the diff I end up sending for the stateful master implementation. There should be no functional change here.
> 
> 
> Diffs
> -----
> 
>   src/master/master.hpp e377af8b3ccd932ae411fa2df4c19642a7310d02 
>   src/master/master.cpp 8e14a070e87ebe579b54d05fb1e8b286edb5e459 
> 
> Diff: https://reviews.apache.org/r/15114/diff/
> 
> 
> Testing
> -------
> 
> make check
> 
> 
> Thanks,
> 
> Ben Mahler
> 
>