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/15 01:48:34 UTC
Review Request: Download endpoint for files.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/7123/
-----------------------------------------------------------
Review request for mesos, Benjamin Hindman and John Sirois.
Description
-------
Also added a 100MB file read/download limit.
This addresses bug MESOS-275.
https://issues.apache.org/jira/browse/MESOS-275
Diffs
-----
src/files/files.cpp 806aa35
src/tests/files_tests.cpp 6ef2004
third_party/libprocess/include/stout/os.hpp d08b0cb
Diff: https://reviews.apache.org/r/7123/diff/
Testing
-------
added download test
Thanks,
Ben Mahler
Re: Review Request: Download endpoint for files.
Posted by Ben Mahler <be...@gmail.com>.
> On Oct. 10, 2012, 7:42 p.m., Vinod Kone wrote:
> > src/files/files.cpp, line 289
> > <https://reviews.apache.org/r/7123/diff/4/?file=175145#file175145line289>
> >
> > Suggestion: Make BadRequest() smart enough to include "\n" if not present at the end.
> >
> > It is kinda confusing, for users to know when to use it?
Done! I made a parent ErrorResponse that does this logic.
- Ben
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/7123/#review12317
-----------------------------------------------------------
On Oct. 9, 2012, 11:16 p.m., Ben Mahler wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/7123/
> -----------------------------------------------------------
>
> (Updated Oct. 9, 2012, 11:16 p.m.)
>
>
> Review request for mesos, Benjamin Hindman and Vinod Kone.
>
>
> Description
> -------
>
> Uses the HTTP PATH type response instead.
>
>
> This addresses bug MESOS-275.
> https://issues.apache.org/jira/browse/MESOS-275
>
>
> Diffs
> -----
>
> src/files/files.cpp c40b7b74485dc717c4291f17a4533b52f2c51a7e
> src/tests/files_tests.cpp 17d284b82de4d9e1dedf1c26f4cedcf8269c4072
> src/tests/utils.hpp 63f8d881da9b06f941bb104546ae03c47d140de0
> third_party/libprocess/include/stout/os.hpp 13dbc715ed08cfe6b24ee20744d427dac1104694
> third_party/libprocess/src/process.cpp fde7154f9a4432acaff09849c0fa69c0694c9834
>
> Diff: https://reviews.apache.org/r/7123/diff/
>
>
> Testing
> -------
>
> added download test
>
>
> Thanks,
>
> Ben Mahler
>
>
Re: Review Request: Download endpoint for files.
Posted by Vinod Kone <vi...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/7123/#review12317
-----------------------------------------------------------
Ship it!
src/files/files.cpp
<https://reviews.apache.org/r/7123/#comment26087>
Suggestion: Make BadRequest() smart enough to include "\n" if not present at the end.
It is kinda confusing, for users to know when to use it?
- Vinod Kone
On Oct. 9, 2012, 11:16 p.m., Ben Mahler wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/7123/
> -----------------------------------------------------------
>
> (Updated Oct. 9, 2012, 11:16 p.m.)
>
>
> Review request for mesos, Benjamin Hindman and Vinod Kone.
>
>
> Description
> -------
>
> Uses the HTTP PATH type response instead.
>
>
> This addresses bug MESOS-275.
> https://issues.apache.org/jira/browse/MESOS-275
>
>
> Diffs
> -----
>
> src/files/files.cpp c40b7b74485dc717c4291f17a4533b52f2c51a7e
> src/tests/files_tests.cpp 17d284b82de4d9e1dedf1c26f4cedcf8269c4072
> src/tests/utils.hpp 63f8d881da9b06f941bb104546ae03c47d140de0
> third_party/libprocess/include/stout/os.hpp 13dbc715ed08cfe6b24ee20744d427dac1104694
> third_party/libprocess/src/process.cpp fde7154f9a4432acaff09849c0fa69c0694c9834
>
> Diff: https://reviews.apache.org/r/7123/diff/
>
>
> Testing
> -------
>
> added download test
>
>
> Thanks,
>
> Ben Mahler
>
>
Re: Review Request: Download endpoint for files.
Posted by Ben Mahler <be...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/7123/
-----------------------------------------------------------
(Updated Oct. 12, 2012, 10:10 p.m.)
Review request for mesos, Benjamin Hindman and Vinod Kone.
Changes
-------
Reverted the abstract ErrorResponse and added newlines as appropriate to the response bodies.
Description
-------
Uses the HTTP PATH type response instead.
This addresses bug MESOS-275.
https://issues.apache.org/jira/browse/MESOS-275
Diffs (updated)
-----
src/files/files.cpp 6d41bf6649479f83b1f5f56665b555c0c553e713
src/tests/files_tests.cpp 764d2c7fdec4a21b9698eb4cb9e0673bd7f20e9d
src/tests/utils.hpp 63f8d881da9b06f941bb104546ae03c47d140de0
third_party/libprocess/include/process/http.hpp 3d2922507bdb904e5bdebefbafb6ccae98c021c4
third_party/libprocess/include/stout/os.hpp 3bc63a07e6614802497b168ab1f8372bb0c6ff24
third_party/libprocess/src/decoder.hpp 6db3db6ca0ca8b9a426af103e554e971f66c8ec0
third_party/libprocess/src/process.cpp e887feb1070cdd03a6d81b8f798145ed8bda7b5c
Diff: https://reviews.apache.org/r/7123/diff/
Testing
-------
added download test
Thanks,
Ben Mahler
Re: Review Request: Download endpoint for files.
Posted by Ben Mahler <be...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/7123/
-----------------------------------------------------------
(Updated Oct. 11, 2012, 12:39 a.m.)
Review request for mesos, Benjamin Hindman and Vinod Kone.
Changes
-------
Adds a trailing newline on error messages in HTTP responses.
Description
-------
Uses the HTTP PATH type response instead.
This addresses bug MESOS-275.
https://issues.apache.org/jira/browse/MESOS-275
Diffs (updated)
-----
src/files/files.cpp 6d41bf6649479f83b1f5f56665b555c0c553e713
src/tests/files_tests.cpp 764d2c7fdec4a21b9698eb4cb9e0673bd7f20e9d
src/tests/utils.hpp 63f8d881da9b06f941bb104546ae03c47d140de0
third_party/libprocess/include/process/http.hpp 3d2922507bdb904e5bdebefbafb6ccae98c021c4
third_party/libprocess/include/stout/os.hpp 3bc63a07e6614802497b168ab1f8372bb0c6ff24
third_party/libprocess/src/decoder.hpp 6db3db6ca0ca8b9a426af103e554e971f66c8ec0
third_party/libprocess/src/process.cpp fde7154f9a4432acaff09849c0fa69c0694c9834
Diff: https://reviews.apache.org/r/7123/diff/
Testing
-------
added download test
Thanks,
Ben Mahler
Re: Review Request: Download endpoint for files.
Posted by Ben Mahler <be...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/7123/
-----------------------------------------------------------
(Updated Oct. 9, 2012, 11:16 p.m.)
Review request for mesos, Benjamin Hindman and Vinod Kone.
Changes
-------
Much cleaner now!
This requires benh's decoder header fix for the tests to pass.
Description (updated)
-------
Uses the HTTP PATH type response instead.
This addresses bug MESOS-275.
https://issues.apache.org/jira/browse/MESOS-275
Diffs (updated)
-----
src/files/files.cpp c40b7b74485dc717c4291f17a4533b52f2c51a7e
src/tests/files_tests.cpp 17d284b82de4d9e1dedf1c26f4cedcf8269c4072
src/tests/utils.hpp 63f8d881da9b06f941bb104546ae03c47d140de0
third_party/libprocess/include/stout/os.hpp 13dbc715ed08cfe6b24ee20744d427dac1104694
third_party/libprocess/src/process.cpp fde7154f9a4432acaff09849c0fa69c0694c9834
Diff: https://reviews.apache.org/r/7123/diff/
Testing
-------
added download test
Thanks,
Ben Mahler
Re: Review Request: Download endpoint for files.
Posted by Ben Mahler <be...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/7123/
-----------------------------------------------------------
(Updated Sept. 18, 2012, 3:15 a.m.)
Review request for mesos, Benjamin Hindman and John Sirois.
Changes
-------
added some (pretty ugly) callbacks for closing the file
Description
-------
Also added a 100MB file read/download limit.
This addresses bug MESOS-275.
https://issues.apache.org/jira/browse/MESOS-275
Diffs (updated)
-----
src/files/files.cpp 806aa35
src/tests/files_tests.cpp 15a378f
src/tests/utils.hpp fc2023b
third_party/libprocess/include/stout/os.hpp 5b909b9
Diff: https://reviews.apache.org/r/7123/diff/
Testing
-------
added download test
Thanks,
Ben Mahler
Re: Review Request: Download endpoint for files.
Posted by Ben Mahler <be...@gmail.com>.
> On Sept. 17, 2012, 9:47 p.m., Benjamin Hindman wrote:
> > src/files/files.cpp, line 355
> > <https://reviews.apache.org/r/7123/diff/2/?file=155599#file155599line355>
> >
> > Close the file please.
- Ben
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/7123/#review11630
-----------------------------------------------------------
On Sept. 14, 2012, 11:48 p.m., Ben Mahler wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/7123/
> -----------------------------------------------------------
>
> (Updated Sept. 14, 2012, 11:48 p.m.)
>
>
> Review request for mesos, Benjamin Hindman and John Sirois.
>
>
> Description
> -------
>
> Also added a 100MB file read/download limit.
>
>
> This addresses bug MESOS-275.
> https://issues.apache.org/jira/browse/MESOS-275
>
>
> Diffs
> -----
>
> src/files/files.cpp 806aa35
> src/tests/files_tests.cpp 6ef2004
> third_party/libprocess/include/stout/os.hpp d08b0cb
>
> Diff: https://reviews.apache.org/r/7123/diff/
>
>
> Testing
> -------
>
> added download test
>
>
> Thanks,
>
> Ben Mahler
>
>
Re: Review Request: Download endpoint for files.
Posted by Benjamin Hindman <be...@berkeley.edu>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/7123/#review11630
-----------------------------------------------------------
src/files/files.cpp
<https://reviews.apache.org/r/7123/#comment25118>
Use process::mime::types please.
src/files/files.cpp
<https://reviews.apache.org/r/7123/#comment25116>
Please put '{' on new line.
src/files/files.cpp
<https://reviews.apache.org/r/7123/#comment25119>
s/read/download/
src/files/files.cpp
<https://reviews.apache.org/r/7123/#comment25120>
s/read/download/
src/files/files.cpp
<https://reviews.apache.org/r/7123/#comment25129>
Close the file please.
src/files/files.cpp
<https://reviews.apache.org/r/7123/#comment25132>
Need to pass the file descriptor to 'reply' so that you can close it. This is a great example of when you might want to do an .onDiscarded and .onFailed to close the file descriptor followed by a .then (we need C++11 now!).
src/tests/files_tests.cpp
<https://reviews.apache.org/r/7123/#comment25124>
Just like we have EXPECT_RESPONSE_STATUS_WILL_EQ and EXPECT_RESPONSE_BODY_WILL_EQ, I could imagine an EXPECT_RESPONSE_CONTENT_TYPE_WILL_EQ ... or something like 'EXPECT_RESPONSE_HEADER_WILL_EQ(expected, "Content-Type", response)' instead ... see my next comment for why wrapping this in EXPECT_RESPONSE_*_WILL_* might be a good idea.
src/tests/files_tests.cpp
<https://reviews.apache.org/r/7123/#comment25123>
You could use EXPECT_RESPONSE_BODY_WILL_EQ now. Of course, this is pretty simple and easy to ready. The upside to using EXPECT_RESPONSE_BODY_WILL_EQ is really that it makes sure you do an 'await' on the future, so as we move code around in tests you don't have to make sure that this EXPECT_EQ is done after something which has awaited on the future.
third_party/libprocess/include/stout/os.hpp
<https://reviews.apache.org/r/7123/#comment25133>
Please put '{' on newline.
- Benjamin Hindman
On Sept. 14, 2012, 11:48 p.m., Ben Mahler wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/7123/
> -----------------------------------------------------------
>
> (Updated Sept. 14, 2012, 11:48 p.m.)
>
>
> Review request for mesos, Benjamin Hindman and John Sirois.
>
>
> Description
> -------
>
> Also added a 100MB file read/download limit.
>
>
> This addresses bug MESOS-275.
> https://issues.apache.org/jira/browse/MESOS-275
>
>
> Diffs
> -----
>
> src/files/files.cpp 806aa35
> src/tests/files_tests.cpp 6ef2004
> third_party/libprocess/include/stout/os.hpp d08b0cb
>
> Diff: https://reviews.apache.org/r/7123/diff/
>
>
> Testing
> -------
>
> added download test
>
>
> Thanks,
>
> Ben Mahler
>
>