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