You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@mesos.apache.org by Ben Mahler <be...@gmail.com> on 2013/11/01 19:17:31 UTC
Re: Review Request 15114: Small cleanups of the Master code.
-----------------------------------------------------------
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.
Changes
-------
Added 'Depends On' field.
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
Re: Review Request 15114: Small cleanups of the Master code.
Posted by 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
>
>
Re: Review Request 15114: Small cleanups of the Master code.
Posted by Vinod Kone <vi...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/15114/#review28427
-----------------------------------------------------------
src/master/master.cpp
<https://reviews.apache.org/r/15114/#comment55246>
Just make this a no-op.
src/master/master.cpp
<https://reviews.apache.org/r/15114/#comment55247>
delete whitelistWatcher.
src/master/master.cpp
<https://reviews.apache.org/r/15114/#comment55250>
Can you use protobuf::createStatusUpdate()?
src/master/master.cpp
<https://reviews.apache.org/r/15114/#comment55251>
How about a return here too?
src/master/master.cpp
<https://reviews.apache.org/r/15114/#comment55252>
if (slave == NULL) {
LOG...
return;
}
src/master/master.cpp
<https://reviews.apache.org/r/15114/#comment55253>
Why did you pull this out when it's used only once?
src/master/master.cpp
<https://reviews.apache.org/r/15114/#comment55254>
How is slave->executors.contains(frameworkId) not possible? Didn't you just get the key above?
- Vinod Kone
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
>
>
Re: Review Request 15114: Small cleanups of the Master code.
Posted by Ben Mahler <be...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/15114/
-----------------------------------------------------------
(Updated Feb. 14, 2014, 11:50 p.m.)
Review request for mesos, Benjamin Hindman and Vinod Kone.
Changes
-------
Formatting and better logging from Vinod's review. NNFR.
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 (updated)
-----
src/master/master.hpp 737bd8b5a1cd88a98a7f094795c50547079921ba
src/master/master.cpp a4e1b1f7327f01eb16f14c3fa08fc7a62048d36c
Diff: https://reviews.apache.org/r/15114/diff/
Testing
-------
make check
Thanks,
Ben Mahler
Re: Review Request 15114: Small cleanups of the Master code.
Posted by Vinod Kone <vi...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/15114/#review34540
-----------------------------------------------------------
Ship it!
src/master/master.cpp
<https://reviews.apache.org/r/15114/#comment64649>
Indentation.
src/master/master.cpp
<https://reviews.apache.org/r/15114/#comment64652>
Can you actually say "Shutting down slave ..."
I think that would be more clear.
src/master/master.cpp
<https://reviews.apache.org/r/15114/#comment64653>
ditto.
- Vinod Kone
On Feb. 11, 2014, 2:14 a.m., Ben Mahler wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/15114/
> -----------------------------------------------------------
>
> (Updated Feb. 11, 2014, 2:14 a.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 7649737283f5f5d786ac40504e943a3be4c1d62b
> src/master/master.cpp 4d9a9d15b38e021adc04c34b2e86734567bb957e
>
> Diff: https://reviews.apache.org/r/15114/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Ben Mahler
>
>
Re: Review Request 15114: Small cleanups of the Master code.
Posted by Ben Mahler <be...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/15114/
-----------------------------------------------------------
(Updated Feb. 11, 2014, 2:14 a.m.)
Review request for mesos, Benjamin Hindman and Vinod Kone.
Changes
-------
Rebased & Vinod's review.
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 (updated)
-----
src/master/master.hpp 7649737283f5f5d786ac40504e943a3be4c1d62b
src/master/master.cpp 4d9a9d15b38e021adc04c34b2e86734567bb957e
Diff: https://reviews.apache.org/r/15114/diff/
Testing
-------
make check
Thanks,
Ben Mahler