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

Reply via email to