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