mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Michael Park" <mcyp...@gmail.com>
Subject Re: Review Request 36987: Extend 'getFormValue' -> 'getFormValues' to parse input uniformly.
Date Mon, 03 Aug 2015 20:09:01 GMT


> 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
> 
>


Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message