You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@mesos.apache.org by Alexander Rukletsov <ru...@gmail.com> on 2017/12/13 13:39:39 UTC
Review Request 64574: Ensured trailing '/' in URL is insignificant.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/64574/
-----------------------------------------------------------
Review request for mesos, Benjamin Bannier and Benjamin Mahler.
Bugs: MESOS-5333
https://issues.apache.org/jira/browse/MESOS-5333
Repository: mesos
Description
-------
Prior to this patch, adding a trailing '/' to a valid URL path, e.g.,
"/state/", yielded a 404 response. This patch ensures that two URLs
which differ only in trailing '/' produce the same result.
Diffs
-----
3rdparty/libprocess/src/process.cpp 75cf1d3b6d3d257ba9bc81c68017a74a6511cebf
3rdparty/libprocess/src/tests/http_tests.cpp 9daac715f0242921b7f9f5c20b3eb27f1be802d4
Diff: https://reviews.apache.org/r/64574/diff/1/
Testing
-------
Ensured the modified test fails without the fix.
`make check` on Mac OS 10.11.6 and several Linux distributions.
Thanks,
Alexander Rukletsov
Re: Review Request 64574: Ensured trailing '/' in URL is
insignificant.
Posted by Benjamin Bannier <be...@mesosphere.io>.
> On Dec. 13, 2017, 11:07 p.m., Benjamin Bannier wrote:
> > Do we have a good argument for support of trailing slashes in leave-like endpoints like e.g., `/master/maintenance/schedule`?
> >
> > I can see how serving both the version with and without trailing slash might make sense for "directory-like" (non-leave) endpoints like `/help`, but am not convinced of allowing the same for leave-like endpoints. Looking at the linked ticket I could currently think of more reasons to close it as `WONT_FIX` than for having this patch.
I thought some more about this, and would agree with the semantics proposed here.
The thinking would be that a trailing slash would only be surprising in an URL ending in a file-like token (e.g., `index.html`); for all others one could expect the versions with and without trailing slash to be equivalent. We do not have file-like URLs in Mesos, so the proposed fix looks good.
- Benjamin
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/64574/#review193742
-----------------------------------------------------------
On Dec. 13, 2017, 2:39 p.m., Alexander Rukletsov wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/64574/
> -----------------------------------------------------------
>
> (Updated Dec. 13, 2017, 2:39 p.m.)
>
>
> Review request for mesos, Benjamin Bannier and Benjamin Mahler.
>
>
> Bugs: MESOS-5333
> https://issues.apache.org/jira/browse/MESOS-5333
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Prior to this patch, adding a trailing '/' to a valid URL path, e.g.,
> "/state/", yielded a 404 response. This patch ensures that two URLs
> which differ only in trailing '/' produce the same result.
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/src/process.cpp 75cf1d3b6d3d257ba9bc81c68017a74a6511cebf
> 3rdparty/libprocess/src/tests/http_tests.cpp 9daac715f0242921b7f9f5c20b3eb27f1be802d4
>
>
> Diff: https://reviews.apache.org/r/64574/diff/1/
>
>
> Testing
> -------
>
> Ensured the modified test fails without the fix.
> `make check` on Mac OS 10.11.6 and several Linux distributions.
>
>
> Thanks,
>
> Alexander Rukletsov
>
>
Re: Review Request 64574: Ensured trailing '/' in URL is
insignificant.
Posted by Benjamin Bannier <be...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/64574/#review193742
-----------------------------------------------------------
Do we have a good argument for support of trailing slashes in leave-like endpoints like e.g., `/master/maintenance/schedule`?
I can see how serving both the version with and without trailing slash might make sense for "directory-like" (non-leave) endpoints like `/help`, but am not convinced of allowing the same for leave-like endpoints. Looking at the linked ticket I could currently think of more reasons to close it as `WONT_FIX` than for having this patch.
- Benjamin Bannier
On Dec. 13, 2017, 2:39 p.m., Alexander Rukletsov wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/64574/
> -----------------------------------------------------------
>
> (Updated Dec. 13, 2017, 2:39 p.m.)
>
>
> Review request for mesos, Benjamin Bannier and Benjamin Mahler.
>
>
> Bugs: MESOS-5333
> https://issues.apache.org/jira/browse/MESOS-5333
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Prior to this patch, adding a trailing '/' to a valid URL path, e.g.,
> "/state/", yielded a 404 response. This patch ensures that two URLs
> which differ only in trailing '/' produce the same result.
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/src/process.cpp 75cf1d3b6d3d257ba9bc81c68017a74a6511cebf
> 3rdparty/libprocess/src/tests/http_tests.cpp 9daac715f0242921b7f9f5c20b3eb27f1be802d4
>
>
> Diff: https://reviews.apache.org/r/64574/diff/1/
>
>
> Testing
> -------
>
> Ensured the modified test fails without the fix.
> `make check` on Mac OS 10.11.6 and several Linux distributions.
>
>
> Thanks,
>
> Alexander Rukletsov
>
>
Re: Review Request 64574: Ensured trailing '/' in URL is
insignificant.
Posted by Benjamin Mahler <bm...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/64574/#review196087
-----------------------------------------------------------
Fix it, then Ship it!
Approach looks good to me! A couple of suggestions:
(1) Can you update the description to reflect the updated approach? Specifically, disallowing route() calls from using a trailing slash?
(2) Would be nice for the commit summary to reflect this is about libprocess routing (as opposed to say, some other component that touches URLs):
```
Updated libprocess routing to treat trailing '/' as insignificant.
```
(3) Might be nice to split the tests out into a separate patch to make it so that if we cherry-pick it's easier to choose whether we cherry-pick back the tests.
3rdparty/libprocess/src/process.cpp
Lines 3551 (patched)
<https://reviews.apache.org/r/64574/#comment275600>
A little more context here would be helpful for the reader:
```
We enforce that routes for ".../path" are resolved when the user provides a trailing slash ".../path/". The trailing slash is stripped here in order to accomplish this.
```
3rdparty/libprocess/src/process.cpp
Lines 3746-3748 (patched)
<https://reviews.apache.org/r/64574/#comment275601>
Can you document this at the API level?
https://github.com/apache/mesos/blob/9e18db59f75b85fc6467272203366eef1c17233e/3rdparty/libprocess/include/process/process.hpp#L270-L275
3rdparty/libprocess/src/tests/http_tests.cpp
Line 260 (original), 264 (patched)
<https://reviews.apache.org/r/64574/#comment275602>
A little more context here about the semantics provided by libprocess' routing would be helpful.
- Benjamin Mahler
On Jan. 18, 2018, 5:46 p.m., Alexander Rukletsov wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/64574/
> -----------------------------------------------------------
>
> (Updated Jan. 18, 2018, 5:46 p.m.)
>
>
> Review request for mesos, Benjamin Bannier and Benjamin Mahler.
>
>
> Bugs: MESOS-5333
> https://issues.apache.org/jira/browse/MESOS-5333
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Prior to this patch, adding a trailing '/' to a valid URL path, e.g.,
> "/state/", yielded a 404 response. This patch ensures that two URLs
> which differ only in trailing '/' produce the same result.
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/src/process.cpp 2126c272a69bfa53b4b3cbbb1a55a3f81a5da8ad
> 3rdparty/libprocess/src/tests/http_tests.cpp f79d366baecc73e6b14df593a906264f5fd1d72b
>
>
> Diff: https://reviews.apache.org/r/64574/diff/2/
>
>
> Testing
> -------
>
> Ensured the modified test fails without the fix.
> `make check` on Mac OS 10.11.6 and several Linux distributions.
>
>
> Thanks,
>
> Alexander Rukletsov
>
>
Re: Review Request 64574: Ensured trailing '/' in URL is
insignificant.
Posted by Benno Evers <be...@mesosphere.com>.
> On Jan. 24, 2018, 2:24 p.m., Benno Evers wrote:
> > Ship It!
Modulo Ben Mahlers comments, of course.
- Benno
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/64574/#review196115
-----------------------------------------------------------
On Jan. 18, 2018, 5:46 p.m., Alexander Rukletsov wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/64574/
> -----------------------------------------------------------
>
> (Updated Jan. 18, 2018, 5:46 p.m.)
>
>
> Review request for mesos, Benjamin Bannier and Benjamin Mahler.
>
>
> Bugs: MESOS-5333
> https://issues.apache.org/jira/browse/MESOS-5333
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Prior to this patch, adding a trailing '/' to a valid URL path, e.g.,
> "/state/", yielded a 404 response. This patch ensures that two URLs
> which differ only in trailing '/' produce the same result.
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/src/process.cpp 2126c272a69bfa53b4b3cbbb1a55a3f81a5da8ad
> 3rdparty/libprocess/src/tests/http_tests.cpp f79d366baecc73e6b14df593a906264f5fd1d72b
>
>
> Diff: https://reviews.apache.org/r/64574/diff/2/
>
>
> Testing
> -------
>
> Ensured the modified test fails without the fix.
> `make check` on Mac OS 10.11.6 and several Linux distributions.
>
>
> Thanks,
>
> Alexander Rukletsov
>
>
Re: Review Request 64574: Ensured trailing '/' in URL is
insignificant.
Posted by Benno Evers <be...@mesosphere.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/64574/#review196115
-----------------------------------------------------------
Ship it!
Ship It!
- Benno Evers
On Jan. 18, 2018, 5:46 p.m., Alexander Rukletsov wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/64574/
> -----------------------------------------------------------
>
> (Updated Jan. 18, 2018, 5:46 p.m.)
>
>
> Review request for mesos, Benjamin Bannier and Benjamin Mahler.
>
>
> Bugs: MESOS-5333
> https://issues.apache.org/jira/browse/MESOS-5333
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Prior to this patch, adding a trailing '/' to a valid URL path, e.g.,
> "/state/", yielded a 404 response. This patch ensures that two URLs
> which differ only in trailing '/' produce the same result.
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/src/process.cpp 2126c272a69bfa53b4b3cbbb1a55a3f81a5da8ad
> 3rdparty/libprocess/src/tests/http_tests.cpp f79d366baecc73e6b14df593a906264f5fd1d72b
>
>
> Diff: https://reviews.apache.org/r/64574/diff/2/
>
>
> Testing
> -------
>
> Ensured the modified test fails without the fix.
> `make check` on Mac OS 10.11.6 and several Linux distributions.
>
>
> Thanks,
>
> Alexander Rukletsov
>
>
Re: Review Request 64574: Updated libprocess routing to treat
trailing '/' as insignificant.
Posted by Alexander Rukletsov <ru...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/64574/
-----------------------------------------------------------
(Updated Jan. 25, 2018, 7:03 p.m.)
Review request for mesos, Benjamin Bannier and Benjamin Mahler.
Bugs: MESOS-5333
https://issues.apache.org/jira/browse/MESOS-5333
Repository: mesos
Description
-------
Prior to this patch, adding a trailing '/' to a valid URL path, e.g.,
"/state/", yielded a 404 response. This patch ensures that two URLs
which differ only in trailing '/' produce the same result.
This is achieved by (1) trimming a trailing '/' when processing a
request and (2) disallowing `route()` calls from using `/`.
Diffs (updated)
-----
3rdparty/libprocess/include/process/process.hpp 8b8323f5fb4617aa9c22bbdb56af3d1cb0b06ea3
3rdparty/libprocess/src/process.cpp 52b492749a6f4f019e147e69e94c7cef2eaee8da
3rdparty/libprocess/src/tests/http_tests.cpp f79d366baecc73e6b14df593a906264f5fd1d72b
Diff: https://reviews.apache.org/r/64574/diff/3/
Changes: https://reviews.apache.org/r/64574/diff/2-3/
Testing
-------
Ensured the modified test fails without the fix.
`make check` on Mac OS 10.11.6 and several Linux distributions.
Thanks,
Alexander Rukletsov
Re: Review Request 64574: Updated libprocess routing to treat
trailing '/' as insignificant.
Posted by Alexander Rukletsov <ru...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/64574/
-----------------------------------------------------------
(Updated Jan. 25, 2018, 6:47 p.m.)
Review request for mesos, Benjamin Bannier and Benjamin Mahler.
Summary (updated)
-----------------
Updated libprocess routing to treat trailing '/' as insignificant.
Bugs: MESOS-5333
https://issues.apache.org/jira/browse/MESOS-5333
Repository: mesos
Description (updated)
-------
Prior to this patch, adding a trailing '/' to a valid URL path, e.g.,
"/state/", yielded a 404 response. This patch ensures that two URLs
which differ only in trailing '/' produce the same result.
This is achieved by (1) trimming a trailing '/' when processing a
request and (2) disallowing `route()` calls from using `/`.
Diffs
-----
3rdparty/libprocess/src/process.cpp 2126c272a69bfa53b4b3cbbb1a55a3f81a5da8ad
3rdparty/libprocess/src/tests/http_tests.cpp f79d366baecc73e6b14df593a906264f5fd1d72b
Diff: https://reviews.apache.org/r/64574/diff/2/
Testing
-------
Ensured the modified test fails without the fix.
`make check` on Mac OS 10.11.6 and several Linux distributions.
Thanks,
Alexander Rukletsov
Re: Review Request 64574: Ensured trailing '/' in URL is
insignificant.
Posted by Mesos Reviewbot Windows <re...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/64574/#review195744
-----------------------------------------------------------
PASS: Mesos patch 64574 was successfully built and tested.
Reviews applied: `['64574']`
All the build artifacts available at: http://dcos-win.westus.cloudapp.azure.com/mesos-build/review/64574
- Mesos Reviewbot Windows
On Jan. 18, 2018, 5:46 p.m., Alexander Rukletsov wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/64574/
> -----------------------------------------------------------
>
> (Updated Jan. 18, 2018, 5:46 p.m.)
>
>
> Review request for mesos, Benjamin Bannier and Benjamin Mahler.
>
>
> Bugs: MESOS-5333
> https://issues.apache.org/jira/browse/MESOS-5333
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Prior to this patch, adding a trailing '/' to a valid URL path, e.g.,
> "/state/", yielded a 404 response. This patch ensures that two URLs
> which differ only in trailing '/' produce the same result.
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/src/process.cpp 2126c272a69bfa53b4b3cbbb1a55a3f81a5da8ad
> 3rdparty/libprocess/src/tests/http_tests.cpp f79d366baecc73e6b14df593a906264f5fd1d72b
>
>
> Diff: https://reviews.apache.org/r/64574/diff/2/
>
>
> Testing
> -------
>
> Ensured the modified test fails without the fix.
> `make check` on Mac OS 10.11.6 and several Linux distributions.
>
>
> Thanks,
>
> Alexander Rukletsov
>
>
Re: Review Request 64574: Ensured trailing '/' in URL is
insignificant.
Posted by Alexander Rukletsov <ru...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/64574/
-----------------------------------------------------------
(Updated Jan. 18, 2018, 5:46 p.m.)
Review request for mesos, Benjamin Bannier and Benjamin Mahler.
Bugs: MESOS-5333
https://issues.apache.org/jira/browse/MESOS-5333
Repository: mesos
Description
-------
Prior to this patch, adding a trailing '/' to a valid URL path, e.g.,
"/state/", yielded a 404 response. This patch ensures that two URLs
which differ only in trailing '/' produce the same result.
Diffs (updated)
-----
3rdparty/libprocess/src/process.cpp 2126c272a69bfa53b4b3cbbb1a55a3f81a5da8ad
3rdparty/libprocess/src/tests/http_tests.cpp f79d366baecc73e6b14df593a906264f5fd1d72b
Diff: https://reviews.apache.org/r/64574/diff/2/
Changes: https://reviews.apache.org/r/64574/diff/1-2/
Testing
-------
Ensured the modified test fails without the fix.
`make check` on Mac OS 10.11.6 and several Linux distributions.
Thanks,
Alexander Rukletsov
Re: Review Request 64574: Ensured trailing '/' in URL is
insignificant.
Posted by Alexander Rukletsov <ru...@gmail.com>.
> On Dec. 14, 2017, 9:40 p.m., Benjamin Mahler wrote:
> > Can you link to some prior art? What do the RFCs say recommend, if anything? What do other http frameworks do?
According to [RFC3986](https://tools.ietf.org/html/rfc3986#section-3.3), every '/' is meaningful, i.e., `/path` and `/path/` have different number of segments: ["path"] vs. ["path", ""]. However in practice it is recommended to treat paths with leading and trailing slashes in the same way to avoid user confusion, see [google's recommendation](https://webmasters.googleblog.com/2010/04/to-slash-or-not-to-slash.html). It is also recommended to redirect one request to another to avoid duplicate content, but I don't think it is relevant for us.
- Alexander
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/64574/#review193848
-----------------------------------------------------------
On Dec. 13, 2017, 1:39 p.m., Alexander Rukletsov wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/64574/
> -----------------------------------------------------------
>
> (Updated Dec. 13, 2017, 1:39 p.m.)
>
>
> Review request for mesos, Benjamin Bannier and Benjamin Mahler.
>
>
> Bugs: MESOS-5333
> https://issues.apache.org/jira/browse/MESOS-5333
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Prior to this patch, adding a trailing '/' to a valid URL path, e.g.,
> "/state/", yielded a 404 response. This patch ensures that two URLs
> which differ only in trailing '/' produce the same result.
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/src/process.cpp 75cf1d3b6d3d257ba9bc81c68017a74a6511cebf
> 3rdparty/libprocess/src/tests/http_tests.cpp 9daac715f0242921b7f9f5c20b3eb27f1be802d4
>
>
> Diff: https://reviews.apache.org/r/64574/diff/1/
>
>
> Testing
> -------
>
> Ensured the modified test fails without the fix.
> `make check` on Mac OS 10.11.6 and several Linux distributions.
>
>
> Thanks,
>
> Alexander Rukletsov
>
>
Re: Review Request 64574: Ensured trailing '/' in URL is
insignificant.
Posted by Benjamin Mahler <bm...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/64574/#review193848
-----------------------------------------------------------
Can you link to some prior art? What do the RFCs say recommend, if anything? What do other http frameworks do?
- Benjamin Mahler
On Dec. 13, 2017, 1:39 p.m., Alexander Rukletsov wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/64574/
> -----------------------------------------------------------
>
> (Updated Dec. 13, 2017, 1:39 p.m.)
>
>
> Review request for mesos, Benjamin Bannier and Benjamin Mahler.
>
>
> Bugs: MESOS-5333
> https://issues.apache.org/jira/browse/MESOS-5333
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Prior to this patch, adding a trailing '/' to a valid URL path, e.g.,
> "/state/", yielded a 404 response. This patch ensures that two URLs
> which differ only in trailing '/' produce the same result.
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/src/process.cpp 75cf1d3b6d3d257ba9bc81c68017a74a6511cebf
> 3rdparty/libprocess/src/tests/http_tests.cpp 9daac715f0242921b7f9f5c20b3eb27f1be802d4
>
>
> Diff: https://reviews.apache.org/r/64574/diff/1/
>
>
> Testing
> -------
>
> Ensured the modified test fails without the fix.
> `make check` on Mac OS 10.11.6 and several Linux distributions.
>
>
> Thanks,
>
> Alexander Rukletsov
>
>
Re: Review Request 64574: Ensured trailing '/' in URL is
insignificant.
Posted by Mesos Reviewbot Windows <re...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/64574/#review193678
-----------------------------------------------------------
FAIL: Some Mesos tests failed.
Reviews applied: `['64574']`
Failed command: `D:\DCOS\mesos\src\mesos-tests.exe --verbose`
All the build artifacts available at: http://dcos-win.westus.cloudapp.azure.com/mesos-build/review/64574
Relevant logs:
- [mesos-tests-stdout.log](http://dcos-win.westus.cloudapp.azure.com/mesos-build/review/64574/logs/mesos-tests-stdout.log):
```
[----------] 1 test from IsolationFlag/CpuIsolatorTest
[ RUN ] IsolationFlag/CpuIsolatorTest.ROOT_UserCpuUsage/0
[ OK ] IsolationFlag/CpuIsolatorTest.ROOT_UserCpuUsage/0 (2550 ms)
[----------] 1 test from IsolationFlag/CpuIsolatorTest (2573 ms total)
[----------] 1 test from IsolationFlag/MemoryIsolatorTest
[ RUN ] IsolationFlag/MemoryIsolatorTest.ROOT_MemUsage/0
[ OK ] IsolationFlag/MemoryIsolatorTest.ROOT_MemUsage/0 (2476 ms)
[----------] 1 test from IsolationFlag/MemoryIsolatorTest (2500 ms total)
[----------] Global test environment tear-down
[==========] 829 tests from 84 test cases ran. (334251 ms total)
[ PASSED ] 819 tests.
[ FAILED ] 10 tests, listed below:
[ FAILED ] OfferOperationStatusUpdateManagerTest.UpdateAndAckNonTerminalUpdate
[ FAILED ] OfferOperationStatusUpdateManagerTest.RecoverCheckpointedStream
[ FAILED ] OfferOperationStatusUpdateManagerTest.RecoverEmptyFile
[ FAILED ] OfferOperationStatusUpdateManagerTest.RecoverTerminatedStream
[ FAILED ] OfferOperationStatusUpdateManagerTest.IgnoreDuplicateUpdate
[ FAILED ] OfferOperationStatusUpdateManagerTest.IgnoreDuplicateUpdateAfterRecover
[ FAILED ] OfferOperationStatusUpdateManagerTest.RejectDuplicateAck
[ FAILED ] OfferOperationStatusUpdateManagerTest.RejectDuplicateAckAfterRecover
[ FAILED ] OfferOperationStatusUpdateManagerTest.NonStrictRecoveryCorruptedFile
[ FAILED ] SlaveTest.ResourceProviderPublishAll
10 FAILED TESTS
YOU HAVE 201 DISABLED TESTS
```
- [mesos-tests-stderr.log](http://dcos-win.westus.cloudapp.azure.com/mesos-build/review/64574/logs/mesos-tests-stderr.log):
```
I1213 14:32:19.697907 432 master.cpp:10158] Updating the state of task 0f46e60I1213 14:32:18.942925 2320 exec.cpp:162] Version: 1.5.0
I1213 14:32:18.967929 3752 exec.cpp:237] Executor registered on agent ae551b65-58c1-41a5-a388-e31c3ff41e65-S0
I1213 14:32:18.971071 5216 executor.cpp:171] Received SUBSCRIBED event
I1213 14:32:18.974937 5216 executor.cpp:175] Subscribed executor on build-srv-03.zq4gs31qjdiunm1ryi1452nvnh.dx.internal.cloudapp.net
I1213 14:32:18.975925 5216 executor.cpp:171] Received LAUNCH event
I1213 14:32:18.979897 5216 executor.cpp:637] Starting task 0f46e603-1ec5-4ac8-b5d1-652231a56ca4
I1213 14:32:19.060920 5216 executor.cpp:477] Running 'D:\DCOS\mesos\src\mesos-containerizer.exe launch <POSSIBLY-SENSITIVE-DATA>'
I1213 14:32:19.670886 5216 executor.cpp:650] Forked command at 4200
I1213 14:32:19.699883 2320 exec.cpp:435] Executor asked to shutdown
I1213 14:32:19.700882 8220 executor.cpp:171] Received SHUTDOWN event
I1213 14:32:19.700882 8220 executor.cpp:747] Shutting down
I1213 14:32:19.700882 8220 executor.cpp:854] Sending SIGTERM to process tree at pid 43-1ec5-4ac8-b5d1-652231a56ca4 of framework ae551b65-58c1-41a5-a388-e31c3ff41e65-0000 (latest state: TASK_KILLED, status update state: TASK_KILLED)
I1213 14:32:19.697907 7724 slave.cpp:3400] Shutting down framework ae551b65-58c1-41a5-a388-e31c3ff41e65-0000
I1213 14:32:19.698895 7724 slave.cpp:6114] Shutting down executor '0f46e603-1ec5-4ac8-b5d1-652231a56ca4' of framework ae551b65-58c1-41a5-a388-e31c3ff41e65-0000 at executor(1)@10.3.1.11:64393
I1213 14:32:19.698895 7724 slave.cpp:909] Agent terminating
W1213 14:32:19.699883 7724 slave.cpp:3396] Ignoring shutdown framework ae551b65-58c1-41a5-a388-e31c3ff41e65-0000 because it is terminating
I1213 14:32:19.700882 432 master.cpp:10264] Removing task 0f46e603-1ec5-4ac8-b5d1-652231a56ca4 with resources cpus(allocated: *):4; mem(allocated: *):2048; disk(allocated: *):1024; ports(allocated: *):[31000-32000] of framework ae551b65-58c1-41a5-a388-e31c3ff41e65-0000 on agent ae551b65-58c1-41a5-a388-e31c3ff41e65-S0 at slave(326)@10.3.1.11:64372 (build-srv-03.zq4gs31qjdiunm1ryi1452nvnh.dx.internal.cloudapp.net)
I1213 14:32:19.702881 6800 containerizer.cpp:2337] Destroying container 9741fef6-75d3-4aad-9265-8f3d1550be74 in RUNNING state
I1213 14:32:19.702881 6800 containerizer.cpp:2939] Transitioning the state of container 9741fef6-75d3-4aad-9265-8f3d1550be74 from RUNNING to DESTROYING
I1213 14:32:19.703905 6800 launcher.cpp:156] Asked to destroy container 9741fef6-75d3-4aad-9265-8f3d1550be74
I1213 14:32:19.703905 432 master.cpp:1305] Agent ae551b65-58c1-41a5-a388-e31c3ff41e65-S0 at slave(326)@10.3.1.11:64372 (build-srv-03.zq4gs31qjdiunm1ryi1452nvnh.dx.internal.cloudapp.net) disconnected
I1213 14:32:19.703905 432 master.cpp:3364] Disconnecting agent ae551b65-58c1-41a5-a388-e31c3ff41e65-S0 at slave(326)@10.3.1.11:64372 (build-srv-03.zq4gs31qjdiunm1ryi1452nvnh.dx.internal.cloudapp.net)
I1213 14:32:19.703905 1860 hierarchical.cpp:344] Removed framework ae551b65-58c1-41a5-a388-e31c3ff41e65-0000
I1213 14:32:19.703905 432 master.cpp:3383] Deactivating agent ae551b65-58c1-41a5-a388-e31c3ff41e65-S0 at slave(326)@10.3.1.11:64372 (build-srv-03.zq4gs31qjdiunm1ryi1452nvnh.dx.internal.cloudapp.net)
I1213 14:32:19.704882 2428 hierarchical.cpp:766] Agent ae551b65-58c1-41a5-a388-e31c3ff41e65-S0 deactivated
I1213 14:32:19.705881 1860 containerizer.cpp:2788] Container 9741fef6-75d3-4aad-9265-8f3d1550be74 has exited
I1213 14:32:19.733886 3928 master.cpp:1147] Master terminating
I1213 14:32:19.735883 5344 hierarchical.cpp:609] Removed agent ae551b65-58c1-41a5-a388-e31c3ff41e65-S0
I1213 14:32:20.082010 3644 process.cpp:887] Failed to accept socket: future discarded
```
- Mesos Reviewbot Windows
On Dec. 13, 2017, 1:39 p.m., Alexander Rukletsov wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/64574/
> -----------------------------------------------------------
>
> (Updated Dec. 13, 2017, 1:39 p.m.)
>
>
> Review request for mesos, Benjamin Bannier and Benjamin Mahler.
>
>
> Bugs: MESOS-5333
> https://issues.apache.org/jira/browse/MESOS-5333
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Prior to this patch, adding a trailing '/' to a valid URL path, e.g.,
> "/state/", yielded a 404 response. This patch ensures that two URLs
> which differ only in trailing '/' produce the same result.
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/src/process.cpp 75cf1d3b6d3d257ba9bc81c68017a74a6511cebf
> 3rdparty/libprocess/src/tests/http_tests.cpp 9daac715f0242921b7f9f5c20b3eb27f1be802d4
>
>
> Diff: https://reviews.apache.org/r/64574/diff/1/
>
>
> Testing
> -------
>
> Ensured the modified test fails without the fix.
> `make check` on Mac OS 10.11.6 and several Linux distributions.
>
>
> Thanks,
>
> Alexander Rukletsov
>
>
Re: Review Request 64574: Ensured trailing '/' in URL is
insignificant.
Posted by Benjamin Mahler <bm...@apache.org>.
> On Dec. 19, 2017, 1:08 a.m., Benjamin Mahler wrote:
> > 3rdparty/libprocess/src/process.cpp
> > Lines 3530-3531 (patched)
> > <https://reviews.apache.org/r/64574/diff/1/?file=1915259#file1915259line3531>
> >
> > This approach doesn't seem quite right to me, since it prevents the user of libprocess from seeing what the original path was. Have you considered any other approaches? (also I think you only needed to trim from the tail?)
>
> Alexander Rukletsov wrote:
> Why so? `name` is a local variable based on request path and used to find appropriate handler. A user of libprocess can still access `request->url.path`.
>
> I do trim from the tail only (see two lines above).
>
> I did consider returning 301 with a trimmed path, but decided not to since I'm not sure we're allowed to return 301 codes in response to operator API endpoints.
Ah yes, I see that it's a local variable now, sorry about that!
A couple of questions:
(1) What happens on the `route()` side? Can a caller setup two routes for "path" and "path/"? And if so, what happens when requests come in for "path" and "path/"?
(2) Maybe we could preserve the original trimming of prefix (since it belongs with the removal of the "ID" comment), and instead add the suffix stripping along with a comment about the overall idea here? As it stands, I think the reader will have a hard time realizing that we've decided to treat trailing slash paths the same as non-trailing slash paths?
- Benjamin
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/64574/#review194106
-----------------------------------------------------------
On Dec. 13, 2017, 1:39 p.m., Alexander Rukletsov wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/64574/
> -----------------------------------------------------------
>
> (Updated Dec. 13, 2017, 1:39 p.m.)
>
>
> Review request for mesos, Benjamin Bannier and Benjamin Mahler.
>
>
> Bugs: MESOS-5333
> https://issues.apache.org/jira/browse/MESOS-5333
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Prior to this patch, adding a trailing '/' to a valid URL path, e.g.,
> "/state/", yielded a 404 response. This patch ensures that two URLs
> which differ only in trailing '/' produce the same result.
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/src/process.cpp 75cf1d3b6d3d257ba9bc81c68017a74a6511cebf
> 3rdparty/libprocess/src/tests/http_tests.cpp 9daac715f0242921b7f9f5c20b3eb27f1be802d4
>
>
> Diff: https://reviews.apache.org/r/64574/diff/1/
>
>
> Testing
> -------
>
> Ensured the modified test fails without the fix.
> `make check` on Mac OS 10.11.6 and several Linux distributions.
>
>
> Thanks,
>
> Alexander Rukletsov
>
>
Re: Review Request 64574: Ensured trailing '/' in URL is
insignificant.
Posted by Alexander Rukletsov <ru...@gmail.com>.
> On Dec. 19, 2017, 1:08 a.m., Benjamin Mahler wrote:
> > 3rdparty/libprocess/src/process.cpp
> > Lines 3530-3531 (patched)
> > <https://reviews.apache.org/r/64574/diff/1/?file=1915259#file1915259line3531>
> >
> > This approach doesn't seem quite right to me, since it prevents the user of libprocess from seeing what the original path was. Have you considered any other approaches? (also I think you only needed to trim from the tail?)
Why so? `name` is a local variable based on request path and used to find appropriate handler. A user of libprocess can still access `request->url.path`.
I do trim from the tail only (see two lines above).
I did consider returning 301 with a trimmed path, but decided not to since I'm not sure we're allowed to return 301 codes in response to operator API endpoints.
- Alexander
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/64574/#review194106
-----------------------------------------------------------
On Dec. 13, 2017, 1:39 p.m., Alexander Rukletsov wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/64574/
> -----------------------------------------------------------
>
> (Updated Dec. 13, 2017, 1:39 p.m.)
>
>
> Review request for mesos, Benjamin Bannier and Benjamin Mahler.
>
>
> Bugs: MESOS-5333
> https://issues.apache.org/jira/browse/MESOS-5333
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Prior to this patch, adding a trailing '/' to a valid URL path, e.g.,
> "/state/", yielded a 404 response. This patch ensures that two URLs
> which differ only in trailing '/' produce the same result.
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/src/process.cpp 75cf1d3b6d3d257ba9bc81c68017a74a6511cebf
> 3rdparty/libprocess/src/tests/http_tests.cpp 9daac715f0242921b7f9f5c20b3eb27f1be802d4
>
>
> Diff: https://reviews.apache.org/r/64574/diff/1/
>
>
> Testing
> -------
>
> Ensured the modified test fails without the fix.
> `make check` on Mac OS 10.11.6 and several Linux distributions.
>
>
> Thanks,
>
> Alexander Rukletsov
>
>
Re: Review Request 64574: Ensured trailing '/' in URL is
insignificant.
Posted by Alexander Rukletsov <ru...@gmail.com>.
> On Dec. 19, 2017, 1:08 a.m., Benjamin Mahler wrote:
> > 3rdparty/libprocess/src/process.cpp
> > Lines 3530-3531 (patched)
> > <https://reviews.apache.org/r/64574/diff/1/?file=1915259#file1915259line3531>
> >
> > This approach doesn't seem quite right to me, since it prevents the user of libprocess from seeing what the original path was. Have you considered any other approaches? (also I think you only needed to trim from the tail?)
>
> Alexander Rukletsov wrote:
> Why so? `name` is a local variable based on request path and used to find appropriate handler. A user of libprocess can still access `request->url.path`.
>
> I do trim from the tail only (see two lines above).
>
> I did consider returning 301 with a trimmed path, but decided not to since I'm not sure we're allowed to return 301 codes in response to operator API endpoints.
>
> Benjamin Mahler wrote:
> Ah yes, I see that it's a local variable now, sorry about that!
>
> A couple of questions:
>
> (1) What happens on the `route()` side? Can a caller setup two routes for "path" and "path/"? And if so, what happens when requests come in for "path" and "path/"?
>
> (2) Maybe we could preserve the original trimming of prefix (since it belongs with the removal of the "ID" comment), and instead add the suffix stripping along with a comment about the overall idea here? As it stands, I think the reader will have a hard time realizing that we've decided to treat trailing slash paths the same as non-trailing slash paths?
(1) Currently, libprocess allows to route both `"/path"` and `"/path/"` and moreover to different handlers. With this patch, route to `"/path/"` is not wired and hence has no effects. We can do following things to improve the logic here:
- disallow `"/path/"` in `route()`
- normalize `"/path/"` to `"/path"` in `route()` (might result in handler overwrite)
- allow both `"/path/"` and `"/path"` and try both if `request.path` handler is not found
I have a light tendency to the first option because of its simplicity. Note that we already normalize `"/path/"` to `"/path"` on [the help page](https://github.com/apache/mesos/blob/634c8af2618c57a1405d20717fa909b399486f37/3rdparty/libprocess/src/help.cpp#L95-L98).
(2) Sure.
- Alexander
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/64574/#review194106
-----------------------------------------------------------
On Dec. 13, 2017, 1:39 p.m., Alexander Rukletsov wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/64574/
> -----------------------------------------------------------
>
> (Updated Dec. 13, 2017, 1:39 p.m.)
>
>
> Review request for mesos, Benjamin Bannier and Benjamin Mahler.
>
>
> Bugs: MESOS-5333
> https://issues.apache.org/jira/browse/MESOS-5333
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Prior to this patch, adding a trailing '/' to a valid URL path, e.g.,
> "/state/", yielded a 404 response. This patch ensures that two URLs
> which differ only in trailing '/' produce the same result.
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/src/process.cpp 75cf1d3b6d3d257ba9bc81c68017a74a6511cebf
> 3rdparty/libprocess/src/tests/http_tests.cpp 9daac715f0242921b7f9f5c20b3eb27f1be802d4
>
>
> Diff: https://reviews.apache.org/r/64574/diff/1/
>
>
> Testing
> -------
>
> Ensured the modified test fails without the fix.
> `make check` on Mac OS 10.11.6 and several Linux distributions.
>
>
> Thanks,
>
> Alexander Rukletsov
>
>
Re: Review Request 64574: Ensured trailing '/' in URL is
insignificant.
Posted by Benjamin Mahler <bm...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/64574/#review194106
-----------------------------------------------------------
3rdparty/libprocess/src/process.cpp
Lines 3530-3531 (patched)
<https://reviews.apache.org/r/64574/#comment272830>
This approach doesn't seem quite right to me, since it prevents the user of libprocess from seeing what the original path was. Have you considered any other approaches? (also I think you only needed to trim from the tail?)
- Benjamin Mahler
On Dec. 13, 2017, 1:39 p.m., Alexander Rukletsov wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/64574/
> -----------------------------------------------------------
>
> (Updated Dec. 13, 2017, 1:39 p.m.)
>
>
> Review request for mesos, Benjamin Bannier and Benjamin Mahler.
>
>
> Bugs: MESOS-5333
> https://issues.apache.org/jira/browse/MESOS-5333
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Prior to this patch, adding a trailing '/' to a valid URL path, e.g.,
> "/state/", yielded a 404 response. This patch ensures that two URLs
> which differ only in trailing '/' produce the same result.
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/src/process.cpp 75cf1d3b6d3d257ba9bc81c68017a74a6511cebf
> 3rdparty/libprocess/src/tests/http_tests.cpp 9daac715f0242921b7f9f5c20b3eb27f1be802d4
>
>
> Diff: https://reviews.apache.org/r/64574/diff/1/
>
>
> Testing
> -------
>
> Ensured the modified test fails without the fix.
> `make check` on Mac OS 10.11.6 and several Linux distributions.
>
>
> Thanks,
>
> Alexander Rukletsov
>
>
Re: Review Request 64574: Ensured trailing '/' in URL is
insignificant.
Posted by Benjamin Bannier <be...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/64574/#review193778
-----------------------------------------------------------
Ship it!
Ship It!
- Benjamin Bannier
On Dec. 13, 2017, 2:39 p.m., Alexander Rukletsov wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/64574/
> -----------------------------------------------------------
>
> (Updated Dec. 13, 2017, 2:39 p.m.)
>
>
> Review request for mesos, Benjamin Bannier and Benjamin Mahler.
>
>
> Bugs: MESOS-5333
> https://issues.apache.org/jira/browse/MESOS-5333
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Prior to this patch, adding a trailing '/' to a valid URL path, e.g.,
> "/state/", yielded a 404 response. This patch ensures that two URLs
> which differ only in trailing '/' produce the same result.
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/src/process.cpp 75cf1d3b6d3d257ba9bc81c68017a74a6511cebf
> 3rdparty/libprocess/src/tests/http_tests.cpp 9daac715f0242921b7f9f5c20b3eb27f1be802d4
>
>
> Diff: https://reviews.apache.org/r/64574/diff/1/
>
>
> Testing
> -------
>
> Ensured the modified test fails without the fix.
> `make check` on Mac OS 10.11.6 and several Linux distributions.
>
>
> Thanks,
>
> Alexander Rukletsov
>
>