----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/17255/#review33087 -----------------------------------------------------------
Stoked to see a HELP included from the very beginning! In addition to the comments I made below it would be great to see a simple test. We've got this great little utility in libprocess called 'http::post' which should help you. ;) src/master/http.cpp <https://reviews.apache.org/r/17255/#comment62340> You guys have likely already discussed this, but 'monitor' or 'metric'? src/master/http.cpp <https://reviews.apache.org/r/17255/#comment62341> Likewise, 'level' makes it sound like there will be a spectrum of health. Is that planned for the future? Will it be a spectrum or more like a collection? If so, does s/level/status/ make more sense? These "interface" questions are harder to change later. ;) src/master/http.cpp <https://reviews.apache.org/r/17255/#comment62342> Please wrap lines longer than 80 characters. In this case, please wrap after the '=', indent by 2 on the newline. src/master/http.cpp <https://reviews.apache.org/r/17255/#comment62343> s/string &/string& /g (here and elsewhere please) src/master/http.cpp <https://reviews.apache.org/r/17255/#comment62344> s/http/HTTP/ src/master/http.cpp <https://reviews.apache.org/r/17255/#comment62345> s/if(/if (/ src/master/http.cpp <https://reviews.apache.org/r/17255/#comment62346> s/json/JSON/g (although, it looks like you capitalized it everywhere else) src/master/http.cpp <https://reviews.apache.org/r/17255/#comment62347> FYI, I prefer going through each key explicitly down here versus the loop of keys above. src/master/http.cpp <https://reviews.apache.org/r/17255/#comment62348> Did you need this explicit assignment? src/master/http.cpp <https://reviews.apache.org/r/17255/#comment62351> What about grouping each 'response.values[KEY]' block? Something like: JSON::Object response; // Add 'monitor'. response.values[MONITOR_KEY] = values[MONITOR_KEY]; // Add 'hosts'. vector<string> hosts = strings::split(values[HOSTS_KEY], ","); JSON::Array array; array.values.assign(hosts.begin(), hosts.end()); response.values[HOSTS_KEY] = array; // Add 'level'. bool isHealthy = strings::upper(values[LEVEL_KEY]) == "OK"; // TODO(ccarson): This is a workaround ... response.values[LEVEL_KEY] = (isHealthy ? JSON::Value(JSON::True()) : JSON::False()); return OK(response ... src/master/http.cpp <https://reviews.apache.org/r/17255/#comment62352> s/workaroudn/workaround/ src/master/http.cpp <https://reviews.apache.org/r/17255/#comment62349> Yeah, a JSON::Boolean would be great! src/master/http.cpp <https://reviews.apache.org/r/17255/#comment62350> Do you want to check for 'jsonp'? src/master/http.cpp <https://reviews.apache.org/r/17255/#comment62353> Do you want to keep this in post testing? Assuming you don't include this, I think you can take two tacks for dealing with missing values here: (1) show all missing values and (2) show a missing value when you encounter it. In the latter case I'd prefer to see code above which explicitly checks for each key and returns BadRequest("Missing '" + KEY + "'.") when it is missing something. In addition, the key/value might be present but the value isn't decodable (http::decode fails) or is empty, both of which could be presented immediately as errors. It's not clear to me that there is that much benefit in determining all the ways in which the endpoint was misused (i.e., all the errors) rather than just returning after you encounter the first error. - Benjamin Hindman On Jan. 23, 2014, 7:09 p.m., Charlie Carson wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/17255/ > ----------------------------------------------------------- > > (Updated Jan. 23, 2014, 7:09 p.m.) > > > Review request for mesos, Benjamin Hindman and Jeff Currier. > > > Bugs: https://issues.apache.org/jira/browse/MESOS-880 > > https://issues.apache.org/jira/browse/https://issues.apache.org/jira/browse/MESOS-880 > > > Repository: mesos-git > > > Description > ------- > > Add observe endpoint to master. > > This changes adds a new HTTP endpoint of observe to master. > This allows clients to report health via an HTTP POST. > Values are: > MONITOR = Monitor for which health is being reported. > HOSTS = Comma seperated list of hosts. > LEVEL = OK for healthy, anything else for unhealthy. > > This also contains a small fix to alphabetize the existing > endpoints / help strings. > > SEE: https://issues.apache.org/jira/browse/MESOS-880 > > Review: https://reviews.apache.org/r/17255 > > > Diffs > ----- > > src/master/http.cpp 546e91dbb9c8ee1014bb4f0b3be2714ad6a2d520 > src/master/master.hpp 99b81811d596a777ee83dce7b157c1b11c064fab > src/master/master.cpp c7d9186b9c22fb40b05aa6d5bc6f2d38fb7f73ea > > Diff: https://reviews.apache.org/r/17255/diff/ > > > Testing > ------- > > make check > manually testing - I'll add unit tests when the repair coordinator is > introduced in the next checkin. > > > Thanks, > > Charlie Carson > >
