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 2014/07/18 01:49:39 UTC
Re: Review Request 23661: Unified status update logging in the master.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/23661/
-----------------------------------------------------------
(Updated July 17, 2014, 11:49 p.m.)
Review request for mesos and Vinod Kone.
Changes
-------
Now uses forward() to unify all status update forwarding.
Summary (updated)
-----------------
Unified status update logging in the master.
Repository: mesos-git
Description (updated)
-------
All status updates in the master are now sent via Master::forward() which means they are all logged in the same format.
This should make debugging easier and it simplifies the Master code.
Diffs (updated)
-----
src/common/protobuf_utils.hpp 12ff00a2e1262d785dbd6ec4d54e50d88d611e20
src/master/master.hpp fa46a67fba0fc0a019cdb37770994eee0099d260
src/master/master.cpp fb2fd5a0288296370a034d57083cf22992306cea
Diff: https://reviews.apache.org/r/23661/diff/
Testing
-------
make check
Thanks,
Ben Mahler
Re: Review Request 23661: Unified status update logging in the master.
Posted by Ben Mahler <be...@gmail.com>.
> On July 18, 2014, 12:06 a.m., Vinod Kone wrote:
> > src/master/master.cpp, lines 3164-3167
> > <https://reviews.apache.org/r/23661/diff/2/?file=634901#file634901line3164>
> >
> > It is a bit confusing to know when to call this overload vs the other. Why not just have the above one?
I've updated this to only have one forward() so that callers are responsible for looking up the framework if necessary.
- Ben
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/23661/#review48090
-----------------------------------------------------------
On July 18, 2014, 1:51 a.m., Ben Mahler wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/23661/
> -----------------------------------------------------------
>
> (Updated July 18, 2014, 1:51 a.m.)
>
>
> Review request for mesos, Benjamin Hindman and Vinod Kone.
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> All status updates in the master are now sent via Master::forward() which means they are all logged in the same format.
>
> This should make debugging easier and it simplifies the Master code.
>
>
> Diffs
> -----
>
> src/common/protobuf_utils.hpp 12ff00a2e1262d785dbd6ec4d54e50d88d611e20
> src/master/master.hpp fa46a67fba0fc0a019cdb37770994eee0099d260
> src/master/master.cpp fb2fd5a0288296370a034d57083cf22992306cea
>
> Diff: https://reviews.apache.org/r/23661/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Ben Mahler
>
>
Re: Review Request 23661: Unified status update logging in the master.
Posted by Vinod Kone <vi...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/23661/#review48090
-----------------------------------------------------------
src/master/master.cpp
<https://reviews.apache.org/r/23661/#comment84380>
It is a bit confusing to know when to call this overload vs the other. Why not just have the above one?
src/master/master.cpp
<https://reviews.apache.org/r/23661/#comment84379>
s/,/;/
src/master/master.cpp
<https://reviews.apache.org/r/23661/#comment84377>
s/,/;/
- Vinod Kone
On July 17, 2014, 11:49 p.m., Ben Mahler wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/23661/
> -----------------------------------------------------------
>
> (Updated July 17, 2014, 11:49 p.m.)
>
>
> Review request for mesos and Vinod Kone.
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> All status updates in the master are now sent via Master::forward() which means they are all logged in the same format.
>
> This should make debugging easier and it simplifies the Master code.
>
>
> Diffs
> -----
>
> src/common/protobuf_utils.hpp 12ff00a2e1262d785dbd6ec4d54e50d88d611e20
> src/master/master.hpp fa46a67fba0fc0a019cdb37770994eee0099d260
> src/master/master.cpp fb2fd5a0288296370a034d57083cf22992306cea
>
> Diff: https://reviews.apache.org/r/23661/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Ben Mahler
>
>
Re: Review Request 23661: Unified status update logging in the master.
Posted by Ben Mahler <be...@gmail.com>.
> On July 18, 2014, 6:06 p.m., Vinod Kone wrote:
> > src/master/master.cpp, line 3150
> > <https://reviews.apache.org/r/23661/diff/3/?file=634948#file634948line3150>
> >
> > Why not just pass the framework pid instead of Framework*?
I was a bit worried about the type signature:
forward(StatusUpdate, UPID, UPID);
vs.
forward(StatusUpdate, UPID, Framework*);
My thinking was:
(1) The latter forces a Framework lookup in the caller, which they need to do anyway to get the UPID.
(2) The latter is less error-prone. With the former, one can accidentally pass the UPIDs in the wrong order. It's hard to read a call-site and know if it's correct, without referring to the argument names.
What do you think?
- Ben
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/23661/#review48147
-----------------------------------------------------------
On July 18, 2014, 1:51 a.m., Ben Mahler wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/23661/
> -----------------------------------------------------------
>
> (Updated July 18, 2014, 1:51 a.m.)
>
>
> Review request for mesos, Benjamin Hindman and Vinod Kone.
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> All status updates in the master are now sent via Master::forward() which means they are all logged in the same format.
>
> This should make debugging easier and it simplifies the Master code.
>
>
> Diffs
> -----
>
> src/common/protobuf_utils.hpp 12ff00a2e1262d785dbd6ec4d54e50d88d611e20
> src/master/master.hpp fa46a67fba0fc0a019cdb37770994eee0099d260
> src/master/master.cpp fb2fd5a0288296370a034d57083cf22992306cea
>
> Diff: https://reviews.apache.org/r/23661/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Ben Mahler
>
>
Re: Review Request 23661: Unified status update logging in the master.
Posted by Vinod Kone <vi...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/23661/#review48147
-----------------------------------------------------------
Ship it!
src/master/master.cpp
<https://reviews.apache.org/r/23661/#comment84443>
Why not just pass the framework pid instead of Framework*?
- Vinod Kone
On July 18, 2014, 1:51 a.m., Ben Mahler wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/23661/
> -----------------------------------------------------------
>
> (Updated July 18, 2014, 1:51 a.m.)
>
>
> Review request for mesos, Benjamin Hindman and Vinod Kone.
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> All status updates in the master are now sent via Master::forward() which means they are all logged in the same format.
>
> This should make debugging easier and it simplifies the Master code.
>
>
> Diffs
> -----
>
> src/common/protobuf_utils.hpp 12ff00a2e1262d785dbd6ec4d54e50d88d611e20
> src/master/master.hpp fa46a67fba0fc0a019cdb37770994eee0099d260
> src/master/master.cpp fb2fd5a0288296370a034d57083cf22992306cea
>
> Diff: https://reviews.apache.org/r/23661/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Ben Mahler
>
>
Re: Review Request 23661: Unified status update logging in the master.
Posted by Ben Mahler <be...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/23661/
-----------------------------------------------------------
(Updated July 18, 2014, 1:51 a.m.)
Review request for mesos, Benjamin Hindman and Vinod Kone.
Changes
-------
Simplified this a bit per Vinod's comment.
Repository: mesos-git
Description
-------
All status updates in the master are now sent via Master::forward() which means they are all logged in the same format.
This should make debugging easier and it simplifies the Master code.
Diffs (updated)
-----
src/common/protobuf_utils.hpp 12ff00a2e1262d785dbd6ec4d54e50d88d611e20
src/master/master.hpp fa46a67fba0fc0a019cdb37770994eee0099d260
src/master/master.cpp fb2fd5a0288296370a034d57083cf22992306cea
Diff: https://reviews.apache.org/r/23661/diff/
Testing
-------
make check
Thanks,
Ben Mahler
Re: Review Request 23661: Unified status update logging in the master.
Posted by Ben Mahler <be...@gmail.com>.
> On July 17, 2014, 11:52 p.m., Dominic Hamon wrote:
> > src/master/master.hpp, line 365
> > <https://reviews.apache.org/r/23661/diff/2/?file=634900#file634900line365>
> >
> > should this be private?
Yeah many of these Master methods should probably be private, but I'll defer on moving them all.
- Ben
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/23661/#review48088
-----------------------------------------------------------
On July 17, 2014, 11:49 p.m., Ben Mahler wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/23661/
> -----------------------------------------------------------
>
> (Updated July 17, 2014, 11:49 p.m.)
>
>
> Review request for mesos and Vinod Kone.
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> All status updates in the master are now sent via Master::forward() which means they are all logged in the same format.
>
> This should make debugging easier and it simplifies the Master code.
>
>
> Diffs
> -----
>
> src/common/protobuf_utils.hpp 12ff00a2e1262d785dbd6ec4d54e50d88d611e20
> src/master/master.hpp fa46a67fba0fc0a019cdb37770994eee0099d260
> src/master/master.cpp fb2fd5a0288296370a034d57083cf22992306cea
>
> Diff: https://reviews.apache.org/r/23661/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Ben Mahler
>
>
Re: Review Request 23661: Unified status update logging in the master.
Posted by Dominic Hamon <dh...@twopensource.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/23661/#review48088
-----------------------------------------------------------
src/master/master.hpp
<https://reviews.apache.org/r/23661/#comment84373>
should this be private?
- Dominic Hamon
On July 17, 2014, 4:49 p.m., Ben Mahler wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/23661/
> -----------------------------------------------------------
>
> (Updated July 17, 2014, 4:49 p.m.)
>
>
> Review request for mesos and Vinod Kone.
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> All status updates in the master are now sent via Master::forward() which means they are all logged in the same format.
>
> This should make debugging easier and it simplifies the Master code.
>
>
> Diffs
> -----
>
> src/common/protobuf_utils.hpp 12ff00a2e1262d785dbd6ec4d54e50d88d611e20
> src/master/master.hpp fa46a67fba0fc0a019cdb37770994eee0099d260
> src/master/master.cpp fb2fd5a0288296370a034d57083cf22992306cea
>
> Diff: https://reviews.apache.org/r/23661/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Ben Mahler
>
>