mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Benjamin Hindman" <b...@berkeley.edu>
Subject Re: Review Request 36404: Added support for peek() to process::io
Date Tue, 25 Aug 2015 16:29:35 GMT

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



3rdparty/libprocess/include/process/io.hpp (line 145)
<https://reviews.apache.org/r/36404/#comment151656>

    I would like us to document the parameters please, specifically it's not obvious the difference
between 'size' and 'limit'. IIUC we'll return right away even if we haven't read all of 'size'?
Same for the overload below.



3rdparty/libprocess/src/io.cpp (lines 69 - 70)
<https://reviews.apache.org/r/36404/#comment151672>

    Same line please.



3rdparty/libprocess/src/io.cpp (line 71)
<https://reviews.apache.org/r/36404/#comment151673>

    How about adding a comment that if 'fd' is not a socket we'll "do the right thing" because
'recv' will fail with ENOTSOCK and we'll propagate that out.



3rdparty/libprocess/src/io.cpp (lines 274 - 286)
<https://reviews.apache.org/r/36404/#comment151671>

    This is the "old style", in the "new style" we just dupliate the file descriptor so that
if someone closes the file descriptor passed to us we don't either read from a closed file
descriptor or WORSE read from a newly opened file descriptor that we shouldn't be reading
from. See 'io::read(int fd)' for an example. Note that the other ones need to get changed
as well but I've done that for the Mesos on Windows work (in the my github/benh/mesos mesos-on-windows
branch, which was necessary to do there because we can't support os::isNonblock on Windows).



3rdparty/libprocess/src/io.cpp (line 400)
<https://reviews.apache.org/r/36404/#comment151670>

    The other internal::_* functions were originally created to (A) deal with the fact we
didn't have C++11 and (B) because they were recursive. We don't need that for 'peek' in this
case, and in the other cases we can ultimately remove the need for the internal function and
just use a recursive lambda. So, how about killing internal::_peek?



3rdparty/libprocess/src/io.cpp (line 407)
<https://reviews.apache.org/r/36404/#comment151662>

    I just checked and we've used 'length' throughout this file for this variable name, let's
keep it consistent for now and do the same here please.



3rdparty/libprocess/src/io.cpp (line 577)
<https://reviews.apache.org/r/36404/#comment151658>

    The declaration uses the variable named 'limit' but here it's 'size' which sort of implies
a different semantics? Either way, we should use the same name please.



3rdparty/libprocess/src/io.cpp (line 582)
<https://reviews.apache.org/r/36404/#comment151659>

    This indentation should be +2 not +4.



3rdparty/libprocess/src/io.cpp (line 588)
<https://reviews.apache.org/r/36404/#comment151661>

    This appears to be an unused variable? Or am I missing something? Which will also render
the TODO(bmahler) above useless.


- Benjamin Hindman


On Aug. 6, 2015, 3:26 a.m., Artem Harutyunyan wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/36404/
> -----------------------------------------------------------
> 
> (Updated Aug. 6, 2015, 3:26 a.m.)
> 
> 
> Review request for mesos, Joris Van Remoortere and Joseph Wu.
> 
> 
> Bugs: MESOS-2964
>     https://issues.apache.org/jira/browse/MESOS-2964
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> JIRA: https://issues.apache.org/jira/browse/MESOS-2964
> 
> 
> Diffs
> -----
> 
>   3rdparty/libprocess/include/process/io.hpp 975923f40f82357f31b89428f24d01df6a8ac9fc

>   3rdparty/libprocess/src/io.cpp 4a6e18a17012994d358099ad32d4c282fea3b0b1 
>   3rdparty/libprocess/src/tests/io_tests.cpp c642b3333ab9e2845668767ad237985cb9ce1109

> 
> Diff: https://reviews.apache.org/r/36404/diff/
> 
> 
> Testing
> -------
> 
> - Added a test case for process::io::peek
> - make check
> 
> 
> Thanks,
> 
> Artem Harutyunyan
> 
>


Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message