You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@mesos.apache.org by Joris Van Remoortere <jo...@gmail.com> on 2014/12/03 20:50:46 UTC
Re: Review Request 28323: Introduce libevent poll implementation.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/28323/
-----------------------------------------------------------
(Updated Dec. 3, 2014, 7:50 p.m.)
Review request for mesos, Benjamin Hindman and Niklas Nielsen.
Changes
-------
Rebased.
Repository: mesos-git
Description
-------
See summary.
Diffs (updated)
-----
3rdparty/libprocess/Makefile.am aebd281da41325a0246f5c57780928afc22a1baa
3rdparty/libprocess/src/libevent_poll.cpp PRE-CREATION
Diff: https://reviews.apache.org/r/28323/diff/
Testing
-------
Thanks,
Joris Van Remoortere
Re: Review Request 28323: Introduce libevent poll implementation.
Posted by Benjamin Hindman <be...@berkeley.edu>.
> On Dec. 25, 2014, 9:13 p.m., Benjamin Hindman wrote:
> >
I fixed up these minor comments and committed, thanks!
- Benjamin
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/28323/#review66112
-----------------------------------------------------------
On Dec. 19, 2014, 12:24 a.m., Joris Van Remoortere wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/28323/
> -----------------------------------------------------------
>
> (Updated Dec. 19, 2014, 12:24 a.m.)
>
>
> Review request for mesos, Benjamin Hindman and Niklas Nielsen.
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> See summary.
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/Makefile.am 75870ac754e500bb4ca689201bde677fa7d854d0
> 3rdparty/libprocess/src/libevent_poll.cpp PRE-CREATION
>
> Diff: https://reviews.apache.org/r/28323/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Joris Van Remoortere
>
>
Re: Review Request 28323: Introduce libevent poll implementation.
Posted by Benjamin Hindman <be...@berkeley.edu>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/28323/#review66112
-----------------------------------------------------------
Ship it!
3rdparty/libprocess/src/libevent_poll.cpp
<https://reviews.apache.org/r/28323/#comment109471>
s/avail/events/ to match with what we call it in libprocess.
3rdparty/libprocess/src/libevent_poll.cpp
<https://reviews.apache.org/r/28323/#comment109472>
s/interest/what/ to match what libevent calls it!
- Benjamin Hindman
On Dec. 19, 2014, 12:24 a.m., Joris Van Remoortere wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/28323/
> -----------------------------------------------------------
>
> (Updated Dec. 19, 2014, 12:24 a.m.)
>
>
> Review request for mesos, Benjamin Hindman and Niklas Nielsen.
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> See summary.
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/Makefile.am 75870ac754e500bb4ca689201bde677fa7d854d0
> 3rdparty/libprocess/src/libevent_poll.cpp PRE-CREATION
>
> Diff: https://reviews.apache.org/r/28323/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Joris Van Remoortere
>
>
Re: Review Request 28323: Introduce libevent poll implementation.
Posted by Joris Van Remoortere <jo...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/28323/
-----------------------------------------------------------
(Updated Dec. 19, 2014, 12:24 a.m.)
Review request for mesos, Benjamin Hindman and Niklas Nielsen.
Changes
-------
Fix style issue.
Repository: mesos-git
Description
-------
See summary.
Diffs (updated)
-----
3rdparty/libprocess/Makefile.am 75870ac754e500bb4ca689201bde677fa7d854d0
3rdparty/libprocess/src/libevent_poll.cpp PRE-CREATION
Diff: https://reviews.apache.org/r/28323/diff/
Testing
-------
Thanks,
Joris Van Remoortere
Re: Review Request 28323: Introduce libevent poll implementation.
Posted by Joris Van Remoortere <jo...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/28323/
-----------------------------------------------------------
(Updated Dec. 5, 2014, 7:25 a.m.)
Review request for mesos, Benjamin Hindman and Niklas Nielsen.
Changes
-------
Clean up.
Repository: mesos-git
Description
-------
See summary.
Diffs (updated)
-----
3rdparty/libprocess/Makefile.am aebd281da41325a0246f5c57780928afc22a1baa
3rdparty/libprocess/src/libevent_poll.cpp PRE-CREATION
Diff: https://reviews.apache.org/r/28323/diff/
Testing
-------
Thanks,
Joris Van Remoortere
Re: Review Request 28323: Introduce libevent poll implementation.
Posted by Benjamin Hindman <be...@berkeley.edu>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/28323/#review63919
-----------------------------------------------------------
Ship it!
3rdparty/libprocess/src/libevent_poll.cpp
<https://reviews.apache.org/r/28323/#comment106248>
{ on newline please.
3rdparty/libprocess/src/libevent_poll.cpp
<https://reviews.apache.org/r/28323/#comment106250>
Do we need the 'struct'? I've seen this a few times, so please do a sweep of these reviews and remove them if possible. (I do know of one place where we need it because libev uses the same name for a struct and a function).
3rdparty/libprocess/src/libevent_poll.cpp
<https://reviews.apache.org/r/28323/#comment106259>
Let's s/auto/Future<short>/ for now, thanks!
3rdparty/libprocess/src/libevent_poll.cpp
<https://reviews.apache.org/r/28323/#comment106260>
How about:
poll->ev = event_new(...);
Then use poll->ev everywhere.
- Benjamin Hindman
On Dec. 3, 2014, 11:27 p.m., Joris Van Remoortere wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/28323/
> -----------------------------------------------------------
>
> (Updated Dec. 3, 2014, 11:27 p.m.)
>
>
> Review request for mesos, Benjamin Hindman and Niklas Nielsen.
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> See summary.
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/Makefile.am aebd281da41325a0246f5c57780928afc22a1baa
> 3rdparty/libprocess/src/libevent_poll.cpp PRE-CREATION
>
> Diff: https://reviews.apache.org/r/28323/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Joris Van Remoortere
>
>
Re: Review Request 28323: Introduce libevent poll implementation.
Posted by Joris Van Remoortere <jo...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/28323/
-----------------------------------------------------------
(Updated Dec. 3, 2014, 11:27 p.m.)
Review request for mesos, Benjamin Hindman and Niklas Nielsen.
Changes
-------
Rename pollcb to pollCallback.
Add comments for enumeration conversion between io::\* and EV_*.
Repository: mesos-git
Description
-------
See summary.
Diffs (updated)
-----
3rdparty/libprocess/Makefile.am aebd281da41325a0246f5c57780928afc22a1baa
3rdparty/libprocess/src/libevent_poll.cpp PRE-CREATION
Diff: https://reviews.apache.org/r/28323/diff/
Testing
-------
Thanks,
Joris Van Remoortere
Re: Review Request 28323: Introduce libevent poll implementation.
Posted by Joris Van Remoortere <jo...@gmail.com>.
> On Dec. 3, 2014, 8:09 p.m., Niklas Nielsen wrote:
> > 3rdparty/libprocess/src/libevent_poll.cpp, lines 27-28
> > <https://reviews.apache.org/r/28323/diff/3/?file=781921#file781921line27>
> >
> > I have a bit of a hard time understanding this - can you expand/comment on what's going on?
Since the enumerations for READ / WRITE are different in io.hpp and libevent, we need to do a conversion.
See:
https://github.com/libevent/libevent/blob/master/include/event2/event.h#L920
vs.
io.hpp#L15
- Joris
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/28323/#review63719
-----------------------------------------------------------
On Dec. 3, 2014, 11:27 p.m., Joris Van Remoortere wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/28323/
> -----------------------------------------------------------
>
> (Updated Dec. 3, 2014, 11:27 p.m.)
>
>
> Review request for mesos, Benjamin Hindman and Niklas Nielsen.
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> See summary.
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/Makefile.am aebd281da41325a0246f5c57780928afc22a1baa
> 3rdparty/libprocess/src/libevent_poll.cpp PRE-CREATION
>
> Diff: https://reviews.apache.org/r/28323/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Joris Van Remoortere
>
>
Re: Review Request 28323: Introduce libevent poll implementation.
Posted by Niklas Nielsen <ni...@qni.dk>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/28323/#review63719
-----------------------------------------------------------
3rdparty/libprocess/src/libevent_poll.cpp
<https://reviews.apache.org/r/28323/#comment106005>
cb for callback - we usually don't abbreviate function names. What do you think about spelling it out?
3rdparty/libprocess/src/libevent_poll.cpp
<https://reviews.apache.org/r/28323/#comment106002>
I have a bit of a hard time understanding this - can you expand/comment on what's going on?
3rdparty/libprocess/src/libevent_poll.cpp
<https://reviews.apache.org/r/28323/#comment106000>
Looks a bit odd to me; should it be single newline?
3rdparty/libprocess/src/libevent_poll.cpp
<https://reviews.apache.org/r/28323/#comment106003>
Same as above
3rdparty/libprocess/src/libevent_poll.cpp
<https://reviews.apache.org/r/28323/#comment106004>
s/nullptr/NULL/
- Niklas Nielsen
On Dec. 3, 2014, 11:50 a.m., Joris Van Remoortere wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/28323/
> -----------------------------------------------------------
>
> (Updated Dec. 3, 2014, 11:50 a.m.)
>
>
> Review request for mesos, Benjamin Hindman and Niklas Nielsen.
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> See summary.
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/Makefile.am aebd281da41325a0246f5c57780928afc22a1baa
> 3rdparty/libprocess/src/libevent_poll.cpp PRE-CREATION
>
> Diff: https://reviews.apache.org/r/28323/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Joris Van Remoortere
>
>