You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@mesos.apache.org by Michael Park <mc...@gmail.com> on 2015/07/31 17:12:00 UTC
Review Request 36987: Extend 'getFormValue' -> 'getFormValues' to
handle invalid input uniformly.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/36987/
-----------------------------------------------------------
Review request for mesos and Jie Yu.
Repository: mesos
Description
-------
See summary.
Diffs
-----
src/master/http.cpp 3772e39015a22655dcad00ad844dc5ddc90db43f
src/tests/repair_tests.cpp 4d513ed9e18ebf04a5e5aadb022b064479739e66
Diff: https://reviews.apache.org/r/36987/diff/
Testing
-------
`make check`
Thanks,
Michael Park
Re: Review Request 36987: Extend 'getFormValue' -> 'getFormValues' to
parse input uniformly.
Posted by Mesos ReviewBot <re...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/36987/#review93727
-----------------------------------------------------------
Bad patch!
Reviews applied: [36040, 35934, 35939, 35947, 35702, 35983, 35984]
Failed command: ./support/apply-review.sh -n -r 35984
Error:
2015-07-31 15:36:52 URL:https://reviews.apache.org/r/35984/diff/raw/ [33591/33591] -> "35984.patch" [1]
error: patch failed: src/Makefile.am:1518
error: src/Makefile.am: patch does not apply
Failed to apply patch
- Mesos ReviewBot
On July 31, 2015, 3:12 p.m., Michael Park wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/36987/
> -----------------------------------------------------------
>
> (Updated July 31, 2015, 3:12 p.m.)
>
>
> Review request for mesos and Jie Yu.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> See summary.
>
>
> Diffs
> -----
>
> src/master/http.cpp 3772e39015a22655dcad00ad844dc5ddc90db43f
> src/tests/repair_tests.cpp 4d513ed9e18ebf04a5e5aadb022b064479739e66
>
> Diff: https://reviews.apache.org/r/36987/diff/
>
>
> Testing
> -------
>
> `make check`
>
>
> Thanks,
>
> Michael Park
>
>
Re: Review Request 36987: Extended 'getFormValue' to 'getFormValues'
in order to parse input uniformly.
Posted by Michael Park <mc...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/36987/
-----------------------------------------------------------
(Updated Aug. 5, 2015, 10:38 a.m.)
Review request for mesos, Ben Mahler and Jie Yu.
Changes
-------
Removed temporary variables.
Bugs: MESOS-2600
https://issues.apache.org/jira/browse/MESOS-2600
Repository: mesos
Description
-------
`getFormValues` takes a `Request` object and a list of required keys, and tries to return a map of the required keys to their corresponding values.
This function helps to reduce the amount of code required to check the each required keys manually as well as to keep the missing parameter error message uniform across multiple endpoints.
This is a short-term solution which improves upon `getFormValue`, but in the future we should consider adding a libprocess level support for processing the required and optional keys from a `Request`.
Diffs (updated)
-----
src/master/http.cpp 76e70801925041f08bc94f0ca18c86f1a573b2b3
src/tests/repair_tests.cpp 4d513ed9e18ebf04a5e5aadb022b064479739e66
Diff: https://reviews.apache.org/r/36987/diff/
Testing
-------
`make check`
Thanks,
Michael Park
Re: Review Request 36987: Extended 'getFormValue' to 'getFormValues'
in order to parse input uniformly.
Posted by Michael Park <mc...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/36987/
-----------------------------------------------------------
(Updated Aug. 5, 2015, 10:04 a.m.)
Review request for mesos, Ben Mahler and Jie Yu.
Bugs: MESOS-2600
https://issues.apache.org/jira/browse/MESOS-2600
Repository: mesos
Description
-------
`getFormValues` takes a `Request` object and a list of required keys, and tries to return a map of the required keys to their corresponding values.
This function helps to reduce the amount of code required to check the each required keys manually as well as to keep the missing parameter error message uniform across multiple endpoints.
This is a short-term solution which improves upon `getFormValue`, but in the future we should consider adding a libprocess level support for processing the required and optional keys from a `Request`.
Diffs
-----
src/master/http.cpp 76e70801925041f08bc94f0ca18c86f1a573b2b3
src/tests/repair_tests.cpp 4d513ed9e18ebf04a5e5aadb022b064479739e66
Diff: https://reviews.apache.org/r/36987/diff/
Testing
-------
`make check`
Thanks,
Michael Park
Re: Review Request 36987: Extended 'getFormValue' to 'getFormValues'
in order to parse input uniformly.
Posted by Michael Park <mc...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/36987/
-----------------------------------------------------------
(Updated Aug. 5, 2015, 9:46 a.m.)
Review request for mesos, Ben Mahler and Jie Yu.
Repository: mesos
Description
-------
`getFormValues` takes a `Request` object and a list of required keys, and tries to return a map of the required keys to their corresponding values.
This function helps to reduce the amount of code required to check the each required keys manually as well as to keep the missing parameter error message uniform across multiple endpoints.
This is a short-term solution which improves upon `getFormValue`, but in the future we should consider adding a libprocess level support for processing the required and optional keys from a `Request`.
Diffs (updated)
-----
src/master/http.cpp 76e70801925041f08bc94f0ca18c86f1a573b2b3
src/tests/repair_tests.cpp 4d513ed9e18ebf04a5e5aadb022b064479739e66
Diff: https://reviews.apache.org/r/36987/diff/
Testing
-------
`make check`
Thanks,
Michael Park
Re: Review Request 36987: Extended 'getFormValue' to 'getFormValues'
in order to parse input uniformly.
Posted by Michael Park <mc...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/36987/
-----------------------------------------------------------
(Updated Aug. 5, 2015, 6:05 a.m.)
Review request for mesos, Ben Mahler and Jie Yu.
Changes
-------
* Added a comment
* Removed the implicit empty string check
Summary (updated)
-----------------
Extended 'getFormValue' to 'getFormValues' in order to parse input uniformly.
Repository: mesos
Description (updated)
-------
`getFormValues` takes a `Request` object and a list of required keys, and tries to return a map of the required keys to their corresponding values.
This function helps to reduce the amount of code required to check the each required keys manually as well as to keep the missing parameter error message uniform across multiple endpoints.
This is a short-term solution which improves upon `getFormValue`, but in the future we should consider adding a libprocess level support for processing the required and optional keys from a `Request`.
Diffs (updated)
-----
src/master/http.cpp 76e70801925041f08bc94f0ca18c86f1a573b2b3
src/tests/repair_tests.cpp 4d513ed9e18ebf04a5e5aadb022b064479739e66
Diff: https://reviews.apache.org/r/36987/diff/
Testing
-------
`make check`
Thanks,
Michael Park
Re: Review Request 36987: Extend 'getFormValue' -> 'getFormValues' to
parse input uniformly.
Posted by Michael Park <mc...@gmail.com>.
> On Aug. 3, 2015, 6:43 p.m., Ben Mahler wrote:
> > src/master/http.cpp, lines 361-362
> > <https://reviews.apache.org/r/36987/diff/2/?file=1026437#file1026437line361>
> >
> > Do we even want to keep this? Note that the 'observe' has never been used.
> >
> > I've found this to be a strange helper in the first place, how would we explain what this does in a comment?
> Note that the 'observe' has never been used.
What do you mean? It looks like `/observe` is an exposed endpoint that calls `observe`?
> I've found this to be a strange helper in the first place, how would we explain what this does in a comment?
I should've accompanied it with a comment. My view of its functionality is something like:
```
Takes an HTTP request and the expected list of keys, tries to return the map of expected keys to their corresponding values.
It returns an Error if we failed to parse the request, if an expected key was not provided, or if the corresponding value is an empty string.
Otherwise, the resulting hashmap contains the mapping from expected keys to provided values.
```
I think it's a useful helper function for capturing the above functionality as well as to facilitate providing consistent error messages.
(e.g. for `/observe` we returned "Missing value for 'monitor'.", whereas for `/teardown` we returned "Missing 'frameworkId' query parameter")
- Michael
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/36987/#review93942
-----------------------------------------------------------
On July 31, 2015, 9:56 p.m., Michael Park wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/36987/
> -----------------------------------------------------------
>
> (Updated July 31, 2015, 9:56 p.m.)
>
>
> Review request for mesos and Jie Yu.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> See summary.
>
>
> Diffs
> -----
>
> src/master/http.cpp 3772e39015a22655dcad00ad844dc5ddc90db43f
> src/tests/repair_tests.cpp 4d513ed9e18ebf04a5e5aadb022b064479739e66
>
> Diff: https://reviews.apache.org/r/36987/diff/
>
>
> Testing
> -------
>
> `make check`
>
>
> Thanks,
>
> Michael Park
>
>
Re: Review Request 36987: Extend 'getFormValue' -> 'getFormValues' to
parse input uniformly.
Posted by Ben Mahler <be...@gmail.com>.
> On Aug. 3, 2015, 6:43 p.m., Ben Mahler wrote:
> > src/master/http.cpp, lines 361-362
> > <https://reviews.apache.org/r/36987/diff/2/?file=1026437#file1026437line361>
> >
> > Do we even want to keep this? Note that the 'observe' has never been used.
> >
> > I've found this to be a strange helper in the first place, how would we explain what this does in a comment?
>
> Michael Park wrote:
> > Note that the 'observe' has never been used.
>
> What do you mean? It looks like `/observe` is an exposed endpoint that calls `observe`?
> > I've found this to be a strange helper in the first place, how would we explain what this does in a comment?
>
> I should've accompanied it with a comment. My view of its functionality is something like:
> ```
> Takes an HTTP request and the expected list of keys, tries to return the map of expected keys to their corresponding values.
> It returns an Error if we failed to parse the request, if an expected key was not provided, or if the corresponding value is an empty string.
> Otherwise, the resulting hashmap contains the mapping from expected keys to provided values.
> ```
> I think it's a useful helper function for capturing the above functionality as well as to facilitate providing consistent error messages.
> (e.g. for `/observe` we returned "Missing value for 'monitor'.", whereas for `/teardown` we returned "Missing 'frameworkId' query parameter")
See the TODO in `observe`, it doesn't do anything yet :)
It just seems unfortunate that without such a big comment, one can't easily intuit what this helper does (i.e. not very readable). What are form values anyway? :) How are optional query parameters dealt with? Also the empty string error case seems pretty implicit?
- Ben
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/36987/#review93942
-----------------------------------------------------------
On July 31, 2015, 9:56 p.m., Michael Park wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/36987/
> -----------------------------------------------------------
>
> (Updated July 31, 2015, 9:56 p.m.)
>
>
> Review request for mesos and Jie Yu.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> See summary.
>
>
> Diffs
> -----
>
> src/master/http.cpp 3772e39015a22655dcad00ad844dc5ddc90db43f
> src/tests/repair_tests.cpp 4d513ed9e18ebf04a5e5aadb022b064479739e66
>
> Diff: https://reviews.apache.org/r/36987/diff/
>
>
> Testing
> -------
>
> `make check`
>
>
> Thanks,
>
> Michael Park
>
>
Re: Review Request 36987: Extend 'getFormValue' -> 'getFormValues' to
parse input uniformly.
Posted by Michael Park <mc...@gmail.com>.
> On Aug. 3, 2015, 6:43 p.m., Ben Mahler wrote:
> > src/master/http.cpp, lines 361-362
> > <https://reviews.apache.org/r/36987/diff/2/?file=1026437#file1026437line361>
> >
> > Do we even want to keep this? Note that the 'observe' has never been used.
> >
> > I've found this to be a strange helper in the first place, how would we explain what this does in a comment?
>
> Michael Park wrote:
> > Note that the 'observe' has never been used.
>
> What do you mean? It looks like `/observe` is an exposed endpoint that calls `observe`?
> > I've found this to be a strange helper in the first place, how would we explain what this does in a comment?
>
> I should've accompanied it with a comment. My view of its functionality is something like:
> ```
> Takes an HTTP request and the expected list of keys, tries to return the map of expected keys to their corresponding values.
> It returns an Error if we failed to parse the request, if an expected key was not provided, or if the corresponding value is an empty string.
> Otherwise, the resulting hashmap contains the mapping from expected keys to provided values.
> ```
> I think it's a useful helper function for capturing the above functionality as well as to facilitate providing consistent error messages.
> (e.g. for `/observe` we returned "Missing value for 'monitor'.", whereas for `/teardown` we returned "Missing 'frameworkId' query parameter")
>
> Ben Mahler wrote:
> See the TODO in `observe`, it doesn't do anything yet :)
>
> It just seems unfortunate that without such a big comment, one can't easily intuit what this helper does (i.e. not very readable). What are form values anyway? :) How are optional query parameters dealt with? Also the empty string error case seems pretty implicit?
> See the TODO in observe, it doesn't do anything yet :)
I see.
> What are form values anyway? :)
I inherited this from the `getFormValue` function which existed prior to this. My guess was that it refers to HTML Forms.
> How are optional query parameters dealt with?
This is a good point, I think perhaps what we ideally want is something closer to a commang-line parser, where we can specify required/optional keys?
I still think this is a good start to that level of abstraction though. MVP so-to-speak I guess?
> Also the empty string error case seems pretty implicit?
I agree, also inherited from the `getFormValue`. Would be happy to remove this implicit behavior.
- Michael
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/36987/#review93942
-----------------------------------------------------------
On July 31, 2015, 9:56 p.m., Michael Park wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/36987/
> -----------------------------------------------------------
>
> (Updated July 31, 2015, 9:56 p.m.)
>
>
> Review request for mesos and Jie Yu.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> See summary.
>
>
> Diffs
> -----
>
> src/master/http.cpp 3772e39015a22655dcad00ad844dc5ddc90db43f
> src/tests/repair_tests.cpp 4d513ed9e18ebf04a5e5aadb022b064479739e66
>
> Diff: https://reviews.apache.org/r/36987/diff/
>
>
> Testing
> -------
>
> `make check`
>
>
> Thanks,
>
> Michael Park
>
>
Re: Review Request 36987: Extend 'getFormValue' -> 'getFormValues' to
parse input uniformly.
Posted by Ben Mahler <be...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/36987/#review93942
-----------------------------------------------------------
src/master/http.cpp (lines 361 - 362)
<https://reviews.apache.org/r/36987/#comment148373>
Do we even want to keep this? Note that the 'observe' has never been used.
I've found this to be a strange helper in the first place, how would we explain what this does in a comment?
- Ben Mahler
On July 31, 2015, 9:56 p.m., Michael Park wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/36987/
> -----------------------------------------------------------
>
> (Updated July 31, 2015, 9:56 p.m.)
>
>
> Review request for mesos and Jie Yu.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> See summary.
>
>
> Diffs
> -----
>
> src/master/http.cpp 3772e39015a22655dcad00ad844dc5ddc90db43f
> src/tests/repair_tests.cpp 4d513ed9e18ebf04a5e5aadb022b064479739e66
>
> Diff: https://reviews.apache.org/r/36987/diff/
>
>
> Testing
> -------
>
> `make check`
>
>
> Thanks,
>
> Michael Park
>
>
Re: Review Request 36987: Extend 'getFormValue' -> 'getFormValues' to
parse input uniformly.
Posted by Michael Park <mc...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/36987/
-----------------------------------------------------------
(Updated July 31, 2015, 9:56 p.m.)
Review request for mesos and Jie Yu.
Changes
-------
Reordered the patches.
Repository: mesos
Description
-------
See summary.
Diffs (updated)
-----
src/master/http.cpp 3772e39015a22655dcad00ad844dc5ddc90db43f
src/tests/repair_tests.cpp 4d513ed9e18ebf04a5e5aadb022b064479739e66
Diff: https://reviews.apache.org/r/36987/diff/
Testing
-------
`make check`
Thanks,
Michael Park
Re: Review Request 36987: Extend 'getFormValue' -> 'getFormValues' to
parse input uniformly.
Posted by Michael Park <mc...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/36987/
-----------------------------------------------------------
(Updated July 31, 2015, 3:12 p.m.)
Review request for mesos and Jie Yu.
Summary (updated)
-----------------
Extend 'getFormValue' -> 'getFormValues' to parse input uniformly.
Repository: mesos
Description
-------
See summary.
Diffs
-----
src/master/http.cpp 3772e39015a22655dcad00ad844dc5ddc90db43f
src/tests/repair_tests.cpp 4d513ed9e18ebf04a5e5aadb022b064479739e66
Diff: https://reviews.apache.org/r/36987/diff/
Testing
-------
`make check`
Thanks,
Michael Park