You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@mesos.apache.org by Ben Mahler <be...@gmail.com> on 2015/08/03 20:43:06 UTC
Re: Review Request 36987: Extend 'getFormValue' -> 'getFormValues' to
parse input uniformly.
-----------------------------------------------------------
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>.
> 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
>
>