----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36037/#review89977 -----------------------------------------------------------
This looks good, Isabel! Just a few nits about error messages and my being obsessive about Error codes and Response types (HTTP has been around a while, and people have come to expect APIs to behave in a certain way). src/master/http.cpp (line 297) <https://reviews.apache.org/r/36037/#comment142886> from what little I've seen in the codebase, we use the leading underscore for "continuation" methods, is this such a method? If so, can we please make sure that we have the `validate()` method next to it? src/master/http.cpp (lines 308 - 309) <https://reviews.apache.org/r/36037/#comment142887> I really really dislike hard-coded strings sprinkled all over the code (I'm sure you will need them elsewhere: if not now, soon :) Can we please collect them in some "well-known" place? there is a constants.cpp file or we can have a "specialized" http_constants.cpp one (preferred). Bottom line: please avoid hard-coded constants (especially for commonly-used, well-known strings) src/master/http.cpp (line 311) <https://reviews.apache.org/r/36037/#comment142889> is the indent off? src/master/http.cpp (line 313) <https://reviews.apache.org/r/36037/#comment142890> bad error message - we should state that ``` Only `keep-alive` connection header allowed ``` or something to that effect src/master/http.cpp (line 315) <https://reviews.apache.org/r/36037/#comment142891> this is surprising - please make sure to add a comment so that people won't expect `Some(response)` here... src/master/http.cpp (line 331) <https://reviews.apache.org/r/36037/#comment142892> this should be a 405 Method Not Allowed http://www.w3.org/Protocols/rfc2616/rfc2616-sec10.html#sec10.4.6 we could also emit a better error message: ``` return MethodNotAllowed("Only POST allowed, received " + request.method + " instead"); ``` src/master/http.cpp (line 339) <https://reviews.apache.org/r/36037/#comment142893> a better name would be contentType src/master/http.cpp (line 342) <https://reviews.apache.org/r/36037/#comment142894> capitalize the header name in the message too. src/master/http.cpp (line 366) <https://reviews.apache.org/r/36037/#comment142895> this should be a 415 (unsupported media type) see ref URL above also: emit the content type you received in the error message as a courtesy to your users: ``` "Unsupported '" + contentType.get() + "' Content-Type; please only use application/json or application/x-protobuf" ``` or something to that effect. src/tests/call_tests.cpp (line 98) <https://reviews.apache.org/r/36037/#comment142904> can we possibly check on just getting a "type" of response (and not on the actual message)? that would be a better check, so we won't fail tests just because we fix typos in messages :) - Marco Massenzio On June 30, 2015, 9:07 a.m., Isabel Jimenez wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/36037/ > ----------------------------------------------------------- > > (Updated June 30, 2015, 9:07 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 a call route with HTTP request header validations > > > Diffs > ----- > > src/master/http.cpp 3503833 > src/master/master.hpp af83d3e > src/master/master.cpp 0782b54 > src/tests/call_tests.cpp PRE-CREATION > src/tests/mesos.hpp 9157ac0 > > Diff: https://reviews.apache.org/r/36037/diff/ > > > Testing > ------- > > make check > > > Thanks, > > Isabel Jimenez > >
