----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/37303/#review94777 -----------------------------------------------------------
looks like the SchedulerTest.Subscribe test failed? src/common/http.hpp (line 66) <https://reviews.apache.org/r/37303/#comment149373> // Deserializes a string message into a protobuf message // based on the HTTP content type src/common/http.hpp (line 67) <https://reviews.apache.org/r/37303/#comment149378> hmm. this is not as generic as serialize. can't this be templated? template <typename T> Try<T> deserialize(...); src/common/http.hpp (line 73) <https://reviews.apache.org/r/37303/#comment149375> this helper seems weird. i would rather have isOK()and isAccepted() helpers. anyway, this seems to be used only once. do we even need a helper? src/common/http.cpp (line 86) <https://reviews.apache.org/r/37303/#comment149379> add a default case please, in case someone adds a new supported contentType enum. src/common/http.cpp (line 101) <https://reviews.apache.org/r/37303/#comment149380> ditto. add a default case. src/scheduler/scheduler.cpp (line 211) <https://reviews.apache.org/r/37303/#comment149382> why new synchronous function (_send()) here instead of directly plopping the code here? we prefer procedural programming style, i.e., we don't introduce new methods/functions unless it's necesary. src/scheduler/scheduler.cpp <https://reviews.apache.org/r/37303/#comment149383> this should've been in the patch that removed pid related stuff. src/scheduler/scheduler.cpp (line 296) <https://reviews.apache.org/r/37303/#comment149384> s/call type Subscribe/Subscribe call/ src/scheduler/scheduler.cpp (line 314) <https://reviews.apache.org/r/37303/#comment149386> ``` s/handleResponse/_send/ ``` src/scheduler/scheduler.cpp (line 315) <https://reviews.apache.org/r/37303/#comment149388> handling error like this loses the context about the call that caused the error, no? i would just do a .onAny() instead of .then() and .onFailed(). src/scheduler/scheduler.cpp (line 320) <https://reviews.apache.org/r/37303/#comment149385> you should make sure subscribe got an OK and everything else got Accepted. for that this method needs to take Call as argument. src/scheduler/scheduler.cpp (line 322) <https://reviews.apache.org/r/37303/#comment149389> ditto. this should include which call/request failed. src/scheduler/scheduler.cpp (line 334) <https://reviews.apache.org/r/37303/#comment149392> This needs a comment. Why would there be an existing reader? Why is an existing reader not closed as soon as a subscribe call is made? src/scheduler/scheduler.cpp (line 345) <https://reviews.apache.org/r/37303/#comment149390> are all other response types (non PIP) ok to receive? src/scheduler/scheduler.cpp (lines 358 - 359) <https://reviews.apache.org/r/37303/#comment149393> Just say that Events stream filed and include the failure message. src/scheduler/scheduler.cpp (line 375) <https://reviews.apache.org/r/37303/#comment149394> ditto. please don't create new functions unless there is re-use. src/scheduler/scheduler.cpp (line 381) <https://reviews.apache.org/r/37303/#comment149396> When does this happen? Add a comment. Also, sending a "subscribe with master.." with every error event seems a bit redundat? Can we just add documentation to the ERROR event in the protobuf saying so? Or are there events where we dont want schedulers to oversubscribe? src/scheduler/scheduler.cpp (lines 392 - 393) <https://reviews.apache.org/r/37303/#comment149397> When does this happen? src/scheduler/scheduler.cpp (lines 406 - 407) <https://reviews.apache.org/r/37303/#comment149400> include the error please. src/scheduler/scheduler.cpp (line 423) <https://reviews.apache.org/r/37303/#comment149401> make this a LOG(WARNING) src/scheduler/scheduler.cpp (lines 483 - 490) <https://reviews.apache.org/r/37303/#comment149387> ``` process = new MesosProcess( arg1, arg2, arg3, arg4, arg5); ``` - Vinod Kone On Aug. 10, 2015, 4:43 p.m., Anand Mazumdar wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/37303/ > ----------------------------------------------------------- > > (Updated Aug. 10, 2015, 4:43 p.m.) > > > Review request for mesos, Ben Mahler and Vinod Kone. > > > Bugs: MESOS-2552 > https://issues.apache.org/jira/browse/MESOS-2552 > > > Repository: mesos > > > Description > ------- > > This changes moves scheduler library to http. The change to remove auth + > install,receive handlers are in other reviews. > > > Diffs > ----- > > src/common/http.hpp 98a12709ae5fcbf4ce3e37d164bff43818096838 > src/common/http.cpp e2ff48ca609f1c97a1b6c20cc6be3d97c3e4bb50 > src/scheduler/scheduler.cpp a8699a7514892a548eb03f9dcb6a1198ce518830 > > Diff: https://reviews.apache.org/r/37303/diff/ > > > Testing > ------- > > make check > > > Thanks, > > Anand Mazumdar > >
