Re: Review Request 36404: Added support for peek() to process::io
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36404/ --- (Updated Aug. 27, 2015, 9:24 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 --- See summary. Diffs (updated) - 3rdparty/libprocess/include/process/io.hpp 975923f40f82357f31b89428f24d01df6a8ac9fc 3rdparty/libprocess/src/io.cpp 4a6e18a17012994d358099ad32d4c282fea3b0b1 3rdparty/libprocess/src/tests/io_tests.cpp c642bab9e2845668767ad237985cb9ce1109 Diff: https://reviews.apache.org/r/36404/diff/ Testing --- - Added a test case for process::io::peek - make check Thanks, Artem Harutyunyan
Re: Review Request 36404: Added support for peek() to process::io
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36404/#review96657 --- Patch looks great! Reviews applied: [36999, 36646, 36404] All tests passed. - Mesos ReviewBot On Aug. 27, 2015, 5:19 a.m., Artem Harutyunyan wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36404/ --- (Updated Aug. 27, 2015, 5:19 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 --- See summary. Diffs - 3rdparty/libprocess/include/process/io.hpp 975923f40f82357f31b89428f24d01df6a8ac9fc 3rdparty/libprocess/src/io.cpp 4a6e18a17012994d358099ad32d4c282fea3b0b1 3rdparty/libprocess/src/tests/io_tests.cpp c642bab9e2845668767ad237985cb9ce1109 Diff: https://reviews.apache.org/r/36404/diff/ Testing --- - Added a test case for process::io::peek - make check Thanks, Artem Harutyunyan
Re: Review Request 36404: Added support for peek() to process::io
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36404/ --- (Updated Aug. 26, 2015, 10:19 p.m.) Review request for mesos, Joris Van Remoortere and Joseph Wu. Changes --- Addressed comments. Bugs: MESOS-2964 https://issues.apache.org/jira/browse/MESOS-2964 Repository: mesos Description (updated) --- See summary. Diffs (updated) - 3rdparty/libprocess/include/process/io.hpp 975923f40f82357f31b89428f24d01df6a8ac9fc 3rdparty/libprocess/src/io.cpp 4a6e18a17012994d358099ad32d4c282fea3b0b1 3rdparty/libprocess/src/tests/io_tests.cpp c642bab9e2845668767ad237985cb9ce1109 Diff: https://reviews.apache.org/r/36404/diff/ Testing --- - Added a test case for process::io::peek - make check Thanks, Artem Harutyunyan
Re: Review Request 36404: Added support for peek() to process::io
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36404/#review96356 --- 3rdparty/libprocess/src/io.cpp (lines 36 - 37) https://reviews.apache.org/r/36404/#comment151674 +2 not +4 please. - 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 c642bab9e2845668767ad237985cb9ce1109 Diff: https://reviews.apache.org/r/36404/diff/ Testing --- - Added a test case for process::io::peek - make check Thanks, Artem Harutyunyan
Re: Review Request 36404: Added support for peek() to process::io
--- 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 c642bab9e2845668767ad237985cb9ce1109 Diff: https://reviews.apache.org/r/36404/diff/ Testing --- - Added a test case for process::io::peek - make check Thanks, Artem Harutyunyan
Re: Review Request 36404: Added support for peek() to process::io
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36404/ --- (Updated Aug. 25, 2015, 10:17 p.m.) Review request for Joris Van Remoortere and Joseph Wu. Changes --- Addressed some comments. Bugs: MESOS-2964 https://issues.apache.org/jira/browse/MESOS-2964 Repository: mesos Description --- JIRA: https://issues.apache.org/jira/browse/MESOS-2964 Diffs (updated) - 3rdparty/libprocess/include/process/io.hpp 975923f40f82357f31b89428f24d01df6a8ac9fc 3rdparty/libprocess/src/io.cpp 4a6e18a17012994d358099ad32d4c282fea3b0b1 3rdparty/libprocess/src/tests/io_tests.cpp c642bab9e2845668767ad237985cb9ce1109 Diff: https://reviews.apache.org/r/36404/diff/ Testing --- - Added a test case for process::io::peek - make check Thanks, Artem Harutyunyan
Re: Review Request 36404: Added support for peek() to process::io
On Aug. 25, 2015, 9:29 a.m., Benjamin Hindman wrote: 3rdparty/libprocess/src/io.cpp, lines 274-286 https://reviews.apache.org/r/36404/diff/7/?file=1027739#file1027739line274 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). I initially had the dup() here (it's in this same review) but then Joris pointed out that ::recv() with MSG_PEEK set can not surpass single message boundaries. I removed the call to dup() because unlike the case with io::read() we are not looping here and are performing just a single peek() request. I will look into this further and will update accordingly. On Aug. 25, 2015, 9:29 a.m., Benjamin Hindman wrote: 3rdparty/libprocess/src/io.cpp, line 577 https://reviews.apache.org/r/36404/diff/7/?file=1027739#file1027739line577 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. That was an oversight. Fixed. - Artem --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36404/#review96342 --- On Aug. 5, 2015, 8:26 p.m., Artem Harutyunyan wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36404/ --- (Updated Aug. 5, 2015, 8:26 p.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 c642bab9e2845668767ad237985cb9ce1109 Diff: https://reviews.apache.org/r/36404/diff/ Testing --- - Added a test case for process::io::peek - make check Thanks, Artem Harutyunyan
Re: Review Request 36404: Added support for peek() to process::io
On July 26, 2015, 6:18 a.m., Joris Van Remoortere wrote: 3rdparty/libprocess/src/tests/io_tests.cpp, line 377 https://reviews.apache.org/r/36404/diff/4/?file=1018378#file1018378line377 we can get rid of some of these literals by: s/3/sizeof(data)/ What do you think? I followed what was done in the read test here. - Artem --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36404/#review93040 --- On Aug. 5, 2015, 8:26 p.m., Artem Harutyunyan wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36404/ --- (Updated Aug. 5, 2015, 8:26 p.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 c642bab9e2845668767ad237985cb9ce1109 Diff: https://reviews.apache.org/r/36404/diff/ Testing --- - Added a test case for process::io::peek - make check Thanks, Artem Harutyunyan
Re: Review Request 36404: Added support for peek() to process::io
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36404/ --- (Updated Aug. 5, 2015, 8:26 p.m.) Review request for mesos, Joris Van Remoortere and Joseph Wu. Changes --- Added missing JIRA ticket. 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 c642bab9e2845668767ad237985cb9ce1109 Diff: https://reviews.apache.org/r/36404/diff/ Testing --- - Added a test case for process::io::peek - make check Thanks, Artem Harutyunyan
Re: Review Request 36404: Added support for peek() to process::io
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36404/#review93994 --- Patch looks great! Reviews applied: [36999, 36646, 36404] All tests passed. - Mesos ReviewBot On Aug. 3, 2015, 6:30 p.m., Artem Harutyunyan wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36404/ --- (Updated Aug. 3, 2015, 6:30 p.m.) Review request for mesos, Joris Van Remoortere and Joseph Wu. 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 c642bab9e2845668767ad237985cb9ce1109 Diff: https://reviews.apache.org/r/36404/diff/ Testing --- - Added a test case for process::io::peek - make check Thanks, Artem Harutyunyan
Re: Review Request 36404: Added support for peek() to process::io
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36404/ --- (Updated Aug. 3, 2015, 11:30 a.m.) Review request for mesos, Joris Van Remoortere and Joseph Wu. Changes --- Missing commas. Repository: mesos Description --- JIRA: https://issues.apache.org/jira/browse/MESOS-2964 Diffs (updated) - 3rdparty/libprocess/include/process/io.hpp 975923f40f82357f31b89428f24d01df6a8ac9fc 3rdparty/libprocess/src/io.cpp 4a6e18a17012994d358099ad32d4c282fea3b0b1 3rdparty/libprocess/src/tests/io_tests.cpp c642bab9e2845668767ad237985cb9ce1109 Diff: https://reviews.apache.org/r/36404/diff/ Testing --- - Added a test case for process::io::peek - make check Thanks, Artem Harutyunyan
Re: Review Request 36404: Added support for peek() to process::io
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36404/ --- (Updated Aug. 3, 2015, 10:44 a.m.) Review request for mesos, Joris Van Remoortere and Joseph Wu. Repository: mesos Description --- JIRA: https://issues.apache.org/jira/browse/MESOS-2964 Diffs (updated) - 3rdparty/libprocess/include/process/io.hpp 975923f40f82357f31b89428f24d01df6a8ac9fc 3rdparty/libprocess/src/io.cpp 4a6e18a17012994d358099ad32d4c282fea3b0b1 3rdparty/libprocess/src/tests/io_tests.cpp c642bab9e2845668767ad237985cb9ce1109 Diff: https://reviews.apache.org/r/36404/diff/ Testing --- - Added a test case for process::io::peek - make check Thanks, Artem Harutyunyan
Re: Review Request 36404: Added support for peek() to process::io
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36404/ --- (Updated July 21, 2015, 10:29 p.m.) Review request for mesos, Joris Van Remoortere and Joseph Wu. Changes --- Rebased and changed comments to Doxygen format. Repository: mesos Description --- JIRA: https://issues.apache.org/jira/browse/MESOS-2964 Diffs (updated) - 3rdparty/libprocess/include/process/io.hpp 975923f40f82357f31b89428f24d01df6a8ac9fc 3rdparty/libprocess/src/io.cpp 4a6e18a17012994d358099ad32d4c282fea3b0b1 3rdparty/libprocess/src/tests/io_tests.cpp c642bab9e2845668767ad237985cb9ce1109 Diff: https://reviews.apache.org/r/36404/diff/ Testing --- - Added a test case for process::io::peek - make check Thanks, Artem Harutyunyan
Re: Review Request 36404: Added support for peek() to process::io
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36404/ --- (Updated July 10, 2015, 3:20 p.m.) Review request for mesos, Joris Van Remoortere and Joseph Wu. Repository: mesos Description --- JIRA: https://issues.apache.org/jira/browse/MESOS-2964 Diffs (updated) - 3rdparty/libprocess/include/process/io.hpp 245716353ad5ffa8d705fc5e826addfa6a3594dc 3rdparty/libprocess/src/io.cpp 4a6e18a17012994d358099ad32d4c282fea3b0b1 3rdparty/libprocess/src/tests/io_tests.cpp c642bab9e2845668767ad237985cb9ce1109 Diff: https://reviews.apache.org/r/36404/diff/ Testing --- - Added a test case for process::io::peek - make check Thanks, Artem Harutyunyan
