Re: Review Request 45000: MESOS-3902: [Updated] Fix location header in redirect from non-leader.
Thanks! On Fri, Mar 25, 2016 at 10:18 AM, Vinod Kone <vinodk...@gmail.com> wrote: > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/45000/ > > Ship it! > > Thanks for following up with a real test! > > Btw, no need for that NOTE as the bug is attached to this review and the > commit will be pasted to that ticket. I'll remove it while committing. > > > - Vinod Kone > > On March 24th, 2016, 2:24 a.m. UTC, Ashwin Murthy wrote: > Review request for mesos and Vinod Kone. > By Ashwin Murthy. > > *Updated March 24, 2016, 2:24 a.m.* > *Repository: * mesos > Description > > MESOS-3902: [Final] Fix location header in redirect from non-leader. Fixed to > append the path from the original URL. > > Testing > > This has been tested end to end with 3 local mesos masters and zk. Valided > the location header is now correct. > > Diffs > >- src/master/http.cpp (97e4b0ce3286540788e3e6c5b484687b83e000f6) > > View Diff <https://reviews.apache.org/r/45000/diff/> >
Re: Review Request 45000: MESOS-3902: [Updated] Fix location header in redirect from non-leader.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/45000/ --- (Updated March 24, 2016, 2:24 a.m.) Review request for mesos and Vinod Kone. Summary (updated) - MESOS-3902: [Updated] Fix location header in redirect from non-leader. Repository: mesos Description (updated) --- MESOS-3902: [Final] Fix location header in redirect from non-leader. Fixed to append the path from the original URL. Diffs (updated) - src/master/http.cpp 97e4b0ce3286540788e3e6c5b484687b83e000f6 Diff: https://reviews.apache.org/r/45000/diff/ Testing (updated) --- This has been tested end to end with 3 local mesos masters and zk. Valided the location header is now correct. Thanks, Ashwin Murthy
Re: Review Request 45000: MESOS-3902: Fix in location header during redirect from non-leader.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/45000/ --- (Updated March 24, 2016, 2:19 a.m.) Review request for mesos and Vinod Kone. Repository: mesos Description --- MESOS-3902: [Updated] Fix location header in redirect from non-leader. Addressed comments from Vinod and Ben. Removed the scheme but added the path from the original request to the location header. Preserved the link to the RFC in the comments. Diffs - src/master/http.cpp bfea8fa811c24d37b2d7a8109728e1f40217e02f Diff: https://reviews.apache.org/r/45000/diff/ Testing (updated) --- This has been tested end to end Thanks, Ashwin Murthy
Re: Review Request 45000: MESOS-3902: Fix in location header during redirect from non-leader.
> On March 20, 2016, 11:43 p.m., Vinod Kone wrote: > > Can you confirm that you did a manual test of this (using curl and multiple > > masters w/ ZK)? Vinod, validated the fix. Here is the o/p from testing with 3 local mesos masters and zookeeper $ curl -v -X POST -H "Content-Type: application/json" --data @body.json http://localhost:5050/api/v1/scheduler * Trying ::1... * connect to ::1 port 5050 failed: Connection refused * Trying 127.0.0.1... * Connected to localhost (127.0.0.1) port 5050 (#0) > POST /api/v1/scheduler HTTP/1.1 > Host: localhost:5050 > User-Agent: curl/7.43.0 > Accept: */* > Content-Type: application/json > Content-Length: 120 > * upload completely sent off: 120 out of 120 bytes < HTTP/1.1 307 Temporary Redirect < Date: Thu, 24 Mar 2016 02:01:08 GMT < Location: //localhost:5052/master/api/v1/scheduler <<<< On March 20, 2016, 11:43 p.m., Vinod Kone wrote: > > src/master/http.cpp, lines 970-972 > > <https://reviews.apache.org/r/45000/diff/3/?file=1307586#file1307586line970> > > > > looks like this fits in one line, why the wrapping? > > > > ``` > > return TemporaryRedirect( > > "//" + hostname.get() + ":" + stringify(info.port()) + > > request.url.path); > > ``` I guess I can make it fit - Ashwin --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/45000/#review124472 --- On March 20, 2016, 11:03 p.m., Ashwin Murthy wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/45000/ > --- > > (Updated March 20, 2016, 11:03 p.m.) > > > Review request for mesos and Vinod Kone. > > > Repository: mesos > > > Description > --- > > MESOS-3902: [Updated] Fix location header in redirect from non-leader. > Addressed comments from Vinod and Ben. Removed the scheme but added the path > from the original request to the location header. Preserved the link to the > RFC in the comments. > > > Diffs > - > > src/master/http.cpp bfea8fa811c24d37b2d7a8109728e1f40217e02f > > Diff: https://reviews.apache.org/r/45000/diff/ > > > Testing > --- > > Was trying to write unit test in scheduler http api test but it turns out > multi master tests cannot be written at this point. Need to manually verify > this by setting up multiple masters with ZK. > > > Thanks, > > Ashwin Murthy > >
Re: Review Request 45000: MESOS-3902: Fix in location header during redirect from non-leader.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/45000/ --- (Updated March 20, 2016, 11:03 p.m.) Review request for mesos and Vinod Kone. Repository: mesos Description (updated) --- MESOS-3902: [Updated] Fix location header in redirect from non-leader. Addressed comments from Vinod and Ben. Removed the scheme but added the path from the original request to the location header. Preserved the link to the RFC in the comments. Diffs (updated) - src/master/http.cpp bfea8fa811c24d37b2d7a8109728e1f40217e02f Diff: https://reviews.apache.org/r/45000/diff/ Testing --- Was trying to write unit test in scheduler http api test but it turns out multi master tests cannot be written at this point. Need to manually verify this by setting up multiple masters with ZK. Thanks, Ashwin Murthy
Re: Review Request 45000: MESOS-3902: Fix in location header during redirect from non-leader.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/45000/ --- (Updated March 20, 2016, 10:07 p.m.) Review request for mesos and Vinod Kone. Repository: mesos Description --- MESOS-3902: Fix in location header during redirect from non-leader. Appended the path from original request to the location header Diffs - src/master/http.cpp bfea8fa811c24d37b2d7a8109728e1f40217e02f src/slave/containerizer/mesos/containerizer.cpp ee7a265975323ca891114a286357c8e42901560c src/tests/containerizer/mesos_containerizer_tests.cpp f3ca32b2d8b8ef9dcfa8f20d9ceaff48b6598a66 Diff: https://reviews.apache.org/r/45000/diff/ Testing --- Was trying to write unit test in scheduler http api test but it turns out multi master tests cannot be written at this point. Need to manually verify this by setting up multiple masters with ZK. Thanks, Ashwin Murthy
Re: Review Request 45000: MESOS-3902: Fix in location header during redirect from non-leader.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/45000/ --- (Updated March 20, 2016, 9:57 p.m.) Review request for mesos and Vinod Kone. Repository: mesos Description (updated) --- MESOS-3902: Fix in location header during redirect from non-leader. Appended the path from original request to the location header Diffs (updated) - src/master/http.cpp bfea8fa811c24d37b2d7a8109728e1f40217e02f src/slave/containerizer/mesos/containerizer.cpp ee7a265975323ca891114a286357c8e42901560c src/tests/containerizer/mesos_containerizer_tests.cpp f3ca32b2d8b8ef9dcfa8f20d9ceaff48b6598a66 Diff: https://reviews.apache.org/r/45000/diff/ Testing --- Was trying to write unit test in scheduler http api test but it turns out multi master tests cannot be written at this point. Need to manually verify this by setting up multiple masters with ZK. Thanks, Ashwin Murthy
Re: Review Request 45078: MESOS-3902: Fix location header in redirect from non-leader.
> On March 20, 2016, 5:51 p.m., Vinod Kone wrote: > > Looks like you created a new review instead of updating the existing review > > (https://reviews.apache.org/r/45000/) ? Can you discard this and update the > > old one instead. That way the review/comment history is not lost. You can > > use support/post-reviews.py to make this easy (see: > > http://mesos.apache.org/documentation/latest/submitting-a-patch/). ok will do. - Ashwin --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/45078/#review124455 --- On March 19, 2016, 9:57 p.m., Ashwin Murthy wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/45078/ > --- > > (Updated March 19, 2016, 9:57 p.m.) > > > Review request for mesos, Ben Whitehead, Joris Van Remoortere, and Vinod Kone. > > > Repository: mesos > > > Description > --- > > MESOS-3902: Fix location header in redirect from non-leader. Adding the path > of the original request into the redirect URL. > > > Diffs > - > > src/master/http.cpp bfea8fa811c24d37b2d7a8109728e1f40217e02f > > Diff: https://reviews.apache.org/r/45078/diff/ > > > Testing > ------- > > Manual testing to be done. Testing framework does not support multi-master > tests as yet. > > > Thanks, > > Ashwin Murthy > >
Review Request 45000: MESOS-3902: Fix in location header during redirect from non-leader.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/45000/ --- Review request for mesos and Vinod Kone. Repository: mesos Description --- MESOS-3902: Fix in location header during redirect from non-leader. The fix is to add the scheme and path to the URL in the location header. Previously, we did not have the scheme and path. We need to update the mesos http API documentation section on master redirect to reflect this change. Diffs - src/master/http.cpp b47ab7cc86c0a56a81815a98bd63f37a1175ba7f Diff: https://reviews.apache.org/r/45000/diff/ Testing --- Was trying to write unit test in scheduler http api test but it turns out multi master tests cannot be written at this point. Need to manually verify this by setting up multiple masters with ZK. Thanks, Ashwin Murthy
Review Request 45078: MESOS-3902: Fix location header in redirect from non-leader.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/45078/ --- Review request for mesos, Ben Whitehead, Joris Van Remoortere, and Vinod Kone. Repository: mesos Description --- MESOS-3902: Fix location header in redirect from non-leader. Adding the path of the original request into the redirect URL. Diffs - src/master/http.cpp bfea8fa811c24d37b2d7a8109728e1f40217e02f Diff: https://reviews.apache.org/r/45078/diff/ Testing --- Manual testing to be done. Testing framework does not support multi-master tests as yet. Thanks, Ashwin Murthy
Re: Review Request 45000: MESOS-3902: Fix in location header during redirect from non-leader.
> On March 19, 2016, 1:24 a.m., Ben Whitehead wrote: > > src/master/http.cpp, line 959 > > <https://reviews.apache.org/r/45000/diff/1/?file=1304561#file1304561line959> > > > > According to the rules defined in > > https://tools.ietf.org/html/rfc3986#section-5 we should be fine to use a > > scheme relative location. > > > > I think keeping the link to the RFC is a good idea here, so that the > > next person that reads this code knows explicitly what this code was > > written against. > > Vinod Kone wrote: > So all we no to do is append the request.path at the end right? Isnt it fine to also preserve the original scheme? It is not presciptive either way. I will definitely add the original comment pointing to the RFC back - Ashwin --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/45000/#review124350 ----------- On March 18, 2016, 12:04 a.m., Ashwin Murthy wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/45000/ > --- > > (Updated March 18, 2016, 12:04 a.m.) > > > Review request for mesos and Vinod Kone. > > > Repository: mesos > > > Description > --- > > MESOS-3902: Fix in location header during redirect from non-leader. The fix > is to add the scheme and path to the URL in the location header. Previously, > we did not have the scheme and path. We need to update the mesos http API > documentation section on master redirect to reflect this change. > > > Diffs > - > > src/master/http.cpp b47ab7cc86c0a56a81815a98bd63f37a1175ba7f > > Diff: https://reviews.apache.org/r/45000/diff/ > > > Testing > --- > > Was trying to write unit test in scheduler http api test but it turns out > multi master tests cannot be written at this point. Need to manually verify > this by setting up multiple masters with ZK. > > > Thanks, > > Ashwin Murthy > >
Review Request 44971: MESOS-3902: Fix location header in redirect from non-leading master.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/44971/ --- Review request for mesos and Vinod Kone. Repository: mesos Description --- MESOS-3902: Fix location header in redirect from non-leading master. Added the scheme and path to the URL that was being set in the location header Diffs - src/master/http.cpp 950206baf7f3a1cccdc49d810126473966d8d021 Diff: https://reviews.apache.org/r/44971/diff/ Testing --- None yet. Was attempting to write a test but turns there is no good way to test multi master in our testing framework. Will need to test this manually by launching multiple masters with ZK setup. Doing that in parallel Thanks, Ashwin Murthy
