> On June 6, 2016, 4:30 p.m., Vinod Kone wrote: > > src/master/http.cpp, line 2773 > > <https://reviews.apache.org/r/48116/diff/1/?file=1406733#file1406733line2773> > > > > this should return `v1::master::Response`. see below comment. > > Vinod Kone wrote: > sorry this should return Future<Nothing>
After we change all RPC handlers return type to `Response`, I think could use `Response` here now. > On June 6, 2016, 4:30 p.m., Vinod Kone wrote: > > src/master/http.cpp, line 2951 > > <https://reviews.apache.org/r/48116/diff/1/?file=1406733#file1406733line2951> > > > > why want this function to return `v1::master::Response` instead of > > `http::Response` to be consistent with how we did for others. since this > > response doesn't have a body, this could be an empty `v1::master::Response`. > > haosdent huang wrote: > Hi, @vinodkone, thank you very much for your review. According to the > design document, seems we need return `Accepted` here? > > ``` > Example #2: Call with parameters but no return. > > TEARDOWN_FRAMEWORK HTTP Request (JSON): > POST /api/v1/ HTTP/1.1 > > Host: masterhost:5050 > Content-Type: application/json > > { > “type” : “TEARDOWN_FRAMEWORK”, > > “teardown_network” : { > > “framework_id” : “1242352-1235235-1235235-235235” > > } > } > > TEARDOWN_FRAMEWORK HTTP Response: > HTTP/1.1 202 Accepted > ``` > > Vinod Kone wrote: > teardown happens asynchronously, so we send 202. afaict, updating of > maintenance schedule is done before we return a response? if yes, that should > be a 200? not sure why the original maintenance handler returned 202 in the > first place. @kaysoky? > > haosdent huang wrote: > Oh, got it. So I should return 200 for `UPDATE_MAINTENANCE_SCHEDULE`. > Other question is we want to validate `schedule` with `master->machines`. > > ``` > // Validate that the schedule only transitions machines between > // `UP` and `DRAINING` modes. > Try<Nothing> isValid = maintenance::validation::schedule( > schedule, > master->machines); > ``` > > If change > > ``` > Future<Response> Master::Http::updateMaintenanceSchedule( > const v1::master::Call& call, > const Option<string>& principal) const > ``` > > to > > ``` > Future<v1::master::Response> Master::Http::updateMaintenanceSchedule( > const v1::master::Call& call, > const Option<string>& principal) const > ``` > > seems could not return `BadRequest` in this case. Are there some ways we > could avoid this? > > Joseph Wu wrote: > The original handler returning a 202 was one of the many inconsistencies > of operator endpoints, prior to the Operator API :) Oh, I change to return `OK` now. - haosdent ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/48116/#review136286 ----------------------------------------------------------- On June 9, 2016, 5:31 p.m., haosdent huang wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/48116/ > ----------------------------------------------------------- > > (Updated June 9, 2016, 5:31 p.m.) > > > Review request for mesos and Vinod Kone. > > > Bugs: MESOS-5505 > https://issues.apache.org/jira/browse/MESOS-5505 > > > Repository: mesos > > > Description > ------- > > Implemented UPDATE_MAINTENANCE_SCHEDULE Call in v1 master API. > > > Diffs > ----- > > src/internal/devolve.hpp 00842bb0de1dd587f2b47c79f17c0e7bd7f51189 > src/internal/devolve.cpp 4233246c6838f85189f1b4c7e66d2bc0a3bf5408 > src/master/http.cpp 6e1bf9557a854a89fa9173223295816a9e114e7c > src/master/master.hpp 2c45dab291a153b42809ab12e4252bf58559feeb > > Diff: https://reviews.apache.org/r/48116/diff/ > > > Testing > ------- > > > Thanks, > > haosdent huang > >
