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/10/10 03:07:16 UTC
Re: Review Request: Changing FilesProcess to use async io::read,
updated pailer to not use redundant 'length' field of response.
-----------------------------------------------------------
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,
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/#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. 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/
-----------------------------------------------------------
(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