Re: Review Request 36037: Adding /call endpoint to Master
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36037/#review95503 --- what's the status of this one? Should it be closed/discarded? - Marco Massenzio On July 2, 2015, 8:16 a.m., Isabel Jimenez wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36037/ --- (Updated July 2, 2015, 8:16 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/Makefile.am a064d17 src/master/http.cpp 2be613b src/master/http_constants.hpp PRE-CREATION src/master/http_constants.cpp PRE-CREATION src/master/master.hpp af83d3e src/master/master.cpp a7486d8 src/master/validation.hpp 469d6f5 src/master/validation.cpp 9d128aa 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
Re: Review Request 36037: Adding /call endpoint to Master
On July 3, 2015, 12:29 a.m., Ben Mahler wrote: I chatted with Isabel on IRC and asked her to break apart this change into more bite-sized chunks, so that we can do smaller reviews and get things committed incrementally: (1) Dummy /call handler on the master. (2) Validation. (3) Partial implementation of Call (i.e. parsing logic). Each part can have its own tests. She will be discarding this review in favor of smaller chunks, which we can commit incrementally. :) I also asked her to: (a) Punt on the constants and remove master/http_constants.hpp, since these constants aren't adding value (CLOSE - close) for the added indirection, and our existing code doesn't follow this pattern. (b) Pull out the change to src/tests/mesos.hpp, since it is independent. Marco Massenzio wrote: All good. However, I beg to disagree on this point: (a) Punt on the constants and remove master/http_constants.hpp, since these constants aren't adding value (CLOSE - close) for the added indirection, and our existing code doesn't follow this pattern. We *do* have a `constants.hpp` (and relative .cpp) file and I do believe it does add value (I, for one, certainly appreciated having it when I did the JSON/ZK change ;) ): for example, I've already seen the string `application/x-protobuf` typed up 10 times in just two reviews: there is value in having an APPLICATION_PROTOBUF constant to: - avoid difficult-to-spot bugs to typos (`application/x-prolobuf`) that may only surface at runtime in production; - avoid typing the same stuff again and again (especially those of us using modern IDEs can take advantage of code-completion ;) ) - this is anyway common standard good practice and would allow us to not having to agonize too much in case we need to refactor something (say, at some point we want to use `application/x-protobuf-binary` for whatever reason - there's only one place to do so; sure, this is an unlikely example, but there may be cases where it may not be so far-fetched). Also, *not* doing it does not save (I think?) any effort in reviewing and/or committing, so seems very low cost for a potential sizeable payoff. Ah, yes, and there's also the fact that hard-coded strings sprinkled all over the code base are hard to maintain - I know, I've had to pick up the pieces at least twice in the last 4 years ;) PS - I do agree that defining `const string CLOSE = close` may be pushing this one step too far... but I'd like to retain it for those more commonly used strings. Ben Mahler wrote: I don't think we're in disagreement, I just want this to be punted so that we can think carefully about how to improve 'Request' and 'Response' usage, rather than bundling it in this code review. Doing more than one thing in a review tends to drag out the review, and I didn't want Isabel to get distracted with this. So let's follow up! In particular, having http constants in master/http_constants.hpp is strange because it isn't master specific (we have common/http.hpp for ones relevant to many components in mesos, http.hpp for libprocess). Also, where possible, I'm hoping to avoid the difficulty in header map manipulation entirely by providing typed members (there's a [TODO](https://github.com/apache/mesos/blob/0.23.0-rc1/3rdparty/libprocess/include/process/http.hpp#L107) which briefly alludes to this). For example, `request.connection` could be an enum to capture the possible connection types. I don't think we're in disagreement Awesome! :) Doing more than one thing in a review tends to drag out the review, Completely agree, I just thought that factoring out the constants at the outset was rather uncontroversial and best done now rather than have to refactor later. having http constants in master/http_constants.hpp yep - I had actually missed that: do you have a better suggestion? (maybe `common/api_constants.hpp` could be a better place/name? something else?) For example, request.connection could be an enum to capture the possible connection types. Oh, yes - totally: typed enum string constant hard-coded string :) - Marco --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36037/#review90302 --- On July 2, 2015, 8:16 a.m., Isabel Jimenez wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36037/ --- (Updated July 2, 2015, 8:16 a.m.) Review request for mesos, Anand Mazumdar, Benjamin Hindman, Ben Mahler, Marco Massenzio, and Vinod Kone. Bugs: MESOS-2860
Re: Review Request 36037: Adding /call endpoint to Master
On July 3, 2015, 12:29 a.m., Ben Mahler wrote: I chatted with Isabel on IRC and asked her to break apart this change into more bite-sized chunks, so that we can do smaller reviews and get things committed incrementally: (1) Dummy /call handler on the master. (2) Validation. (3) Partial implementation of Call (i.e. parsing logic). Each part can have its own tests. She will be discarding this review in favor of smaller chunks, which we can commit incrementally. :) I also asked her to: (a) Punt on the constants and remove master/http_constants.hpp, since these constants aren't adding value (CLOSE - close) for the added indirection, and our existing code doesn't follow this pattern. (b) Pull out the change to src/tests/mesos.hpp, since it is independent. Marco Massenzio wrote: All good. However, I beg to disagree on this point: (a) Punt on the constants and remove master/http_constants.hpp, since these constants aren't adding value (CLOSE - close) for the added indirection, and our existing code doesn't follow this pattern. We *do* have a `constants.hpp` (and relative .cpp) file and I do believe it does add value (I, for one, certainly appreciated having it when I did the JSON/ZK change ;) ): for example, I've already seen the string `application/x-protobuf` typed up 10 times in just two reviews: there is value in having an APPLICATION_PROTOBUF constant to: - avoid difficult-to-spot bugs to typos (`application/x-prolobuf`) that may only surface at runtime in production; - avoid typing the same stuff again and again (especially those of us using modern IDEs can take advantage of code-completion ;) ) - this is anyway common standard good practice and would allow us to not having to agonize too much in case we need to refactor something (say, at some point we want to use `application/x-protobuf-binary` for whatever reason - there's only one place to do so; sure, this is an unlikely example, but there may be cases where it may not be so far-fetched). Also, *not* doing it does not save (I think?) any effort in reviewing and/or committing, so seems very low cost for a potential sizeable payoff. Ah, yes, and there's also the fact that hard-coded strings sprinkled all over the code base are hard to maintain - I know, I've had to pick up the pieces at least twice in the last 4 years ;) PS - I do agree that defining `const string CLOSE = close` may be pushing this one step too far... but I'd like to retain it for those more commonly used strings. I don't think we're in disagreement, I just want this to be punted so that we can think carefully about how to improve 'Request' and 'Response' usage, rather than bundling it in this code review. Doing more than one thing in a review tends to drag out the review, and I didn't want Isabel to get distracted with this. So let's follow up! In particular, having http constants in master/http_constants.hpp is strange because it isn't master specific (we have common/http.hpp for ones relevant to many components in mesos, http.hpp for libprocess). Also, where possible, I'm hoping to avoid the difficulty in header map manipulation entirely by providing typed members (there's a [TODO](https://github.com/apache/mesos/blob/0.23.0-rc1/3rdparty/libprocess/include/process/http.hpp#L107) which briefly alludes to this). For example, `request.connection` could be an enum to capture the possible connection types. - Ben --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36037/#review90302 --- On July 2, 2015, 8:16 a.m., Isabel Jimenez wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36037/ --- (Updated July 2, 2015, 8:16 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/Makefile.am a064d17 src/master/http.cpp 2be613b src/master/http_constants.hpp PRE-CREATION src/master/http_constants.cpp PRE-CREATION src/master/master.hpp af83d3e src/master/master.cpp a7486d8 src/master/validation.hpp 469d6f5 src/master/validation.cpp 9d128aa 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
Re: Review Request 36037: Adding /call endpoint to Master
On July 3, 2015, 12:29 a.m., Ben Mahler wrote: I chatted with Isabel on IRC and asked her to break apart this change into more bite-sized chunks, so that we can do smaller reviews and get things committed incrementally: (1) Dummy /call handler on the master. (2) Validation. (3) Partial implementation of Call (i.e. parsing logic). Each part can have its own tests. She will be discarding this review in favor of smaller chunks, which we can commit incrementally. :) I also asked her to: (a) Punt on the constants and remove master/http_constants.hpp, since these constants aren't adding value (CLOSE - close) for the added indirection, and our existing code doesn't follow this pattern. (b) Pull out the change to src/tests/mesos.hpp, since it is independent. All good. However, I beg to disagree on this point: (a) Punt on the constants and remove master/http_constants.hpp, since these constants aren't adding value (CLOSE - close) for the added indirection, and our existing code doesn't follow this pattern. We *do* have a `constants.hpp` (and relative .cpp) file and I do believe it does add value (I, for one, certainly appreciated having it when I did the JSON/ZK change ;) ): for example, I've already seen the string `application/x-protobuf` typed up 10 times in just two reviews: there is value in having an APPLICATION_PROTOBUF constant to: - avoid difficult-to-spot bugs to typos (`application/x-prolobuf`) that may only surface at runtime in production; - avoid typing the same stuff again and again (especially those of us using modern IDEs can take advantage of code-completion ;) ) - this is anyway common standard good practice and would allow us to not having to agonize too much in case we need to refactor something (say, at some point we want to use `application/x-protobuf-binary` for whatever reason - there's only one place to do so; sure, this is an unlikely example, but there may be cases where it may not be so far-fetched). Also, *not* doing it does not save (I think?) any effort in reviewing and/or committing, so seems very low cost for a potential sizeable payoff. Ah, yes, and there's also the fact that hard-coded strings sprinkled all over the code base are hard to maintain - I know, I've had to pick up the pieces at least twice in the last 4 years ;) PS - I do agree that defining `const string CLOSE = close` may be pushing this one step too far... but I'd like to retain it for those more commonly used strings. - Marco --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36037/#review90302 --- On July 2, 2015, 8:16 a.m., Isabel Jimenez wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36037/ --- (Updated July 2, 2015, 8:16 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/Makefile.am a064d17 src/master/http.cpp 2be613b src/master/http_constants.hpp PRE-CREATION src/master/http_constants.cpp PRE-CREATION src/master/master.hpp af83d3e src/master/master.cpp a7486d8 src/master/validation.hpp 469d6f5 src/master/validation.cpp 9d128aa 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
Re: Review Request 36037: Adding /call endpoint to Master
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36037/ --- (Updated July 2, 2015, 8:16 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 (updated) - src/Makefile.am a064d17 src/master/http.cpp 2be613b src/master/http_constants.hpp PRE-CREATION src/master/http_constants.cpp PRE-CREATION src/master/master.hpp af83d3e src/master/master.cpp a7486d8 src/master/validation.hpp 469d6f5 src/master/validation.cpp 9d128aa 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
Re: Review Request 36037: Adding /call endpoint to Master
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36037/#review90302 --- I chatted with Isabel on IRC and asked her to break apart this change into more bite-sized chunks, so that we can do smaller reviews and get things committed incrementally: (1) Dummy /call handler on the master. (2) Validation. (3) Partial implementation of Call (i.e. parsing logic). Each part can have its own tests. She will be discarding this review in favor of smaller chunks, which we can commit incrementally. :) I also asked her to: (a) Punt on the constants and remove master/http_constants.hpp, since these constants aren't adding value (CLOSE - close) for the added indirection, and our existing code doesn't follow this pattern. (b) Pull out the change to src/tests/mesos.hpp, since it is independent. - Ben Mahler On July 2, 2015, 8:16 a.m., Isabel Jimenez wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36037/ --- (Updated July 2, 2015, 8:16 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/Makefile.am a064d17 src/master/http.cpp 2be613b src/master/http_constants.hpp PRE-CREATION src/master/http_constants.cpp PRE-CREATION src/master/master.hpp af83d3e src/master/master.cpp a7486d8 src/master/validation.hpp 469d6f5 src/master/validation.cpp 9d128aa 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
Re: Review Request 36037: Adding /call endpoint to Master
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36037/ --- (Updated July 1, 2015, 6:52 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 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
Re: Review Request 36037: Adding /call endpoint to Master
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36037/#review90019 --- I haven't looked at the patch itself, but looks like reviewbot is failing to apply patches due to irregular dependencies. This patch depends on 35934 and 36073. 36073 in turn also depends on 35934 and I think that will cause reviewbot to fail too. Would it be possible to put the patch dependencies in a linear fashion, that way, it's easier to review as well :-). - Kapil Arya On June 30, 2015, 9:52 p.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:52 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 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
Re: Review Request 36037: Adding /call endpoint to Master
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36037/#review90102 --- Adding some minor test-based comments. src/tests/call_tests.cpp (line 85) https://reviews.apache.org/r/36037/#comment143078 From the design doc : https://docs.google.com/document/d/1pnIY_HckimKNvpqhKRhbc9eSItWNFT-priXh_urR-T0/edit# We need to send back a OK with a PIPED stream as a response with the Transfer-Encoding: chunked header set. We need to test for the header too and the response status would not be a Accepted 202 here. src/tests/call_tests.cpp (line 222) https://reviews.apache.org/r/36037/#comment143074 From the HTTP spec : If no Accept header field is present, then it is assumed that the client accepts all media types, so this test is not valid. In essence , we need to test for an accept type which the server can't support that we already seem to be doing in CallEndpointWrongHeaderAccept, so we can delete this test. - Anand Mazumdar On July 1, 2015, 6:52 p.m., Isabel Jimenez wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36037/ --- (Updated July 1, 2015, 6:52 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 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
Re: Review Request 36037: Adding /call endpoint to Master
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36037/#review90188 --- Ship it! src/master/http.cpp (line 322) https://reviews.apache.org/r/36037/#comment143187 Another great candidate for the constants file: `Content-Type` src/master/http.cpp (lines 349 - 350) https://reviews.apache.org/r/36037/#comment143188 this is a micro-nit (feel free to ignore) my impression is that the message would read better if you were to invert the order of the actual type and the name of the header: ``` Unsupported Content-Type: ' + contentType.get() + '; Expecting one of ( + APPLICATION_PROTOBUF + , + APPLICATION_JSON +); ``` so, if we add more, the comment can be easily extended. src/master/validation.cpp (line 86) https://reviews.apache.org/r/36037/#comment143186 thanks for adding the comment! Ideally, this should be part of the javadoc at the top of the method? so people reading the API will see the @return value also, missing a `d`: *determined* src/master/http_constants.hpp (line 26) https://reviews.apache.org/r/36037/#comment143189 note if you use the javadoc notation: ``` /** * Supported Content-Tye and Accept headers */ ``` you will get this added to our Doxygen free of charge :) This looks good, Isabel! - Marco Massenzio On July 1, 2015, 7:30 p.m., Isabel Jimenez wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36037/ --- (Updated July 1, 2015, 7:30 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 a call route with HTTP request header validations Diffs - src/Makefile.am a064d17 src/master/http.cpp 2be613b src/master/http_constants.hpp PRE-CREATION src/master/http_constants.cpp PRE-CREATION src/master/master.hpp af83d3e src/master/master.cpp 34ce744 src/master/validation.hpp 469d6f5 src/master/validation.cpp 9d128aa 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
Re: Review Request 36037: Adding /call endpoint to Master
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36037/ --- (Updated July 2, 2015, 2:05 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 (updated) - src/Makefile.am a064d17 src/master/http.cpp 2be613b src/master/http_constants.hpp PRE-CREATION src/master/http_constants.cpp PRE-CREATION src/master/master.hpp af83d3e src/master/master.cpp a7486d8 src/master/validation.hpp 469d6f5 src/master/validation.cpp 9d128aa 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
Re: Review Request 36037: Adding /call endpoint to Master
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36037/#review90017 --- Bad patch! Reviews applied: [35934, 36073, 35934] Failed command: ./support/apply-review.sh -n -r 35934 Error: 2015-07-01 06:39:05 URL:https://reviews.apache.org/r/35934/diff/raw/ [1774/1774] - 35934.patch [1] error: patch failed: 3rdparty/libprocess/include/process/http.hpp:377 error: 3rdparty/libprocess/include/process/http.hpp: patch does not apply Failed to apply patch - Mesos ReviewBot On July 1, 2015, 1:52 a.m., Isabel Jimenez wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36037/ --- (Updated July 1, 2015, 1:52 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
Re: Review Request 36037: Adding /call endpoint to Master
--- 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
Re: Review Request 36037: Adding /call endpoint to Master
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36037/ --- (Updated July 1, 2015, 1:43 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
Re: Review Request 36037: Adding /call endpoint to Master
--- 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
