Re: Review Request 36360: Adding common constants for HTTP API
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36360/#review91782 --- Ship it! src/common/http.hpp https://reviews.apache.org/r/36360/#comment145431 Whoops, this is still used? src/common/http.cpp (line 44) https://reviews.apache.org/r/36360/#comment145432 Whoops, space after =? - Ben Mahler On July 15, 2015, 2:50 a.m., Isabel Jimenez wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36360/ --- (Updated July 15, 2015, 2:50 a.m.) Review request for mesos, Anand Mazumdar, Benjamin Hindman, Ben Mahler, Marco Massenzio, and Vinod Kone. Bugs: MESOS-2860 https://issues.apache.org/jira/browse/MESOS-2860 Repository: mesos-incubating Description --- Adding constants used commonly through the different HTTP endpoints Diffs - src/common/http.hpp bbd063d src/common/http.cpp 73a4de1 Diff: https://reviews.apache.org/r/36360/diff/ Testing --- Thanks, Isabel Jimenez
Re: Review Request 36360: Adding common constants for HTTP API
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36360/ --- (Updated July 15, 2015, 2:50 a.m.) Review request for mesos, Anand Mazumdar, Benjamin Hindman, Ben Mahler, Marco Massenzio, and Vinod Kone. Bugs: MESOS-2860 https://issues.apache.org/jira/browse/MESOS-2860 Repository: mesos-incubating Description --- Adding constants used commonly through the different HTTP endpoints Diffs (updated) - src/common/http.hpp bbd063d src/common/http.cpp 73a4de1 Diff: https://reviews.apache.org/r/36360/diff/ Testing --- Thanks, Isabel Jimenez
Re: Review Request 36360: Adding common constants for HTTP API
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36360/#review91280 --- src/common/http.hpp (lines 52 - 53) https://reviews.apache.org/r/36360/#comment144572 Let's convert them to static functions as per https://issues.apache.org/jira/browse/MESOS-1023. E.g. see https://reviews.apache.org/r/30841/. - Alexander Rukletsov On July 9, 2015, 10:34 p.m., Isabel Jimenez wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36360/ --- (Updated July 9, 2015, 10:34 p.m.) Review request for mesos, Anand Mazumdar, Benjamin Hindman, Ben Mahler, Marco Massenzio, and Vinod Kone. Bugs: MESOS-2860 https://issues.apache.org/jira/browse/MESOS-2860 Repository: mesos-incubating Description --- Adding constants used commonly through the different HTTP endpoints Diffs - src/common/http.hpp bbd063d src/common/http.cpp 73a4de1 Diff: https://reviews.apache.org/r/36360/diff/ Testing --- Thanks, Isabel Jimenez
Re: Review Request 36360: Adding common constants for HTTP API
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36360/#review91204 --- Patch looks great! Reviews applied: [36360] All tests passed. - Mesos ReviewBot On July 9, 2015, 9:07 p.m., Isabel Jimenez wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36360/ --- (Updated July 9, 2015, 9:07 p.m.) Review request for mesos, Anand Mazumdar, Benjamin Hindman, Ben Mahler, Marco Massenzio, and Vinod Kone. Bugs: MESOS-2860 https://issues.apache.org/jira/browse/MESOS-2860 Repository: mesos-incubating Description --- Adding constants used commonly through the different HTTP endpoints Diffs - src/Makefile.am e5b5d36 src/common/http_constants.hpp PRE-CREATION src/common/http_constants.cpp PRE-CREATION Diff: https://reviews.apache.org/r/36360/diff/ Testing --- Thanks, Isabel Jimenez
Re: Review Request 36360: Adding common constants for HTTP API
On July 9, 2015, 8:23 p.m., Anand Mazumdar wrote: src/common/http_constants.cpp, line 26 https://reviews.apache.org/r/36360/diff/1/?file=1003774#file1003774line26 minor nit-pick , might consider using std::string; before-hand ? :) done - Isabel --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36360/#review91178 --- On July 9, 2015, 9:07 p.m., Isabel Jimenez wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36360/ --- (Updated July 9, 2015, 9:07 p.m.) Review request for mesos, Anand Mazumdar, Benjamin Hindman, Ben Mahler, Marco Massenzio, and Vinod Kone. Bugs: MESOS-2860 https://issues.apache.org/jira/browse/MESOS-2860 Repository: mesos-incubating Description --- Adding constants used commonly through the different HTTP endpoints Diffs - src/Makefile.am e5b5d36 src/common/http_constants.hpp PRE-CREATION src/common/http_constants.cpp PRE-CREATION Diff: https://reviews.apache.org/r/36360/diff/ Testing --- Thanks, Isabel Jimenez
Re: Review Request 36360: Adding common constants for HTTP API
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36360/ --- (Updated July 9, 2015, 9:07 p.m.) Review request for mesos, Anand Mazumdar, Benjamin Hindman, Ben Mahler, Marco Massenzio, and Vinod Kone. Bugs: MESOS-2860 https://issues.apache.org/jira/browse/MESOS-2860 Repository: mesos-incubating Description --- Adding constants used commonly through the different HTTP endpoints Diffs (updated) - src/Makefile.am e5b5d36 src/common/http_constants.hpp PRE-CREATION src/common/http_constants.cpp PRE-CREATION Diff: https://reviews.apache.org/r/36360/diff/ Testing --- Thanks, Isabel Jimenez
Re: Review Request 36360: Adding common constants for HTTP API
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36360/#review91205 --- Can you move this into the existing common/http.hpp, and remove the content type one? For content type, would rather see a typed member on Request/Response than constants here, given the other occurrences: ``` ➜ mesos git:(master) ✗ grep -R Content-Type src | grep -v js | grep -v html src/files/files.cpp: response.headers[Content-Type] = application/octet-stream; src/files/files.cpp: response.headers[Content-Type] = mime::types[extension]; src/tests/fault_tolerance_tests.cpp: Content-Type, src/tests/files_tests.cpp: Content-Type, src/tests/files_tests.cpp: AWAIT_EXPECT_RESPONSE_HEADER_EQ(image/gif, Content-Type, response); src/tests/master_tests.cpp: response.get().headers.get(Content-Type)); src/tests/master_tests.cpp: response.get().headers.get(Content-Type)); src/tests/master_tests.cpp: response.get().headers.get(Content-Type)); src/tests/master_tests.cpp: response.get().headers.get(Content-Type)); src/tests/master_tests.cpp: response.get().headers.get(Content-Type)); src/tests/master_tests.cpp: response.get().headers.get(Content-Type)); src/tests/master_tests.cpp: response.get().headers.get(Content-Type)); src/tests/master_tests.cpp: response.get().headers.get(Content-Type)); src/tests/master_tests.cpp: response.get().headers.get(Content-Type)); src/tests/metrics_tests.cpp: response.get().headers.get(Content-Type)); src/tests/metrics_tests.cpp: response.get().headers.get(Content-Type)); src/tests/monitor_tests.cpp: Content-Type, src/tests/monitor_tests.cpp: Content-Type, src/tests/monitor_tests.cpp: Content-Type, src/tests/monitor_tests.cpp: Content-Type, src/tests/repair_tests.cpp:Content-Type, \ src/tests/scheduler_tests.cpp: response.get().headers.get(Content-Type)); src/tests/slave_tests.cpp: response.get().headers.get(Content-Type)); src/tests/slave_tests.cpp: response.get().headers.get(Content-Type)); src/tests/slave_tests.cpp: response.get().headers.get(Content-Type)); src/tests/utils.cpp: response.get().headers.get(Content-Type)); ➜ mesos git:(master) ✗ grep -R Content-Type 3rdparty | grep -v js | grep -v html 3rdparty/libprocess/examples/example.cpp:response.headers[Content-Type] = text/plain; 3rdparty/libprocess/examples/example.cpp: response.headers[Content-Type] = text/plain; 3rdparty/libprocess/include/process/http.hpp: // specify the 'Content-Type' header, but the 'Content-Length' and 3rdparty/libprocess/include/process/http.hpp: headers[Content-Type] = text/javascript; 3rdparty/libprocess/include/process/process.hpp: // '/path/file'. The 'Content-Type' header of the HTTP response will 3rdparty/libprocess/src/help.cpp:response.headers[Content-Type] = text/x-markdown; 3rdparty/libprocess/src/http.cpp: // Overwrite Content-Type if necessary. 3rdparty/libprocess/src/http.cpp:headers[Content-Type] = contentType.get(); 3rdparty/libprocess/src/http.cpp:return Failure(Attempted to do a POST with a Content-Type but no body); 3rdparty/libprocess/src/http.cpp:return Failure(Attempted to do a POST with a Content-Type but no body); 3rdparty/libprocess/src/process.cpp:// While the user is expected to properly set a 'Content-Type' 3rdparty/libprocess/src/process.cpp:// While the user is expected to properly set a 'Content-Type' 3rdparty/libprocess/src/process.cpp:// Try and determine the Content-Type from an extension. 3rdparty/libprocess/src/process.cpp:response.headers[Content-Type] = assets[name].types[extension]; 3rdparty/libprocess/src/profiler.cpp: response.headers[Content-Type] = application/octet-stream; 3rdparty/libprocess/src/tests/decoder_tests.cpp:Content-Type: text/plain\r\n 3rdparty/libprocess/src/tests/decoder_tests.cpp:Content-Type: text/plain\r\n 3rdparty/libprocess/src/tests/decoder_tests.cpp:Content-Type: text/plain\r\n 3rdparty/libprocess/src/tests/http_tests.cpp: headers[Content-Type] = text/plain; 3rdparty/libprocess/src/tests/http_tests.cpp: EXPECT_EQ(text/javascript, response.headers[Content-Type]); 3rdparty/libprocess/src/tests/system_tests.cpp: response.get().headers.get(Content-Type)); ``` - Ben Mahler On July 9, 2015, 9:07 p.m., Isabel Jimenez wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36360/ --- (Updated July 9, 2015, 9:07 p.m.) Review request for mesos, Anand Mazumdar, Benjamin Hindman, Ben Mahler, Marco Massenzio, and Vinod Kone. Bugs: MESOS-2860
Re: Review Request 36360: Adding common constants for HTTP API
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36360/#review91178 --- Ship it! LGTM, Just some minor comments. src/common/http_constants.hpp (line 18) https://reviews.apache.org/r/36360/#comment144466 This is not a self-contained header file that we encourage everyone to build up, kindly do #include string and remove the adjacent include from the cpp file. src/common/http_constants.hpp (line 26) https://reviews.apache.org/r/36360/#comment144463 s/Supported Content-Tye and Accept headers/Supported media types for Content-Type and Accept headers. src/common/http_constants.cpp (line 26) https://reviews.apache.org/r/36360/#comment144465 minor nit-pick , might consider using std::string; before-hand ? - Anand Mazumdar On July 9, 2015, 7:58 p.m., Isabel Jimenez wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36360/ --- (Updated July 9, 2015, 7:58 p.m.) Review request for mesos, Anand Mazumdar, Benjamin Hindman, Ben Mahler, Marco Massenzio, and Vinod Kone. Bugs: MESOS-2860 https://issues.apache.org/jira/browse/MESOS-2860 Repository: mesos-incubating Description --- Adding constants used commonly through the different HTTP endpoints Diffs - src/Makefile.am e5b5d36 src/common/http_constants.hpp PRE-CREATION src/common/http_constants.cpp PRE-CREATION Diff: https://reviews.apache.org/r/36360/diff/ Testing --- Thanks, Isabel Jimenez
Re: Review Request 36360: Adding common constants for HTTP API
On July 9, 2015, 9:32 p.m., Ben Mahler wrote: Can you move this into the existing common/http.hpp, and remove the content type one? For content type, would rather see a typed member on Request/Response than constants here, given the other occurrences: ``` ? mesos git:(master) ? grep -R Content-Type src | grep -v js | grep -v html src/files/files.cpp: response.headers[Content-Type] = application/octet-stream; src/files/files.cpp: response.headers[Content-Type] = mime::types[extension]; src/tests/fault_tolerance_tests.cpp: Content-Type, src/tests/files_tests.cpp: Content-Type, src/tests/files_tests.cpp: AWAIT_EXPECT_RESPONSE_HEADER_EQ(image/gif, Content-Type, response); src/tests/master_tests.cpp: response.get().headers.get(Content-Type)); src/tests/master_tests.cpp: response.get().headers.get(Content-Type)); src/tests/master_tests.cpp: response.get().headers.get(Content-Type)); src/tests/master_tests.cpp: response.get().headers.get(Content-Type)); src/tests/master_tests.cpp: response.get().headers.get(Content-Type)); src/tests/master_tests.cpp: response.get().headers.get(Content-Type)); src/tests/master_tests.cpp: response.get().headers.get(Content-Type)); src/tests/master_tests.cpp: response.get().headers.get(Content-Type)); src/tests/master_tests.cpp: response.get().headers.get(Content-Type)); src/tests/metrics_tests.cpp: response.get().headers.get(Content-Type)); src/tests/metrics_tests.cpp: response.get().headers.get(Content-Type)); src/tests/monitor_tests.cpp: Content-Type, src/tests/monitor_tests.cpp: Content-Type, src/tests/monitor_tests.cpp: Content-Type, src/tests/monitor_tests.cpp: Content-Type, src/tests/repair_tests.cpp:Content-Type, \ src/tests/scheduler_tests.cpp: response.get().headers.get(Content-Type)); src/tests/slave_tests.cpp: response.get().headers.get(Content-Type)); src/tests/slave_tests.cpp: response.get().headers.get(Content-Type)); src/tests/slave_tests.cpp: response.get().headers.get(Content-Type)); src/tests/utils.cpp: response.get().headers.get(Content-Type)); ? mesos git:(master) ? grep -R Content-Type 3rdparty | grep -v js | grep -v html 3rdparty/libprocess/examples/example.cpp: response.headers[Content-Type] = text/plain; 3rdparty/libprocess/examples/example.cpp: response.headers[Content-Type] = text/plain; 3rdparty/libprocess/include/process/http.hpp: // specify the 'Content-Type' header, but the 'Content-Length' and 3rdparty/libprocess/include/process/http.hpp: headers[Content-Type] = text/javascript; 3rdparty/libprocess/include/process/process.hpp: // '/path/file'. The 'Content-Type' header of the HTTP response will 3rdparty/libprocess/src/help.cpp:response.headers[Content-Type] = text/x-markdown; 3rdparty/libprocess/src/http.cpp: // Overwrite Content-Type if necessary. 3rdparty/libprocess/src/http.cpp:headers[Content-Type] = contentType.get(); 3rdparty/libprocess/src/http.cpp:return Failure(Attempted to do a POST with a Content-Type but no body); 3rdparty/libprocess/src/http.cpp:return Failure(Attempted to do a POST with a Content-Type but no body); 3rdparty/libprocess/src/process.cpp:// While the user is expected to properly set a 'Content-Type' 3rdparty/libprocess/src/process.cpp:// While the user is expected to properly set a 'Content-Type' 3rdparty/libprocess/src/process.cpp:// Try and determine the Content-Type from an extension. 3rdparty/libprocess/src/process.cpp: response.headers[Content-Type] = assets[name].types[extension]; 3rdparty/libprocess/src/profiler.cpp: response.headers[Content-Type] = application/octet-stream; 3rdparty/libprocess/src/tests/decoder_tests.cpp:Content-Type: text/plain\r\n 3rdparty/libprocess/src/tests/decoder_tests.cpp:Content-Type: text/plain\r\n 3rdparty/libprocess/src/tests/decoder_tests.cpp:Content-Type: text/plain\r\n 3rdparty/libprocess/src/tests/http_tests.cpp: headers[Content-Type] = text/plain; 3rdparty/libprocess/src/tests/http_tests.cpp: EXPECT_EQ(text/javascript, response.headers[Content-Type]); 3rdparty/libprocess/src/tests/system_tests.cpp: response.get().headers.get(Content-Type)); ``` ok :) - Isabel --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36360/#review91205 --- On July 9, 2015, 10:34 p.m., Isabel Jimenez wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36360/
Re: Review Request 36360: Adding common constants for HTTP API
On July 9, 2015, 9:32 p.m., Ben Mahler wrote: Can you move this into the existing common/http.hpp, and remove the content type one? For content type, would rather see a typed member on Request/Response than constants here, given the other occurrences: ``` ? mesos git:(master) ? grep -R Content-Type src | grep -v js | grep -v html src/files/files.cpp: response.headers[Content-Type] = application/octet-stream; src/files/files.cpp: response.headers[Content-Type] = mime::types[extension]; src/tests/fault_tolerance_tests.cpp: Content-Type, src/tests/files_tests.cpp: Content-Type, src/tests/files_tests.cpp: AWAIT_EXPECT_RESPONSE_HEADER_EQ(image/gif, Content-Type, response); src/tests/master_tests.cpp: response.get().headers.get(Content-Type)); src/tests/master_tests.cpp: response.get().headers.get(Content-Type)); src/tests/master_tests.cpp: response.get().headers.get(Content-Type)); src/tests/master_tests.cpp: response.get().headers.get(Content-Type)); src/tests/master_tests.cpp: response.get().headers.get(Content-Type)); src/tests/master_tests.cpp: response.get().headers.get(Content-Type)); src/tests/master_tests.cpp: response.get().headers.get(Content-Type)); src/tests/master_tests.cpp: response.get().headers.get(Content-Type)); src/tests/master_tests.cpp: response.get().headers.get(Content-Type)); src/tests/metrics_tests.cpp: response.get().headers.get(Content-Type)); src/tests/metrics_tests.cpp: response.get().headers.get(Content-Type)); src/tests/monitor_tests.cpp: Content-Type, src/tests/monitor_tests.cpp: Content-Type, src/tests/monitor_tests.cpp: Content-Type, src/tests/monitor_tests.cpp: Content-Type, src/tests/repair_tests.cpp:Content-Type, \ src/tests/scheduler_tests.cpp: response.get().headers.get(Content-Type)); src/tests/slave_tests.cpp: response.get().headers.get(Content-Type)); src/tests/slave_tests.cpp: response.get().headers.get(Content-Type)); src/tests/slave_tests.cpp: response.get().headers.get(Content-Type)); src/tests/utils.cpp: response.get().headers.get(Content-Type)); ? mesos git:(master) ? grep -R Content-Type 3rdparty | grep -v js | grep -v html 3rdparty/libprocess/examples/example.cpp: response.headers[Content-Type] = text/plain; 3rdparty/libprocess/examples/example.cpp: response.headers[Content-Type] = text/plain; 3rdparty/libprocess/include/process/http.hpp: // specify the 'Content-Type' header, but the 'Content-Length' and 3rdparty/libprocess/include/process/http.hpp: headers[Content-Type] = text/javascript; 3rdparty/libprocess/include/process/process.hpp: // '/path/file'. The 'Content-Type' header of the HTTP response will 3rdparty/libprocess/src/help.cpp:response.headers[Content-Type] = text/x-markdown; 3rdparty/libprocess/src/http.cpp: // Overwrite Content-Type if necessary. 3rdparty/libprocess/src/http.cpp:headers[Content-Type] = contentType.get(); 3rdparty/libprocess/src/http.cpp:return Failure(Attempted to do a POST with a Content-Type but no body); 3rdparty/libprocess/src/http.cpp:return Failure(Attempted to do a POST with a Content-Type but no body); 3rdparty/libprocess/src/process.cpp:// While the user is expected to properly set a 'Content-Type' 3rdparty/libprocess/src/process.cpp:// While the user is expected to properly set a 'Content-Type' 3rdparty/libprocess/src/process.cpp:// Try and determine the Content-Type from an extension. 3rdparty/libprocess/src/process.cpp: response.headers[Content-Type] = assets[name].types[extension]; 3rdparty/libprocess/src/profiler.cpp: response.headers[Content-Type] = application/octet-stream; 3rdparty/libprocess/src/tests/decoder_tests.cpp:Content-Type: text/plain\r\n 3rdparty/libprocess/src/tests/decoder_tests.cpp:Content-Type: text/plain\r\n 3rdparty/libprocess/src/tests/decoder_tests.cpp:Content-Type: text/plain\r\n 3rdparty/libprocess/src/tests/http_tests.cpp: headers[Content-Type] = text/plain; 3rdparty/libprocess/src/tests/http_tests.cpp: EXPECT_EQ(text/javascript, response.headers[Content-Type]); 3rdparty/libprocess/src/tests/system_tests.cpp: response.get().headers.get(Content-Type)); ``` Isabel Jimenez wrote: ok :) LOL - there's something to be said about hard-coded strings :) Thanks, Ben! - Marco --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36360/#review91205 --- On July 9, 2015, 10:34 p.m., Isabel Jimenez wrote: --- This
Re: Review Request 36360: Adding common constants for HTTP API
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36360/#review91241 --- Patch looks great! Reviews applied: [36360] All tests passed. - Mesos ReviewBot On July 9, 2015, 10:34 p.m., Isabel Jimenez wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36360/ --- (Updated July 9, 2015, 10:34 p.m.) Review request for mesos, Anand Mazumdar, Benjamin Hindman, Ben Mahler, Marco Massenzio, and Vinod Kone. Bugs: MESOS-2860 https://issues.apache.org/jira/browse/MESOS-2860 Repository: mesos-incubating Description --- Adding constants used commonly through the different HTTP endpoints Diffs - src/common/http.hpp bbd063d src/common/http.cpp 73a4de1 Diff: https://reviews.apache.org/r/36360/diff/ Testing --- Thanks, Isabel Jimenez
Re: Review Request 36360: Adding common constants for HTTP API
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36360/#review91242 --- Ship it! Ship It! - Marco Massenzio On July 9, 2015, 10:34 p.m., Isabel Jimenez wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36360/ --- (Updated July 9, 2015, 10:34 p.m.) Review request for mesos, Anand Mazumdar, Benjamin Hindman, Ben Mahler, Marco Massenzio, and Vinod Kone. Bugs: MESOS-2860 https://issues.apache.org/jira/browse/MESOS-2860 Repository: mesos-incubating Description --- Adding constants used commonly through the different HTTP endpoints Diffs - src/common/http.hpp bbd063d src/common/http.cpp 73a4de1 Diff: https://reviews.apache.org/r/36360/diff/ Testing --- Thanks, Isabel Jimenez
