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 2012/09/12 02:23:30 UTC
Review Request: Changing FilesProcess to use async io::read
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/7048/
-----------------------------------------------------------
Review request for mesos and Benjamin Hindman.
Description
-------
see above
also:
-removed redundant length field from json
-added missing test for buffered io::read
Diffs
-----
src/files/files.cpp 806aa35
src/tests/files_tests.cpp 6ef2004
src/webui/master/static/jquery.pailer.js edd23d9
third_party/libprocess/src/tests.cpp 41bf973
Diff: https://reviews.apache.org/r/7048/diff/
Testing
-------
make check on osx, and redhat
mesos-local.sh to verify pailer
Thanks,
Ben Mahler
Re: Review Request: Changing FilesProcess to use async io::read
Posted by Ben Mahler <be...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/7048/#review12254
-----------------------------------------------------------
discussed with benh, the current read is ok, I will do the following:
-Cap the read size at 16k.
-Update the pailer documentation to indicate the lack of 'length' in the response.
-Double check that the read is ok with partial data
- Ben Mahler
On Sept. 14, 2012, midnight, Ben Mahler wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/7048/
> -----------------------------------------------------------
>
> (Updated Sept. 14, 2012, midnight)
>
>
> Review request for mesos and Benjamin Hindman.
>
>
> Description
> -------
>
> see above
>
> also:
> -removed redundant length field from json
> -added missing test for buffered io::read
>
>
> Diffs
> -----
>
> src/files/files.cpp 806aa35
> src/tests/files_tests.cpp 6ef2004
> src/webui/master/static/jquery.pailer.js edd23d9
> third_party/libprocess/include/process/io.hpp 722b15f
> third_party/libprocess/src/process.cpp 2d2b56c
> third_party/libprocess/src/tests.cpp 41bf973
>
> Diff: https://reviews.apache.org/r/7048/diff/
>
>
> Testing
> -------
>
> make check on osx, and redhat
>
> mesos-local.sh to verify pailer
>
>
> Thanks,
>
> Ben Mahler
>
>
Re: Review Request: Changing FilesProcess to use async io::read,
updated pailer to not use redundant 'length' field of response.
Posted by Ben Mahler <be...@gmail.com>.
> On Oct. 10, 2012, 7:17 p.m., Vinod Kone wrote:
> > src/tests/files_tests.cpp, line 105
> > <https://reviews.apache.org/r/7048/diff/2-5/?file=152777#file152777line105>
> >
> > checking on the body is nice but i suspect this is too brittle, when someone changes the response string.
This is borrowed from benh's tests (logging_tests.cpp).
I think I also like checking the string because it ensures:
1. The request fails _for the correct reason_.
2. We're providing good diagnostic output to the end user.
If someone changes the response string the test will fail, and they'll update it. :)
> On Oct. 10, 2012, 7:17 p.m., Vinod Kone wrote:
> > src/files/files.cpp, line 260
> > <https://reviews.apache.org/r/7048/diff/2-5/?file=152776#file152776line260>
> >
> > Is this cap documented somewhere for the users of read()? Maybe a warning is in order?
This is documented in the pailer, in terms of what it expects of the read function provided to it.
However, the documentation is lacking here, so I've updated each HTTP endpoint function documentation. Please read through the documentation I've added to the function signatures.
Things are ok when this cap occurs, it's merely mimics how the read syscall behaves.
Let me know what you think!
> On Oct. 10, 2012, 7:17 p.m., Vinod Kone wrote:
> > third_party/libprocess/src/tests.cpp, line 1233
> > <https://reviews.apache.org/r/7048/diff/2-5/?file=152779#file152779line1233>
> >
> > bad diff?
hm.. looks like it!
- Ben
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/7048/#review12316
-----------------------------------------------------------
On Oct. 10, 2012, 9:46 p.m., Ben Mahler wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/7048/
> -----------------------------------------------------------
>
> (Updated Oct. 10, 2012, 9:46 p.m.)
>
>
> Review request for mesos, Benjamin Hindman and Vinod Kone.
>
>
> Description
> -------
>
> see above
>
> also:
> -removed redundant length field from json
> -added missing test for buffered io::read
>
>
> Diffs
> -----
>
> src/files/files.cpp 6d41bf6649479f83b1f5f56665b555c0c553e713
> src/tests/files_tests.cpp 764d2c7fdec4a21b9698eb4cb9e0673bd7f20e9d
> src/webui/master/static/jquery.pailer.js edd23d9efa03086679af67a8bd13a273d409798b
> third_party/libprocess/include/process/io.hpp 722b15f11db4d3b6c941c8b4a6c5d395b11b035f
> third_party/libprocess/src/process.cpp fde7154f9a4432acaff09849c0fa69c0694c9834
> third_party/libprocess/src/tests.cpp 1cb26cd1729ce1cf0ed9573d3eb40690705d4b4e
>
> Diff: https://reviews.apache.org/r/7048/diff/
>
>
> Testing
> -------
>
> make check on osx, and redhat
>
> mesos-local.sh to verify pailer
>
>
> Thanks,
>
> Ben Mahler
>
>
Re: Review Request: Changing FilesProcess to use async io::read,
updated pailer to not use redundant 'length' field of response.
Posted by Vinod Kone <vi...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/7048/#review12316
-----------------------------------------------------------
Ship it!
src/files/files.cpp
<https://reviews.apache.org/r/7048/#comment26084>
Is this cap documented somewhere for the users of read()? Maybe a warning is in order?
src/tests/files_tests.cpp
<https://reviews.apache.org/r/7048/#comment26085>
checking on the body is nice but i suspect this is too brittle, when someone changes the response string.
third_party/libprocess/src/tests.cpp
<https://reviews.apache.org/r/7048/#comment26086>
bad diff?
- Vinod Kone
On Oct. 10, 2012, 1:07 a.m., Ben Mahler wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/7048/
> -----------------------------------------------------------
>
> (Updated Oct. 10, 2012, 1:07 a.m.)
>
>
> Review request for mesos, Benjamin Hindman and Vinod Kone.
>
>
> Description
> -------
>
> see above
>
> also:
> -removed redundant length field from json
> -added missing test for buffered io::read
>
>
> Diffs
> -----
>
> src/files/files.cpp c40b7b74485dc717c4291f17a4533b52f2c51a7e
> src/tests/files_tests.cpp 17d284b82de4d9e1dedf1c26f4cedcf8269c4072
> src/webui/master/static/jquery.pailer.js edd23d9efa03086679af67a8bd13a273d409798b
> third_party/libprocess/include/process/io.hpp 722b15f11db4d3b6c941c8b4a6c5d395b11b035f
> third_party/libprocess/src/process.cpp fde7154f9a4432acaff09849c0fa69c0694c9834
> third_party/libprocess/src/tests.cpp 1df7ae61f24643a0ceab146646e77da6909dfd5f
>
> Diff: https://reviews.apache.org/r/7048/diff/
>
>
> Testing
> -------
>
> make check on osx, and redhat
>
> mesos-local.sh to verify pailer
>
>
> Thanks,
>
> Ben Mahler
>
>
Re: Review Request: Changing FilesProcess to use async io::read,
updated pailer to not use redundant 'length' field of response.
Posted by Benjamin Hindman <be...@berkeley.edu>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/7048/#review12431
-----------------------------------------------------------
Ship it!
Also, if you can rebase this that would be amazing!
third_party/libprocess/src/process.cpp
<https://reviews.apache.org/r/7048/#comment26374>
If you want the test below to always be testing what you expect, then I'd suggest factoring out this value into io.hpp as READ_SIZE or something similar and using that in the test.
- Benjamin Hindman
On Oct. 12, 2012, 9:56 p.m., Ben Mahler wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/7048/
> -----------------------------------------------------------
>
> (Updated Oct. 12, 2012, 9:56 p.m.)
>
>
> Review request for mesos, Benjamin Hindman and Vinod Kone.
>
>
> Description
> -------
>
> see above
>
> also:
> -removed redundant length field from json
> -added missing test for buffered io::read
>
>
> Diffs
> -----
>
> src/files/files.cpp 6d41bf6649479f83b1f5f56665b555c0c553e713
> src/tests/files_tests.cpp 764d2c7fdec4a21b9698eb4cb9e0673bd7f20e9d
> src/webui/master/static/jquery.pailer.js edd23d9efa03086679af67a8bd13a273d409798b
> third_party/libprocess/include/process/io.hpp 722b15f11db4d3b6c941c8b4a6c5d395b11b035f
> third_party/libprocess/src/process.cpp e887feb1070cdd03a6d81b8f798145ed8bda7b5c
> third_party/libprocess/src/tests.cpp 9cae36aede41bc1e6bb966122c581cff97483717
>
> Diff: https://reviews.apache.org/r/7048/diff/
>
>
> Testing
> -------
>
> make check on osx, and redhat
>
> mesos-local.sh to verify pailer
>
>
> Thanks,
>
> Ben Mahler
>
>
Re: Review Request: Changing FilesProcess to use async io::read,
updated pailer to not use redundant 'length' field of response.
Posted by Ben Mahler <be...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/7048/
-----------------------------------------------------------
(Updated Oct. 15, 2012, 6:27 p.m.)
Review request for mesos, Benjamin Hindman and Vinod Kone.
Changes
-------
Rebased off trunk.
Added a BUFFERED_READ_SIZE constant in io.hpp.
Re-tested.
Description
-------
see above
also:
-removed redundant length field from json
-added missing test for buffered io::read
Diffs (updated)
-----
src/files/files.cpp b270d965746287ee9e33ac2ca3feb07e9dfca73d
src/tests/files_tests.cpp 130c0d36eab43f5bb94d93f48bb73a4c41aadee1
src/webui/master/static/jquery.pailer.js 5962f9e9d347f10610229e9a50d1e2a93f2f8dc0
third_party/libprocess/include/process/io.hpp 722b15f11db4d3b6c941c8b4a6c5d395b11b035f
third_party/libprocess/src/process.cpp fe612ebf50889b30663516385879b8e06136c3fd
third_party/libprocess/src/tests.cpp bbef0387837725fd4f6307eaf6b51b9b0a80025e
Diff: https://reviews.apache.org/r/7048/diff/
Testing
-------
make check on osx, and redhat
mesos-local.sh to verify pailer
Thanks,
Ben Mahler
Re: Review Request: Changing FilesProcess to use async io::read,
updated pailer to not use redundant 'length' field of response.
Posted by Ben Mahler <be...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/7048/#review12447
-----------------------------------------------------------
third_party/libprocess/src/process.cpp
<https://reviews.apache.org/r/7048/#comment26422>
Done!
- Ben Mahler
On Oct. 12, 2012, 9:56 p.m., Ben Mahler wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/7048/
> -----------------------------------------------------------
>
> (Updated Oct. 12, 2012, 9:56 p.m.)
>
>
> Review request for mesos, Benjamin Hindman and Vinod Kone.
>
>
> Description
> -------
>
> see above
>
> also:
> -removed redundant length field from json
> -added missing test for buffered io::read
>
>
> Diffs
> -----
>
> src/files/files.cpp 6d41bf6649479f83b1f5f56665b555c0c553e713
> src/tests/files_tests.cpp 764d2c7fdec4a21b9698eb4cb9e0673bd7f20e9d
> src/webui/master/static/jquery.pailer.js edd23d9efa03086679af67a8bd13a273d409798b
> third_party/libprocess/include/process/io.hpp 722b15f11db4d3b6c941c8b4a6c5d395b11b035f
> third_party/libprocess/src/process.cpp e887feb1070cdd03a6d81b8f798145ed8bda7b5c
> third_party/libprocess/src/tests.cpp 9cae36aede41bc1e6bb966122c581cff97483717
>
> Diff: https://reviews.apache.org/r/7048/diff/
>
>
> Testing
> -------
>
> make check on osx, and redhat
>
> mesos-local.sh to verify pailer
>
>
> Thanks,
>
> Ben Mahler
>
>
Re: Review Request: Changing FilesProcess to use async io::read,
updated pailer to not use redundant 'length' field of response.
Posted by Ben Mahler <be...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/7048/
-----------------------------------------------------------
(Updated Oct. 12, 2012, 9:56 p.m.)
Review request for mesos, Benjamin Hindman and Vinod Kone.
Changes
-------
Changed the read size max to 16 pages.
Description
-------
see above
also:
-removed redundant length field from json
-added missing test for buffered io::read
Diffs (updated)
-----
src/files/files.cpp 6d41bf6649479f83b1f5f56665b555c0c553e713
src/tests/files_tests.cpp 764d2c7fdec4a21b9698eb4cb9e0673bd7f20e9d
src/webui/master/static/jquery.pailer.js edd23d9efa03086679af67a8bd13a273d409798b
third_party/libprocess/include/process/io.hpp 722b15f11db4d3b6c941c8b4a6c5d395b11b035f
third_party/libprocess/src/process.cpp e887feb1070cdd03a6d81b8f798145ed8bda7b5c
third_party/libprocess/src/tests.cpp 9cae36aede41bc1e6bb966122c581cff97483717
Diff: https://reviews.apache.org/r/7048/diff/
Testing
-------
make check on osx, and redhat
mesos-local.sh to verify pailer
Thanks,
Ben Mahler
Re: Review Request: Changing FilesProcess to use async io::read,
updated pailer to not use redundant 'length' field of response.
Posted by Ben Mahler <be...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/7048/
-----------------------------------------------------------
(Updated Oct. 11, 2012, 12:46 a.m.)
Review request for mesos, Benjamin Hindman and Vinod Kone.
Changes
-------
Fixed whitespace alignment.
Description
-------
see above
also:
-removed redundant length field from json
-added missing test for buffered io::read
Diffs (updated)
-----
src/files/files.cpp 6d41bf6649479f83b1f5f56665b555c0c553e713
src/tests/files_tests.cpp 764d2c7fdec4a21b9698eb4cb9e0673bd7f20e9d
src/webui/master/static/jquery.pailer.js edd23d9efa03086679af67a8bd13a273d409798b
third_party/libprocess/include/process/io.hpp 722b15f11db4d3b6c941c8b4a6c5d395b11b035f
third_party/libprocess/src/process.cpp fde7154f9a4432acaff09849c0fa69c0694c9834
third_party/libprocess/src/tests.cpp 1cb26cd1729ce1cf0ed9573d3eb40690705d4b4e
Diff: https://reviews.apache.org/r/7048/diff/
Testing
-------
make check on osx, and redhat
mesos-local.sh to verify pailer
Thanks,
Ben Mahler
Re: Review Request: Changing FilesProcess to use async io::read,
updated pailer to not use redundant 'length' field of response.
Posted by Vinod Kone <vi...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/7048/#review12320
-----------------------------------------------------------
Ship it!
lgtm
src/tests/files_tests.cpp
<https://reviews.apache.org/r/7048/#comment26093>
align this with pid
- Vinod Kone
On Oct. 10, 2012, 9:46 p.m., Ben Mahler wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/7048/
> -----------------------------------------------------------
>
> (Updated Oct. 10, 2012, 9:46 p.m.)
>
>
> Review request for mesos, Benjamin Hindman and Vinod Kone.
>
>
> Description
> -------
>
> see above
>
> also:
> -removed redundant length field from json
> -added missing test for buffered io::read
>
>
> Diffs
> -----
>
> src/files/files.cpp 6d41bf6649479f83b1f5f56665b555c0c553e713
> src/tests/files_tests.cpp 764d2c7fdec4a21b9698eb4cb9e0673bd7f20e9d
> src/webui/master/static/jquery.pailer.js edd23d9efa03086679af67a8bd13a273d409798b
> third_party/libprocess/include/process/io.hpp 722b15f11db4d3b6c941c8b4a6c5d395b11b035f
> third_party/libprocess/src/process.cpp fde7154f9a4432acaff09849c0fa69c0694c9834
> third_party/libprocess/src/tests.cpp 1cb26cd1729ce1cf0ed9573d3eb40690705d4b4e
>
> Diff: https://reviews.apache.org/r/7048/diff/
>
>
> Testing
> -------
>
> make check on osx, and redhat
>
> mesos-local.sh to verify pailer
>
>
> Thanks,
>
> Ben Mahler
>
>
Re: Review Request: Changing FilesProcess to use async io::read,
updated pailer to not use redundant 'length' field of response.
Posted by Ben Mahler <be...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/7048/
-----------------------------------------------------------
(Updated Oct. 10, 2012, 9:46 p.m.)
Review request for mesos, Benjamin Hindman and Vinod Kone.
Changes
-------
Updated the HTTP Handler documentation.
Description
-------
see above
also:
-removed redundant length field from json
-added missing test for buffered io::read
Diffs (updated)
-----
src/files/files.cpp 6d41bf6649479f83b1f5f56665b555c0c553e713
src/tests/files_tests.cpp 764d2c7fdec4a21b9698eb4cb9e0673bd7f20e9d
src/webui/master/static/jquery.pailer.js edd23d9efa03086679af67a8bd13a273d409798b
third_party/libprocess/include/process/io.hpp 722b15f11db4d3b6c941c8b4a6c5d395b11b035f
third_party/libprocess/src/process.cpp fde7154f9a4432acaff09849c0fa69c0694c9834
third_party/libprocess/src/tests.cpp 1cb26cd1729ce1cf0ed9573d3eb40690705d4b4e
Diff: https://reviews.apache.org/r/7048/diff/
Testing
-------
make check on osx, and redhat
mesos-local.sh to verify pailer
Thanks,
Ben Mahler
Re: Review Request: Changing FilesProcess to use async io::read,
updated pailer to not use redundant 'length' field of response.
Posted by Ben Mahler <be...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/7048/
-----------------------------------------------------------
(Updated Oct. 10, 2012, 1:07 a.m.)
Review request for mesos, Benjamin Hindman and Vinod Kone.
Changes
-------
-Read size is capped at 2x the system page size.
-The pailer documentation is updated to reflect the fact that the 'length' is removed.
-Retested on linux, and with the webui pailer.
Summary (updated)
-----------------
Changing FilesProcess to use async io::read, updated pailer to not use redundant 'length' field of response.
Description
-------
see above
also:
-removed redundant length field from json
-added missing test for buffered io::read
Diffs (updated)
-----
src/files/files.cpp c40b7b74485dc717c4291f17a4533b52f2c51a7e
src/tests/files_tests.cpp 17d284b82de4d9e1dedf1c26f4cedcf8269c4072
src/webui/master/static/jquery.pailer.js edd23d9efa03086679af67a8bd13a273d409798b
third_party/libprocess/include/process/io.hpp 722b15f11db4d3b6c941c8b4a6c5d395b11b035f
third_party/libprocess/src/process.cpp fde7154f9a4432acaff09849c0fa69c0694c9834
third_party/libprocess/src/tests.cpp 1df7ae61f24643a0ceab146646e77da6909dfd5f
Diff: https://reviews.apache.org/r/7048/diff/
Testing
-------
make check on osx, and redhat
mesos-local.sh to verify pailer
Thanks,
Ben Mahler
Re: Review Request: Changing FilesProcess to use async io::read
Posted by Ben Mahler <be...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/7048/
-----------------------------------------------------------
(Updated Sept. 14, 2012, midnight)
Review request for mesos and Benjamin Hindman.
Description
-------
see above
also:
-removed redundant length field from json
-added missing test for buffered io::read
Diffs (updated)
-----
src/files/files.cpp 806aa35
src/tests/files_tests.cpp 6ef2004
src/webui/master/static/jquery.pailer.js edd23d9
third_party/libprocess/include/process/io.hpp 722b15f
third_party/libprocess/src/process.cpp 2d2b56c
third_party/libprocess/src/tests.cpp 41bf973
Diff: https://reviews.apache.org/r/7048/diff/
Testing
-------
make check on osx, and redhat
mesos-local.sh to verify pailer
Thanks,
Ben Mahler
Re: Review Request: Changing FilesProcess to use async io::read
Posted by Ben Mahler <be...@gmail.com>.
> On Sept. 13, 2012, 7:24 p.m., Benjamin Hindman wrote:
> > third_party/libprocess/src/process.cpp, line 3196
> > <https://reviews.apache.org/r/7048/diff/3/?file=153210#file153210line3196>
> >
> > PSA: As much as we are able, let's move towards std::tr1 and C++11 std (rather than Boost).
right, I used shared_array because tr1 and C++11 do not have an equivalent array handling shared_ptr (calling delete[] instead of delete)
If I wrapped up this data into a struct:
-I can now use a tr1 shared_ptr
-I could use unique_ptr once we have std::move right?
want me to add that struct now?
> On Sept. 13, 2012, 7:24 p.m., Benjamin Hindman wrote:
> > third_party/libprocess/src/process.cpp, lines 3174-3175
> > <https://reviews.apache.org/r/7048/diff/3/?file=153210#file153210line3174>
> >
> > const &
so does this work only because bind makes a copy of the shared_ptr / shared_array?
> On Sept. 13, 2012, 7:24 p.m., Benjamin Hindman wrote:
> > src/files/files.cpp, line 172
> > <https://reviews.apache.org/r/7048/diff/3/?file=153207#file153207line172>
> >
> > const &
see my question below about why this works below
> On Sept. 13, 2012, 7:24 p.m., Benjamin Hindman wrote:
> > src/files/files.cpp, line 177
> > <https://reviews.apache.org/r/7048/diff/3/?file=153207#file153207line177>
> >
> > This isn't complete. It's possible that some number of bytes n which is less than length will get returned and you'll want to keep reading the data until all of length has been read.
true.. seems that io::read is not so useful for files given we typically we want it to handle the read loop for us
how does this all tie in with vinod's async abstraction?
do we want to use that, or expand our poll based io functions?
(I'm asking because it seems we are in need of a new io::read that has a read loop in it, trying to read a minimum amount of data)
- Ben
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/7048/#review11491
-----------------------------------------------------------
On Sept. 13, 2012, 2:57 a.m., Ben Mahler wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/7048/
> -----------------------------------------------------------
>
> (Updated Sept. 13, 2012, 2:57 a.m.)
>
>
> Review request for mesos and Benjamin Hindman.
>
>
> Description
> -------
>
> see above
>
> also:
> -removed redundant length field from json
> -added missing test for buffered io::read
>
>
> Diffs
> -----
>
> src/files/files.cpp 806aa35
> src/tests/files_tests.cpp 6ef2004
> src/webui/master/static/jquery.pailer.js edd23d9
> third_party/libprocess/src/process.cpp 2d2b56c
> third_party/libprocess/src/tests.cpp 41bf973
>
> Diff: https://reviews.apache.org/r/7048/diff/
>
>
> Testing
> -------
>
> make check on osx, and redhat
>
> mesos-local.sh to verify pailer
>
>
> Thanks,
>
> Ben Mahler
>
>
Re: Review Request: Changing FilesProcess to use async io::read
Posted by Benjamin Hindman <be...@berkeley.edu>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/7048/#review11491
-----------------------------------------------------------
src/files/files.cpp
<https://reviews.apache.org/r/7048/#comment24623>
const &
src/files/files.cpp
<https://reviews.apache.org/r/7048/#comment24625>
This isn't complete. It's possible that some number of bytes n which is less than length will get returned and you'll want to keep reading the data until all of length has been read.
src/files/files.cpp
<https://reviews.apache.org/r/7048/#comment24622>
s/nonblock/Failed to make file descriptor non-blocking/
src/files/files.cpp
<https://reviews.apache.org/r/7048/#comment24624>
s/temp/data/
In the future, we want to use something more akin to unique_ptr.
third_party/libprocess/src/process.cpp
<https://reviews.apache.org/r/7048/#comment24626>
const &
third_party/libprocess/src/process.cpp
<https://reviews.apache.org/r/7048/#comment24627>
const &
third_party/libprocess/src/process.cpp
<https://reviews.apache.org/r/7048/#comment24628>
const &
third_party/libprocess/src/process.cpp
<https://reviews.apache.org/r/7048/#comment24629>
PSA: As much as we are able, let's move towards std::tr1 and C++11 std (rather than Boost).
- Benjamin Hindman
On Sept. 13, 2012, 2:57 a.m., Ben Mahler wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/7048/
> -----------------------------------------------------------
>
> (Updated Sept. 13, 2012, 2:57 a.m.)
>
>
> Review request for mesos and Benjamin Hindman.
>
>
> Description
> -------
>
> see above
>
> also:
> -removed redundant length field from json
> -added missing test for buffered io::read
>
>
> Diffs
> -----
>
> src/files/files.cpp 806aa35
> src/tests/files_tests.cpp 6ef2004
> src/webui/master/static/jquery.pailer.js edd23d9
> third_party/libprocess/src/process.cpp 2d2b56c
> third_party/libprocess/src/tests.cpp 41bf973
>
> Diff: https://reviews.apache.org/r/7048/diff/
>
>
> Testing
> -------
>
> make check on osx, and redhat
>
> mesos-local.sh to verify pailer
>
>
> Thanks,
>
> Ben Mahler
>
>
Re: Review Request: Changing FilesProcess to use async io::read
Posted by Ben Mahler <be...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/7048/
-----------------------------------------------------------
(Updated Sept. 13, 2012, 2:57 a.m.)
Review request for mesos and Benjamin Hindman.
Description
-------
see above
also:
-removed redundant length field from json
-added missing test for buffered io::read
Diffs (updated)
-----
src/files/files.cpp 806aa35
src/tests/files_tests.cpp 6ef2004
src/webui/master/static/jquery.pailer.js edd23d9
third_party/libprocess/src/process.cpp 2d2b56c
third_party/libprocess/src/tests.cpp 41bf973
Diff: https://reviews.apache.org/r/7048/diff/
Testing
-------
make check on osx, and redhat
mesos-local.sh to verify pailer
Thanks,
Ben Mahler
Re: Review Request: Changing FilesProcess to use async io::read
Posted by Ben Mahler <be...@gmail.com>.
> On Sept. 13, 2012, 12:13 a.m., Vinod Kone wrote:
> > src/files/files.cpp, line 167
> > <https://reviews.apache.org/r/7048/diff/2/?file=152776#file152776line167>
> >
> > why const here and not for fd or offset?
this is due to a bug in libprocess, snippet from previous review:
benh: "Please add the following at the end of this line:
// TODO(benh): Remove 'const &' after fixing libprocess."
me: "done, lost me on this one..?"
benh: "Normally you wouldn't need to make a primitive like size_t be const &, but in this case I only have Future::then versions for arguments that are const &, so that's why it has to be const &. This is a bug on my side."
so I've added the same todo here now to clarify
> On Sept. 13, 2012, 12:13 a.m., Vinod Kone wrote:
> > third_party/libprocess/src/tests.cpp, line 1236
> > <https://reviews.apache.org/r/7048/diff/2/?file=152779#file152779line1236>
> >
> > CHECK(data.size() == 128) ? :)
hahah, alright
> On Sept. 13, 2012, 12:13 a.m., Vinod Kone wrote:
> > src/files/files.cpp, line 301
> > <https://reviews.apache.org/r/7048/diff/2/?file=152776#file152776line301>
> >
> > memory leak?
great catch, this is actually a memory leak when the future is failed or discarded!
Fixed here and in process.cpp as well where we have another leak, it's definitely very tricky for me to understand whether the shared_ptr and shared_array usage is correct with binds.. please take a hard look, and I'll have benh look as well
- Ben
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/7048/#review11443
-----------------------------------------------------------
On Sept. 12, 2012, 12:23 a.m., Ben Mahler wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/7048/
> -----------------------------------------------------------
>
> (Updated Sept. 12, 2012, 12:23 a.m.)
>
>
> Review request for mesos and Benjamin Hindman.
>
>
> Description
> -------
>
> see above
>
> also:
> -removed redundant length field from json
> -added missing test for buffered io::read
>
>
> Diffs
> -----
>
> src/files/files.cpp 806aa35
> src/tests/files_tests.cpp 6ef2004
> src/webui/master/static/jquery.pailer.js edd23d9
> third_party/libprocess/src/tests.cpp 41bf973
>
> Diff: https://reviews.apache.org/r/7048/diff/
>
>
> Testing
> -------
>
> make check on osx, and redhat
>
> mesos-local.sh to verify pailer
>
>
> Thanks,
>
> Ben Mahler
>
>
Re: Review Request: Changing FilesProcess to use async io::read
Posted by Vinod Kone <vi...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/7048/#review11443
-----------------------------------------------------------
src/files/files.cpp
<https://reviews.apache.org/r/7048/#comment24483>
why const here and not for fd or offset?
src/files/files.cpp
<https://reviews.apache.org/r/7048/#comment24484>
memory leak?
third_party/libprocess/src/tests.cpp
<https://reviews.apache.org/r/7048/#comment24485>
CHECK(data.size() == 128) ? :)
- Vinod Kone
On Sept. 12, 2012, 12:23 a.m., Ben Mahler wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/7048/
> -----------------------------------------------------------
>
> (Updated Sept. 12, 2012, 12:23 a.m.)
>
>
> Review request for mesos and Benjamin Hindman.
>
>
> Description
> -------
>
> see above
>
> also:
> -removed redundant length field from json
> -added missing test for buffered io::read
>
>
> Diffs
> -----
>
> src/files/files.cpp 806aa35
> src/tests/files_tests.cpp 6ef2004
> src/webui/master/static/jquery.pailer.js edd23d9
> third_party/libprocess/src/tests.cpp 41bf973
>
> Diff: https://reviews.apache.org/r/7048/diff/
>
>
> Testing
> -------
>
> make check on osx, and redhat
>
> mesos-local.sh to verify pailer
>
>
> Thanks,
>
> Ben Mahler
>
>