You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@mesos.apache.org by Benno Evers <be...@mesosphere.com> on 2018/11/22 01:20:12 UTC
Re: Review Request 68795: Added deduplication for read-only master
requests.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/68795/
-----------------------------------------------------------
(Updated Nov. 22, 2018, 1:20 a.m.)
Review request for mesos, Alexander Rukletsov and Joseph Wu.
Changes
-------
Rebased onto latest master.
Repository: mesos
Description
-------
This change will skip the computation of an HTTP response
for requests with the same principal, endpoint and request
headers where both requests are authenticated within the
same batching windows.
Diffs (updated)
-----
src/master/http.cpp 68ee2a6dcffbc772afec6e797b1af8da48f61937
src/master/master.hpp 3b3c1a4e61de9503c8d038dd3bee623ded5914c9
Diff: https://reviews.apache.org/r/68795/diff/3/
Changes: https://reviews.apache.org/r/68795/diff/2-3/
Testing
-------
Thanks,
Benno Evers
Re: Review Request 68795: Added deduplication for read-only master
requests.
Posted by Joseph Wu <jo...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/68795/#review211031
-----------------------------------------------------------
src/master/http.cpp
Lines 2359-2362 (original), 2361-2364 (patched)
<https://reviews.apache.org/r/68795/#comment295882>
Nit: Remove this comment. And similarly above sections.
- Joseph Wu
On Nov. 21, 2018, 5:20 p.m., Benno Evers wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/68795/
> -----------------------------------------------------------
>
> (Updated Nov. 21, 2018, 5:20 p.m.)
>
>
> Review request for mesos, Alexander Rukletsov and Joseph Wu.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> This change will skip the computation of an HTTP response
> for requests with the same principal, endpoint and request
> headers where both requests are authenticated within the
> same batching windows.
>
>
> Diffs
> -----
>
> src/master/http.cpp 68ee2a6dcffbc772afec6e797b1af8da48f61937
> src/master/master.hpp 3b3c1a4e61de9503c8d038dd3bee623ded5914c9
>
>
> Diff: https://reviews.apache.org/r/68795/diff/3/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Benno Evers
>
>
Re: Review Request 68795: Added deduplication for read-only master
requests.
Posted by Benno Evers <be...@mesosphere.com>.
> On Nov. 30, 2018, 2:03 a.m., Joseph Wu wrote:
> > src/master/http.cpp
> > Lines 2392 (patched)
> > <https://reviews.apache.org/r/68795/diff/3/?file=2109719#file2109719line2392>
> >
> > Micro nit: s/Note that this is/NOTE: This is/
Counter-nit: Would be cool to have these as issues, just so I get to click on 'Fixed' afterwards ;) Anyways, fixed.
> On Nov. 30, 2018, 2:03 a.m., Joseph Wu wrote:
> > src/master/http.cpp
> > Lines 2401 (patched)
> > <https://reviews.apache.org/r/68795/diff/3/?file=2109719#file2109719line2401>
> >
> > You might want to leave a note here, or elsewhere, to state that only `BODY` type Responses should be used here. When we copy the `Future<Response>` to multiple recipients, we effectively copy the `std::string body` that comprises the response text.
> >
> > This might also work for `PATH` type responses, but since we'd be copying the path to a file, we'd still be reading the same file twice.
> >
> > The `PIPE` response would be the one that should be avoided. When you copy the `Option<Pipe::Reader> reader`, this does not duplicate the stream. So multiple readers would each get separate parts of the stream, and neither would get a cohesive response (or if lucky, one reader would read everything, and the other reader(s) would get nothing).
Added a comment in the header, in the hope that it's most visible there.
- Benno
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/68795/#review210960
-----------------------------------------------------------
On Dec. 12, 2018, 8:48 p.m., Benno Evers wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/68795/
> -----------------------------------------------------------
>
> (Updated Dec. 12, 2018, 8:48 p.m.)
>
>
> Review request for mesos, Alexander Rukletsov and Joseph Wu.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> This change will skip the computation of an HTTP response
> for requests with the same principal, endpoint and request
> headers where both requests are authenticated within the
> same batching windows.
>
>
> Diffs
> -----
>
> src/master/http.cpp 68ee2a6dcffbc772afec6e797b1af8da48f61937
> src/master/master.hpp c7becfa615964674dcf1ebd9424aa5818a0fdb85
>
>
> Diff: https://reviews.apache.org/r/68795/diff/4/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Benno Evers
>
>
Re: Review Request 68795: Added deduplication for read-only master
requests.
Posted by Joseph Wu <jo...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/68795/#review210960
-----------------------------------------------------------
The `PIPE` issue below is currently not a problem, since none of these ReadOnly handlers chunk their responses.
src/master/http.cpp
Lines 2392 (patched)
<https://reviews.apache.org/r/68795/#comment295783>
Micro nit: s/Note that this is/NOTE: This is/
src/master/http.cpp
Lines 2401 (patched)
<https://reviews.apache.org/r/68795/#comment295787>
You might want to leave a note here, or elsewhere, to state that only `BODY` type Responses should be used here. When we copy the `Future<Response>` to multiple recipients, we effectively copy the `std::string body` that comprises the response text.
This might also work for `PATH` type responses, but since we'd be copying the path to a file, we'd still be reading the same file twice.
The `PIPE` response would be the one that should be avoided. When you copy the `Option<Pipe::Reader> reader`, this does not duplicate the stream. So multiple readers would each get separate parts of the stream, and neither would get a cohesive response (or if lucky, one reader would read everything, and the other reader(s) would get nothing).
- Joseph Wu
On Nov. 21, 2018, 5:20 p.m., Benno Evers wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/68795/
> -----------------------------------------------------------
>
> (Updated Nov. 21, 2018, 5:20 p.m.)
>
>
> Review request for mesos, Alexander Rukletsov and Joseph Wu.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> This change will skip the computation of an HTTP response
> for requests with the same principal, endpoint and request
> headers where both requests are authenticated within the
> same batching windows.
>
>
> Diffs
> -----
>
> src/master/http.cpp 68ee2a6dcffbc772afec6e797b1af8da48f61937
> src/master/master.hpp 3b3c1a4e61de9503c8d038dd3bee623ded5914c9
>
>
> Diff: https://reviews.apache.org/r/68795/diff/3/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Benno Evers
>
>
Re: Review Request 68795: Added deduplication for read-only master
requests.
Posted by Greg Mann <gr...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/68795/#review211373
-----------------------------------------------------------
Fix it, then Ship it!
src/master/http.cpp
Lines 2385-2395 (patched)
<https://reviews.apache.org/r/68795/#comment296379>
Could you add a comment here which notes that this is effectively adding a layer of authorization permissions caching? Since we don't directly compare equality of approvers (which seems difficult to accomplish), we only check for equality of the principal, which implies that the permissions for the principal have remained constant.
I think this is fine because in most cases the deferred requests will be satisfied relatively quickly, but in the case of a heavily-loaded master this implicit "caching timeout" could grow to seconds in duration. Let's be explicit about this and note it in a comment.
- Greg Mann
On Dec. 12, 2018, 8:48 p.m., Benno Evers wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/68795/
> -----------------------------------------------------------
>
> (Updated Dec. 12, 2018, 8:48 p.m.)
>
>
> Review request for mesos, Alexander Rukletsov and Joseph Wu.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> This change will skip the computation of an HTTP response
> for requests with the same principal, endpoint and request
> headers where both requests are authenticated within the
> same batching windows.
>
>
> Diffs
> -----
>
> src/master/http.cpp 68ee2a6dcffbc772afec6e797b1af8da48f61937
> src/master/master.hpp c7becfa615964674dcf1ebd9424aa5818a0fdb85
>
>
> Diff: https://reviews.apache.org/r/68795/diff/4/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Benno Evers
>
>
Re: Review Request 68795: Added deduplication for read-only master
requests.
Posted by Benno Evers <be...@mesosphere.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/68795/
-----------------------------------------------------------
(Updated Dec. 18, 2018, 6:33 p.m.)
Review request for mesos, Alexander Rukletsov and Joseph Wu.
Changes
-------
Added comment about authorization caching.
Repository: mesos
Description
-------
This change will skip the computation of an HTTP response
for requests with the same principal, endpoint and request
headers where both requests are authenticated within the
same batching windows.
Diffs (updated)
-----
src/master/http.cpp e38513bf8feda7ffc55d9f571e41ff970df28299
src/master/master.hpp 99549ab857b16d722f0dd991f98dbe54e9ed19a1
Diff: https://reviews.apache.org/r/68795/diff/5/
Changes: https://reviews.apache.org/r/68795/diff/4-5/
Testing
-------
Thanks,
Benno Evers
Re: Review Request 68795: Added deduplication for read-only master
requests.
Posted by Benno Evers <be...@mesosphere.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/68795/
-----------------------------------------------------------
(Updated Dec. 12, 2018, 8:48 p.m.)
Review request for mesos, Alexander Rukletsov and Joseph Wu.
Changes
-------
Addressed feedback.
Repository: mesos
Description
-------
This change will skip the computation of an HTTP response
for requests with the same principal, endpoint and request
headers where both requests are authenticated within the
same batching windows.
Diffs (updated)
-----
src/master/http.cpp 68ee2a6dcffbc772afec6e797b1af8da48f61937
src/master/master.hpp c7becfa615964674dcf1ebd9424aa5818a0fdb85
Diff: https://reviews.apache.org/r/68795/diff/4/
Changes: https://reviews.apache.org/r/68795/diff/3-4/
Testing
-------
Thanks,
Benno Evers