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

Reply via email to